2

I am trying to write a ERC-721 NFT minting contract and I am noticing that a lot of users are guarding against reentrancy attack. I was wondering why(and if) is this needed. An example of a contracts that will be similar to what I am writing: https://rinkeby.etherscan.io/address/0xEF6aB0b5CD245dB057eA6eE2beb858eC81f6145d#code https://etherscan.io/address/0xf21d1b31b15282592ff0e48f7b474b57ae418361#code#F1#L1 From what I can see you might be able to mint more than what the mint limit is for a single address, but that is the only thing that can be targeted. Should I even include a reentrancy guard in these situations and if so why?

1
  • Please find better starting points than those contrats Commented Jan 3, 2022 at 5:38

3 Answers 3

7

TL;DR: _safeMint is not that safe. So ensure your mint process is safe.

Take a look at the mint method here contract code,

require(_mintAmount <= maxMint, "Cant mint more then maxmint" );

for (uint256 i = 1; i <= _mintAmount; i++) {
    _safeMint(msg.sender, supply + i);
}

Here the contract is calling _safeMint in a loop for a max of _mintAmount which is required for an upper bound of maxMint. The _safeMint implementation is as follows,

    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, _data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );

see the _checkOnERC721Received, which is as follows,

if (to.isContract()) {
    try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) {
        return retval == IERC721Receiver.onERC721Received.selector;
    } catch (bytes memory reason) {
        if (reason.length == 0) {
            revert("ERC721: transfer to non ERC721Receiver implementer");
        } else {
            assembly {
                revert(add(32, reason), mload(reason))
            }
        }
    }
}

if the mint was initiated by a contract, then the contract is checked for its ability to receive ERC721 tokens. Without reentrancy guard, onERC721Received will allow an attacker controlled contract to call the mint again, which may not be desirable to some parties, like allowing minting more than allowed. This may become pretty annoying when things are not implemented correctly. For example, take a look at this code,

function unpack(uint256 _tokenId) public {
    require(ownerOf(_tokenId) == _msgSender());

    // Insert custom logic for configuring the item here.
    for (uint256 i = 0; i < NUM_CREATURES_PER_BOX; i++) {
        // Mint the ERC721 item(s).
        FactoryERC721 factory = FactoryERC721(factoryAddress);
        factory.mint(OPTION_ID, _msgSender());
    }

    // Burn the presale item.
    _burn(_tokenId);
}

if you call unpack before the burn exploiting reentrancy, you may (I haven't tested it, correct me if I'm wrong) get more items per loot box.

So, look before you leap.

1

I don't see any benefit in using the nonReentrant modifier here. it would just increase the gas cost for calling mint() by ~3.5k gas.

using nonRentrant is for protection against attack scenarios - mainly when a functions calls some malicious external contract that takes over the control flow, before the first invocation of the function is finished.

0

I think a more efficient way to handle attacks by other contracts it to simply use a modifier like this:

modifier callerIsUser() {
    require(tx.origin == msg.sender, "The caller is another contract");
    _;
  }

Then you can modify your mint function like:

function allowlistMint() external payable callerIsUser {
1
  • This will also block multisig users, and if it matter it will block any contract.
    – Ismael
    Commented Feb 9, 2022 at 7:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.