Review of ERC721Consecutive in v4.8.0-rc.0

[tinchoabbate]

Hey folks!
These days I'm reviewing the v4.8.0-rc.0 release candidate. I'll drop in this issue my concerns and comments and questions about the ERC721Consecutive contract and the related functions in the ERC721 contract.

I spent only a couple of hours on these, so take these notes just as a contribution from an independent researcher, not as a comprehensive audit 😄 And needless to say, feel free to disregard any of the suggestions here as you find best.

I'll number the notes so that it's easier to reference in comments or other issues or PRs. They're listed in no particular order of priority.

1. The EIP 2309 is only concerned about the event, and not the creation/transfer of batches of tokens. However, the docstrings for ERC721Consecutive state: Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in https://eips.ethereum.org/EIPS/eip-2309. Which is questionable, because the OZ implementation is adding opinionated features and limitations not indicated by the standard. Two examples:

   • In ERC721Consecutive, the token IDs of minted tokens in batches are limited to a uint96 type, which is not indicated by this EIP. If I only consider how the event is defined, where it uses a uint256 for the token IDs, I could argue that this implementation deviates from the standard. In any case, my main concern here is that (as far as I have seen) this limitation is not explicitly documented nor tested.
   • In ERC721Consecutive, token IDs of minted tokens are assumed to be consecutive and starting at 0. The former is stated by the EIP (so that's fine), but the latter is not. I'm definitely not familiar with real use-cases for ERC721 implementations, so I wouldn't be appropriate to say what's best here. Just saying that it may be worth thinking whether imposing this on developers is a sensible choice or not.

2. The hardcoded 5000 maximum batch size could be moved to a contract-level constant. Even better, even if you may consider 5000 the default size, it could be interesting provide a way for it to be overridden by developers to another number that suits them best.

3. Defining _beforeConsecutiveTokenTransfer and _afterConsecutiveTokenTransfer in the ERC721 contract instead of in ERC721Consecutive seems out of place. I understand this is done because _balances is private, am I correct ? Doesn't this unnecessarily increase deployment costs of any ERC721 contract?

4. When minting a batch using ERC721Consecutive::_mintConsecutive, the update of balances (i.e., the write to _balances) happens within the ERC721::_beforeConsecutiveTokenTransfer hook. This is different to what happens when minting a single token. Because in ERC721::_mint the actual update of balances is done after the internal hook ERC721::_beforeTokenTransfer. This doesn't seem to be a problem on its own. Though I do wonder whether it'd be cleaner if ERC721Consecutive followed the behavior of ERC721, which is what developers are already used to.

5. The docstrings for _beforeConsecutiveTokenTransfer and _afterConsecutiveTokenTransfer might be misleading, because "consecutive token transfers" could be understood as two transfers of the same token in the same transaction. I'd try to be more specific here.

6. The ERC721Consecutive::_mintConsecutive function returns a uint96 type, but:
   - It's unclear why this is needed. Its counterpart ERC721::_mint doesn't return anything. I'd at least try to document the purpose of this returned value.
   - The returned value is never tested.
   - The returned value is called first, which does not seem self-explanatory to me.
   - The uint96 returned is always set to the first token ID to be minted, even if no tokens are minted when batchSize is zero. Could this be confusing for a caller contract ? Why not simply revert if batchSize is zero ?

7. The ERC721Consecutive::_mintConsecutive function can only mint tokens to a non-zero address. Therefore, it seems that in ERC721::_beforeConsecutiveTokenTransfer the from != address(0) and to != address(0) checks are not necessary. Perhaps those check are there for future extensions ? Or just as an example ? Not sure. Might be worth documenting why those checks are there.

8. The hook onERC721Received on the receiver of a batch minting operation is never called. The EIP says nothing about it, so it's definitely up to the implementation. Still I'd at least comment that you've decided not to include it, and people should ensure the receiver of a batch mint can handle the tokens.

9. According to this contract, batch minting can only happen during construction. In an upgradeable setting, this means that a reinitialization of the proxy would not allow minting tokens in batch. In principle I'd say that's fair, because the docs are already saying that minting can only happen during construction. Which is a whole different thing than reinitialization of a proxy. I just wonder whether the average developer is fully aware of the distinction. I'm mentioning this because it's unclear whether this is an intended behavior you thought about. In any case, seems worth testing and documenting.

10. There's an off-by-one error in the ERC721Consecutive::_afterTokenTransfer function. If I understand correctly, tokenId <= _totalConsecutiveSupply() should be tokenId < _totalConsecutiveSupply().

11. I found it interesting that in ERC721Consecutive::_afterTokenTransfer, the call to super._afterTokenTransfer happens after the extended logic. I checked other token extensions and the order is different. Example: in ERC20Votes::_afterTokenTransfer, the call to super._afterTokenTransfer happens before the extended logic. Honestly not sure what's best here, because in both cases _afterTokenTransfer is empty, so it doesn't seem too relevant at first. Do you follow any guidelines for these cases ?

12. There's an unused SafeCast.sol import.

13. Two tiny typos. Line 102: "token" should say "tokens". Line 112: "is" should say "in".

14. Have you considered making ERC721Consecutive::_totalConsecutiveSupply internal instead of private? It'd be easier for implementation contracts to access the token ID after a batch mint. I found myself having to manually change it to internal to run some custom local tests.

[Amxx]

Hello @tinchoabbate

You raise a lot of interesting points! I'm going to answer including extra description that you may not need, but that would possibly help other readers.

1. I think the two come together are they are about the difference between the ConsecutiveTransfer event and its use by the NFT community during batch minting. Indeed we are not providing a generic implementation of EIP-2309, but rather a limited feature set that leverage this ERC. We believe the features we implement to align with the needs of the community in terms of batch minting, as seen in many other contracts.

That being said, we are possibly missing the point. Maybe we should try go be beyond what already available and feature that no-one else provides (AFAIK). It would be a balance against the complexity of the code, and the resulting costs, but I'd be open to have the discussed in a dedicated issue !

Note that the uint96 limitation is indeed hidden, and result for data packing for gas optimizations. IMO, its not an issue because of point 2.

2. This is definitely not a contract limitation. The contract limitation is the use uint96 for storing the "anchors". However, we noticed that indexers such as OpenSea have strong limits, since they do store one entry per token. Our goal with this limit is to avoid devs shouting themselves in the foot, minting 100k tokens at once, and being unhappy about the tokens not showing up on opensea.

We could use an internal view virtual function that devs could override. We would have to be very clear about the consequences of changing that ...

3. It is because of the balances, and I agree its not nice. We haven't found a way around it. It will not affect the ERC721 instances that do not include this extensions, as non-used internal function are dropped and not included in the actual bytecode.

4. It actually is an issue we discussed. If someone overrides the hook and inject code after the super call, they would be executing in a context where balances are increassed but ownership is not set. Doing an external call here would be really bad. Again, we don't really have a fix for 4.x. Next breaking change we will possibly refactor some of that (removing the hooks is on the table)

5. Thanks, we should clarify that. Feel free to open a PR if you have ideas how to word it.

6. The return value is to optimise possible override of the function. Lets say you want to do X when batches of tokens are minted, you can do

function _mintConsecutive(address to, uint96 batchSize) internal virtual returns (uint96) {
    uint96 first = super._mintConsecutive(to, batchSize);
    // DO YOU STUFF
    return first;
}

If we did not return the value, you'd have a hard time knowing which tokens were minted. It would require an storage read operation that we can avoid. Note that since this is internal, its only designed for dev building features on top of it, not external users querying it.

As for the batch mint of size 0, they are no-ops, and our policy is to accept no-ops to avoid disturbing workflows (just like we accept transferFrom of 0 tokens when approval is 0.

[Amxx]

7. it's for sanity/future extensions. Someone else might create a module that calls there hooks somehow ... we want to help making that secure.

8. Interesting point! IMO calling is off the table as it would make batch minting terribly expensive. Also not that its a mint, and not a safeMint. But sure, we should document that.

9. The intended behavior was to follow ERC-721 to the letter. ERC-721 says that a contract MUST emit the transfer event for all transfers (including minting and burning) unless they happen during construction. Allowing batch minting during an upgradeable re initialization would not follow ERC-721, so we don't allow it. Should that be better documented?

[Amxx]

10. This is right, we could replace <= with <. That is not a security issue though. Changing it would only be for gas optimizations.

[Amxx]

11. We do the operation before the super call, because we want the super call to happen in a context were ownership of the token is correctly set. The super call might not be empty. Someone could write

MyContract is ERC721, MyCustomERC721, ERC721Consecutive {

And super._afterTokenTransfer in ERC721Consecutive would include the logic from MyCustomERC721.

Inheritance is a pain for us, because we don't control the linearization order of modules in the users contracts... still we try to do our best

12. Thx for finding that!

13. Thx for finding that!

14. This is an interresing idea. When we make things internal, we also try to make them virtual. That is something we should discuss/consider.

[frangio]

2. The recommendation for the upper limit of 5000 came from Support EIP-2309: ERC-721 Consecutive Transfer Extension #2355 (comment). I think I feel ok with making that customizable through an overrideable function. Even if OpenSea caps it at 5000, it makes no sense for us to force that limit on everyone. Someone may decide to ignore OpenSea and that's fine as far as I'm concerned. As long as we document the potential consequences it would be ok.

5. We could rename the hooks from "consecutive" to "batch".

14. If we made this function internal, I would not want to make it virtual (so I would just avoid making it internal). To obtain the id of the tokens that were just minted, IMO you should use the return value of _mintConsecutive! This is the answer to your point 6 I believe.

[tinchoabbate]

wow, great answers 🙌 I'll try to summarize the most relevant points here. Everything not included I acknowledged and agreed with you.

2. To different extents, it seems we all agree on making the value customizable and documenting the potential consequences of changing it. This point is already listed in #3712.

3. I wasn't sure whether non-used internal function were dropped or not. That's good to know!

5. I like @frangio's idea to rename hooks to "batch". This point is already listed in #3712.

6. Agreed. Still, note the returned value is not tested nor documented. Seems worth including in #3712.

8. Haha agreed, I wasn't expecting to execute a call for each minted token. Perhaps there's room to do something clever with the additional data field in the onERC721Received hook, to only call once and signal that a batch is being minted. But that's definitely out of scope here. Just mentioning that the minting is not calling the hook would be fair enough. I see this is already listed in #3712.

9. I re-read the docstrings and tests for the ERC721Consecutive contract with fresh eyes. They clearly state that minting only happens in construction. I think it's enough.

10. Agreed that it's not a security issue. Still worth changing. I see this is already done in #3712.

14. Agreed. Which makes a stronger argument for testing and documenting the return value of _mintConsecutive mentioned in point 6.