ERC721: static call for ownerOf function

[fulldecent]

In the ERC721 contract, the ownerOf function is defined as virtual which allows implementations to change this function.

But invocations of this function do NOT call with virtual dispatch (i.e. they use ERC721.ownerOf).

This creates the possibility somebody will override it and it will stop working.

Proposed solutions:

1. Remove virtual—because the ownership is tightly coupled with other implementation details. This is a "breaking change", whatever, file this under "Will say's it's okay".
2. Use virtual dispatch (s/ERC721\.//g)—this might be inconsistent and confusing

---

P.S., in the past, other ERC721 functions were called without virtual dispatch and it created a security problem risk. This affects past versions of OZC. I'm not sure if those versions are still supported (see #2759). I have seen an implementation in the wild that is vulnerable as a result of this.

[frangio]

Please see #2154 for extensive prior discussion, in particular #2154 (comment):

Note that 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.

The reason it's built this way is that it's otherwise impossible for us to assure internal consistency of our contract if ERC721.ownerOf can be anything other than the owner we have in storage. The burden of guaranteeing internal consistency is moved to the developer who wishes to override in these niche cases.

---

in the past, other ERC721 functions were called without virtual dispatch and it created a security problem risk. This affects past versions of OZC

Can you please give detail about this?

[fulldecent]

I agree that providing this power is useful. And at the same time, of course, we are making a risk of somebody doing something wrong.

Maybe a nice way to handle this is to provide "subclassing" notes everywhere we allow subclassing.

A good example of developer subclassing notes for an object-oriented language is the documentation for Swift. Please search the web with "Subclassing Notes" site:https://developer.apple.com/documentation to see how this looks.

---

Of course. I sent detail to you by email. I can create a new issue for that when needed.

[frangio]

Closing this issue for reasons explained in my previous comment: "The burden of guaranteeing internal consistency [when ownerOf is overriden] is moved to the developer who wishes to override in these niche cases."

The specific case of isApprovedForAll in Contracts 3.x is tracked in #2792.

[matthiasgeihs]

@frangio

I came upon this issue when dealing with the following: I'd like to have an ERC721 contract where initially a fixed number (100+) of tokens are assigned to the contract owner. To do this in a gas-efficient way, the idea is to override ownerOf such that it returns the contract owner in case the token is not in _owners.

Now this does not work because the OZC ERC721 implementation uses ERC721.ownerOf internally, and not ownerOf (the overridden function).

What's the purpose of making ownerOf overridable when, at the same time, the core contract is forced to use the original implementation?

[fulldecent]

@matthiasgeihs Thank you for checking in. There are some implementations you can study which are doing that approach:

• Su Squares implements where the first 10,000 NFT are transferred to the contract owner at time of deployment.
• ERC721a includes a batch transfer function you can use at time of contract deployment.

You can use both of those ideas while still employing OpenZeppelin Contracts, it's just nice to see how others have done it.

Specifically regarding OZC and your proposed implementation, please ensure you are testing that YourContract.ownerOf (the external call) will throw for tokens that have not been minted. (Unless you are minting exactly 2^256 tokens at deployment.) This is a requirement of ERC-721.

If you would like to review your work together live, you can join my weekly call, please see https://phor.net/#hour, follow, and add yourself into the schedule.

Based on what we learn, if we find anything that affects OZC and specifically the virtual part of ownerOf subclassing, then we can report back our findings here.

[frangio]

@matthiasgeihs For efficient batch minting please see our new ERC721Consecutive contract. It is included in 4.8, to be released next week, but already available in a release candidate. It will allow you to mint 100+ tokens in the constructor.

[fulldecent]

Looks good. Thank you @frangio for restricting it to the constructor not just documenting that caveat!

[matthiasgeihs]

Thanks @frangio for pointing out ERC721Consecutive. That was exactly what I needed. (Looks like you have included the necessary modification to ERC721 in #3311.)