Base file I used to use for my mainnet contracts to be compatible with OpenSea

README.md

Warning !!!

Recent events have shown that the auto-approval for user proxies is way too dangerous.

Security of users' NFTs should come before the convenience of auto approving collection for trading.

This is why I decided to remove the files in this gist.

Security risk

Since a few weeks, people are loosing NFTs because of approvals on OpenSea.

It's in no way OpenSea's fault, it's because some bad people are phishing signatures of sale orders.

The only way to protect against this, is for users to revoke the approval they gave to OpenSea, using tools like etherscan approval checker or other revoke apps.

However this is not possible if there is no event emitted. Those website can not know that an approval has been given. Users are not even aware that their NFTs are approved on OpenSea, but still if they get phished, might see them disappear anyway.

Personal thoughts

This pattern seemed convenient a few months back, but experience have just shown that it's too dangerous to have it used, especially without any way to revoke it.

Users wanting to sell on Marketplaces are expecting to make a profit. Therefore my opinion is that they should take the cost of approval into account when they calculate their possible profit, and include that in their sale price.

It's a small price compared to the security of their tokens if they don't try to sell it.

Any idea why the opensea-creature implementation has more logic than portrayed here? for example,
Also, any idea why the logic is different in here, https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/CreatureFactory.sol#L148
compared to this one here (which is similar to what you have implemented), https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/ERC721Tradable.sol#L77

Also, any idea on the use of ContextMixin here?
https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/ERC721Tradable.sol#L101

I too feel stupid, could you pls explain how opensea interacts with our contract when minting/transfering? I know they use operator-proxy contract, but which methods they are calling, It seems they dont call 'mint' at all, but does transfer always, which seems to be the case with opensea-creature template atleast.

https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/CreatureFactory.sol#L148

I have no idea what this is but it should NEVER be used in production. It returns the contract owner as the owner of each and every nfts. It's probably a simple example. Do not use this factory anywhere in prod.

https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/ERC721Tradable.sol#L77

This is the "normal" use of this proxy registry, however it can fail because they don't check if the proxyRegistry is not null, so it will fail if proxyRegistry is not set.

https://github.com/ProjectOpenSea/opensea-creatures/blob/74e24b99471380d148057d5c93115dfaf9a1fa9e/contracts/ERC721Tradable.sol#L101

they use ContextMixin because on Polygon, they do MetaTransaction, so the transfer are called by them with a signature by the real owner.
MetaTransaction allows a tier to do a transaction for an account, using a signature.

thanks! shouldn't the _setOptOutForAccount be allowed to be called from public?

I updated that quickly a few days ago and didn't have time to finish creating the public interface for it.

To be honest I'm really thinking deprecating this gist. Because using this proxyRegistry is actually pretty dangerous, for many reasons.
That's a pattern that can be critical very quicly and I think OS should change their market contract for something not using it.

Recent events have shown that this auto-approval for user proxies can be dangerous in some cases. Because this auto approval doesn't trigger any event, it is not possible for users to find it nor to revoke it.

Can you please provide some links about this?

I was thinking of approving OpenSea's and Rarible's proxies.

Can you please provide some links about this?

Well, since you auto approve the proxies, there are no events emitted.

And since you have it auto approved in the code, there is no way to revoke the approval.

Which makes it pretty dangerous, especially in light of recent events.

since you have it auto approved in the code

So would a simple solution be to write an onlyOwner function that enables possibility to update proxy addresses?

So would a simple solution be to write an onlyOwner function that enables possibility to update proxy addresses?

I think the simplest solution would be to not implement this. The last few weeks have shown that it's too dangerous (and I can think of 2 attack vectors that could make this even more dangerous).

Especially because this approval does not emit any event, which makes "revoke helpers" websites completely unaware of its existence.

Users that want their NFTs approved on OpenSea are expecting to make a profit. They should simply include the cost of doing the setApprovalForAll in their calculations.

---

I'm going to remove this gist for this reason. I created it a few months ago when this was pretty unknown, and thought it was a good thing.
Experience have shown us it's not a good pattern.

@dievardump , please provide some links where the risks are discussed. This gist is the only place so far that I've come across that mentions risks, so I'd like to read others' opinions. Thanks.

Since a few weeks, people are loosing NFTs because of approvals on OpenSea.

It's not OpenSea's fault, it's because some bad people are phishing signatures of sale orders.

The only way to protect against this, is for users to revoke the approval they gave to OpenSea, using tools like etherscan approval checker or other revoke apps.
However this is not possible if there is no event emitted. Users that are collectors and not sellers will not even know that their NFTs are approved on OpenSea and might see them disappear anyway.

This should already be enough to stop using this implementation.

I'm also aware of 2 other vector of attack linked to the use of the proxy pattern, that I won't discuss in a public space.

@dievardump I understand and appreciate your concerns regarding opensea's proxy, yet, I'd still want to give it a look as my only reference is the creature contract they provide, is there a way for me to get my hands into this?
Also, would love to know your thoughts regarding their contract migration and wether it affects the proxy/gasless listing discussed here or not, I don't even know if they registry address changed at this point 😐

@dievardump However, if we don't use proxies, OpenSea will return error message every time we buy or sell NFTs on Polygon. OpenSea doesn't really consider transactions that require gas bills will be carried out. For this reason, without proxies, buyers feel uneasy or troublesome and don't buy. Is there a alternative solution to this?

Also, would love to know your thoughts regarding their contract migration and wether it affects the proxy/gasless listing discussed here or not, I don't even know if they registry address changed at this point neutral_face

The contract doesn't affect the proxy / galess stuff

@dievardump However, if we don't use proxies, OpenSea will return error message every time we buy or sell NFTs on Polygon. OpenSea doesn't really consider transactions that require gas bills will be carried out. For this reason, without proxies, buyers feel uneasy or troublesome and don't buy. Is there a alternative solution to this?

Polygon is something else since, for a reason beyond my understanding, it's mandatory to use it to be able to list on OpenSea.
edit: error on my side

Isn't overriding isApprovedForAll() optional (even for Polygon), just to reduce trading friction? In OpenSea's documentation it doesn't say it's compulsory: https://docs.opensea.io/docs/polygon-basic-integration#overriding-isapprovedforall-to-reduce-trading-friction

It looks like an option, but if you don't override it, you'll get a lot of errors and a decent mind person will hesitate to buy or sell.

Guys can someone remind me the OpenSea proxy registry addresses for: Rinkeby, ETH mainnet and Polygon?

@villarode I'm not sure if those are correct but this is their repo so maybe - https://github.com/ProjectOpenSea/opensea-creatures/blob/master/migrations/2_deploy_contracts.js#L29

Hi @dievardump, is that possible to emit the "approve" event upon mint or transfer? So at least revoke website will be aware of it.

hi @dievardump ,

very interesting topic, I see many contract auto-approuve OS proxy registry like this one :

function isApprovedForAll(address owner, address operator) public view override returns (bool) {
ProxyRegistry proxyRegistry = ProxyRegistry(openSeaProxyRegistryAddress);
if (
isOpenSeaProxyActive && address(proxyRegistry.proxies(owner)) == operator
) {
return true;
}
return super.isApprovedForAll(owner, operator);
}

I understood from this thread that :

• it's a bad practice because no approval event is triggered, so no one knows about an approval.
• the approval cannot be revoked.
• it might lead to an exploit

On that last point, I don't understand how, do you have an example of a past exploit or a link that explains it in detail?

No I do not want to spread the information on how this can be exploited.
The less people know about how to do that, the better.

---

Furthermore, with the new SeaPort the OpenSea registry doesn't seem to be used anymore (at least people need to reapprove collections to list) so I imagine that this topic is really not needed anymore.

I haven't check SeaPort implementation, but isn't only a new operator addr ? or is it a logic update ?
In case it's only an addr update then the exploit is probably still the same unless the contract doesn't update the openSeaProxyRegistryAddress variable (from code above).

Thinking about the problem I came out with this solution.
With a boolean in the "mint" function, we can ask the user if they want to approve the collection at mint time for popular platforms.
Then instead of override "isApprovedForAll" we call _setApprovalForAll so :

• user is aware approvals
• he choose the approvals
• the ApprovalForAll is emit so 3rd party app can read it and propose a disapprove

@dievardump what do you think about it ?

    function mint(bool _approvalOS, bool _approvalLR, bool _approvalX2Y2) public {
        [...]
    
        _mint(msg.sender, _tokenIds.current());

        if (_approvalOS){
        _setApprovalForAll(msg.sender, OS_OPERATOR, true);
        }
        if (_approvalLR){
        _setApprovalForAll(msg.sender, LR_OPERATOR, true);
        }
        if (_approvalX2Y2){
        _setApprovalForAll(msg.sender, X2Y2_OPERATOR, true);
        }
    }

I haven't check SeaPort implementation, but isn't only a new operator addr ? or is it a logic update ? In case it's only an addr update then the exploit is probably still the same unless the contract doesn't update the openSeaProxyRegistryAddress variable (from code above).

Thinking about the problem I came out with this solution. With a boolean in the "mint" function, we can ask the user if they want to approve the collection at mint time for popular platforms. Then instead of override "isApprovedForAll" we call _setApprovalForAll so :

• user is aware approvals
• he choose the approvals
• the ApprovalForAll is emit so 3rd party app can read it and propose a disapprove

@dievardump what do you think about it ?

    function mint(bool _approvalOS, bool _approvalLR, bool _approvalX2Y2) public {
        [...]
    
        _mint(msg.sender, _tokenIds.current());

        if (_approvalOS){
        _setApprovalForAll(msg.sender, OS_OPERATOR, true);
        }
        if (_approvalLR){
        _setApprovalForAll(msg.sender, LR_OPERATOR, true);
        }
        if (_approvalX2Y2){
        _setApprovalForAll(msg.sender, X2Y2_OPERATOR, true);
        }
    }

As I understand, this should cost extra gas similar to call setApprovalForAll afterward. But yes it should save a little bit of gas comparing to call it separately.