ERC-721 implementation uses static calls of override functions, and can cause problems when subclassed

[fulldecent]

This issue was originally alluded in #2784 (comment).

Executive summary

In certain versions of OpenZeppelin Contracts, properly subclassing the ERC721 contract in certain supported ways required you override dependent functions other than the one you want to change. If you did not do this, you could create a vulnerable contract.

Introduction

So basically people are using OZC ERC721 and then overriding isApprovedForAll (and maybe other stuff). These people would add their own rules for who has access in here. But they didn't realize they also needed to reimplement everything in the upstream ERC721 functions that called isApprovedForAll statically. And so tokens can be transferred not respecting the purported permissions of isApprovedForAll.

This highlights the fact that subclassing (i.e. overriding) a contract is a complicated process for people who know what they are doing.

Affected versions

This was introduced at 18c7efe (v3.4.0-rc.0~1)

And fixed at 3dc374d (v4.1.0-rc.0~21)

Impact notes

Who would do this... right? Well actually OpenSea was recommending everyone to use OpenZeppelin Contracts and to make that specific change for access control. So there's a bunch of contracts that did this using the affected OZC versions.

Quick notes:

1. I have already fixed this with OpenSea.
2. I found this exact case on an NFT audit I'm doing.
3. About ~600 deployed NFT contracts on Ethereum Mainnet are affected.
4. I reviewed ~10 popular ones and don't see anywhere this causes an obvious exploitable vulnerability.
5. Thank you to Etherscan and their great tools for making the above quick research possible!

But there are many NFT contracts which are deployed with the affected versions, and which are following advice from OpenSea... so there is probably a vulnerability somewhere in the wild from this.

Recommendations

1. I am not clear if old versions of OZC are continually supported (Supported versions is not specified #2759). If they are, please apply 3dc374d, release new versions, then deprecate the affected versions.
2. In all contracts which allow subclassing (have virtual), add a contract-level comment, inside the docblock, providing advice and caveats for subclassing. Apple's Swift documentation is an example of developer documentation with great notes for subclassing. One such page is: https://developer.apple.com/documentation/appkit/nsanimation

[frangio]

Thanks for the report. We're evaluating whether the fix should be backported to 3.x.

As a reminder for everyone when overriding functions:

In many cases overriding a function will not imply that other functions in the contract will automatically adapt to the overridden definitions. For example, overriding ERC20.balanceOf does not guarantee that that balance is available for transferring. However, it will generaly be possible to obtain the desired behavior by overriding other functions in the contract, in this case ERC20._transfer. Understand that this limitation was necessary to keep the contracts simple and consistent, but it means that the responsibility to ensure correctness when functions are overriden will be on the users and not on the OpenZeppelin maintainers. People who wish to override should consult the source code to fully understand the impact it will have, and should consider whether they need to override additional functions to achieve the desired behavior. As always, feel free to ask for help in the OpenZeppelin Forum and we'll be happy to help.

[frangio]

Closing in favor of more general issue:

• Stop using hardcoded resolution of functions #3883