Re-entrancy attack explanation and possible fix for specific NFT token - hashmasks

Question

One popular recent NFT project - Hashmasks - Tweeted that some coder called 'samczsun' uncovered a possible re-entrancy attack in their mint function which would have allowed to mint more than the possible supply of 16,384 Hashmasks.They say , If timed perfectly correctly, the maximum number of extra masks mintable mathematically would have been 210 (20 + 19 + … 1)

Hashmasks token link on etherscan: HASMASKS

The mint function is as follows:

'''

function mintNFT(uint256 numberOfNfts) public payable {
    require(totalSupply() < MAX_NFT_SUPPLY, "Sale has already ended");
    require(numberOfNfts > 0, "numberOfNfts cannot be 0");
    require(numberOfNfts <= 20, "You may not buy more than 20 NFTs at once");
    require(totalSupply().add(numberOfNfts) <= MAX_NFT_SUPPLY, "Exceeds MAX_NFT_SUPPLY");
    require(getNFTPrice().mul(numberOfNfts) == msg.value, "Ether value sent is not correct");

    for (uint i = 0; i < numberOfNfts; i++) {
        uint mintIndex = totalSupply();
        if (block.timestamp < REVEAL_TIMESTAMP) {
            _mintedBeforeReveal[mintIndex] = true;
        }
        _safeMint(msg.sender, mintIndex);
    }

    /**
    * Source of randomness. Theoretical miner withhold manipulation possible but should be sufficient in a pragmatic sense
    */
    if (startingIndexBlock == 0 && (totalSupply() == MAX_NFT_SUPPLY || block.timestamp >= REVEAL_TIMESTAMP)) {
        startingIndexBlock = block.number;
    }
}



function _safeMint(address to, uint256 tokenId, bytes memory _data) internal virtual {
    _mint(to, tokenId);
    require(_checkOnERC721Received(address(0), to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");
}   

'''

I do not understand why the issue occurs. Could anyone enlighten me why this re-entrancy attack can happen and also how it could be fixed in the code. Thanks all.

Answer 1

Background: Reentrancy Attacks & Miner Withholding

Because Ethereum is deterministic, and future data can reliably be determined ahead of time, random number generation (RNG) is not possible without specific contracts.

This is intentional on the consensus layer of Ethereum to ensure there is no deviation from given input during mining. The vulnerability can be exploited however by a malicious actor and miner (or a miner who is minting a token) colluding to perform a reentrancy attack. The miner intentionally withholds the transaction from a block giving them time to call the same function over and over again before it is executed.

Problem Code

In this case, the for-loop sets the mintIndex (TokenID) equal to totalSupply() based on the numberOfNfts before the minting takes place.

This allows the transaction to be called over and over again if not committed as the MAX_NFT_SUPPLY does not get updated until it's placed on chain. But the numberOfNfts is updated on every iteration of the loop. So while the randomness generator allows for the vulnerability to be exploited, the for-loop is what the bad actor is using to create more NFTs than they're supposed to.

Why include problematic code in the first place?

Since a contract cannot simply call a random number, one workaround that doesn't require including a contract(s) just for RNG, making the contract less gas efficient is to use the previous block hash as a source of randomness. Let's take a look at the code dealing with this workaround:

if (startingIndexBlock == 0
  && (totalSupply() == MAX_NFT_SUPPLY
    || block.timestamp >= REVEAL_TIMESTAMP))
    {
        startingIndexBlock = block.number;
    }

NOTE: This type of vulnerability is only worth doing if the resulting tokens being minted is greater than the mining reward for the block. Since it is completely unknown what the value of these randomly generated NFTs are, this is likely why the contract creator assumed it would be "sufficient in a pragmatic sense."

How is this exploited?

Take note of a few things in the above code to generate randomness. The totalSupply() is instantiated in the previous for-loop, so until this transaction is finalized, the totalSupply() isn't stored on-chain. Also, the startingIndexBlock is the current block height when the function was initially called.

Since miners have access to all of the block information, which is what is used for randomness generation, an unscrupulous miner withholding committing from the blockchain, they can repeatedly call the mint function, generating more than the maximum numberOfNfts which is supposed to be 20.

Instead they can create up to 210 extra NFT's per address minting if each address has never minted before and each one mints 20 NFTs, as each iteration of the for-loop reduces the max number for that account by 1.

How to prevent exploitation?

One workflow that ensures a bad actor and miner cannot collude:

• Generate a random number using the block height, hash it, and commit it on-chain.
• Submit the random number on a future block.
• Hash the random number (which is only known to the person minting) using the block hash on the commit block (which is only known to the miner).

The final hash is a good source of randomness as it can be assured it was not tampered with. However, if the resulting asset is worth more than the block reward, collusion throughout the entire process could still take place.

The most secure way to generate random numbers is to utilize an external smart contract library such as ChainLink, which as an oracle, can utilize off-chain data to produce a reliable RNG.

If you're using workarounds like this, at least make sure to use the _safeMint function after a single iteration, then check the wallet balance of the user to determine how many mints they have left. It also helps, if you're not hiding the image until release, to tie each NFT to it's IPFS hash and then remove the reference from memory once it has been created so bad actors using the public explorer cannot take advantage and in any way mint false versions of your assets before the REVEAL_TIMESTAMP.