Possible reentrancy issues in _mint() function?

[jpegdigital]

Hi, I'm looking to implement ERC721A in a project so have been combing through the code. Still new to reentrancy patterns so wanted to ask the question first before submitting a PR.

When calling _safeMint(), control is handed off to a receiving contract before totalSupply is updated. Could a malicious contract create an exploit or wreck havoc by re-entering the mint function from here? In the re-entrant call, startTokenId would be identical to the original call, but balances changed and events have been emitted so it's not entirely obvious to me what kind of damage could occur.

Code in question:

uint256 startTokenId = _currentIndex;
...
unchecked {
    _addressData[to].balance += uint128(quantity);
    _addressData[to].numberMinted += uint128(quantity);

    _ownerships[startTokenId].addr = to;
    _ownerships[startTokenId].startTimestamp = uint64(block.timestamp);

    uint256 updatedIndex = startTokenId;

    for (uint256 i; i < quantity; i++) {
        emit Transfer(address(0), to, updatedIndex);
        if (safe && !_checkOnERC721Received(address(0), to, updatedIndex, _data)) {
            revert TransferToNonERC721ReceiverImplementer();
        }

        updatedIndex++;
    }

    _currentIndex = updatedIndex;
}

[jpegdigital]

In terms of solutions/alternatives, I've been thinking of three options for my project:

1. Remove _safeMint() all together as handing off control to another contract is risky in this 2022 minting environment.
2. Call _checkOnERC721Received() only once for the final token (ensures state is fully updated). Or maybe creating a new _checkOnERC721ReceivedBatch standard.
3. Ignore and advise implementing contracts to use reentrancy blockers.

[Vectorized]

Good points. Looks like balance and the number minted can become inconsistent.

Perhaps a simple check like this could help:

uint256 startTokenId = _currentIndex;
...
for (uint256 i; i < quantity; i++) {
       ...
}
// Prevents reentrancy
if (safe && _currentIndex != startTokenId) revert();
_currentIndex = updatedIndex;

The compiler could optimize it away if safe is always false, but otherwise the gas overhead should be minimal.

[ahbanavi]

I think the implementing contracts should handle reentrancy, as many contracts (like Azuki) block minting from other contracts in the first place and therefore there's no reentrancy problem (correct me if I'm wrong).
So a warning in _safeMint specs or in the docs I think is enough, and we should keep ERC721A implementation close to the original.

[cygaar]

Agree with @ahbanavi here, implementing contracts should handle reentrancy. We want to stick as close as possible to the original ERC721 implementation

[jpegdigital]

What do y'all think about calling _checkOnERC721Received() only once then at the end of the batch? It could be called with start tokenId (or end) and pass the quantity via the _data parameter?

Thoughts on why this would be appropriate:

1. Ensures contract state is consistent before handing off control.
2. EIP-721 states The ERC721 smart contract calls this function on the recipient after a transfer which could be interpreted to be called just once during a batch.
3. Main purpose of safe functions is to ensure receiving contracts can handle NFTs and won't be permanently lost. In most cases, if a contract can receive one token, it can receive all.

[cygaar]

Hmm that's a good point, however I think we want to err on the safe side and not make any assumptions on this: "In most cases, if a contract can receive one token, it can receive all". If the community feels strongly about only checking onERC721Received at the end of the batch we can revisit this. Feel free to open a separate issue for that so we can close this one out @jpegdigital

[kyokosdream]

We followed @cygaar suggestion and added OZ re-entrancy guard to applicable functions in our contract which implements ERC721A. I agree that this should not be included in ERC721A. ERC721A is designed to add a batch minting API to ERC721 and remove unnecessary gas consumption caused by ERC721Enumerable.

EDIT: I've learned more about the difference between _safeMint and _mint and am swapping our calls from the former to the latter and will wait for the #131 pull to be merged.

[Vectorized]

Fixed in #131 . Can be closed.