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.

@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.