code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L171-L187

Vulnerability details

Add limit on NFT Mining Count

Context:

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L171-L187

Description:

Mint Function is the most important architectural part of an NFT project. The mint function has no quantity limit. For example, in a 10000 NFT project, 10000 NFT Mints can be triggered at the same time.

However, due to the block gas limit on the Ethereum network, a maximum of ~600 mints can be performed. A normal user may not know this architectural limit and may want to mint, this may cause the user's transaction to revert - high gas consumption.

Architectures that can cause such high gas expenditures should be avoided in the mint function

Classified as medium due to its impact on the user , at some point the transaction will be too big maybe bigger than a block

As of today 2022-August in mainnet the block gas limit is 30M. If minting a single token consumes 50K gas then you can at most create 600 tokens.

 30000000 / 50000 = 600

Recommended Mitigation Steps

add to this require in mintCountTo function

function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
require(count < 601 , "You can Mint up to 600 NFTs at a time");
}

HardlyDifficult commented 2 years ago

We believe this is invalid. The number of NFTs being minted is being provided as an input to this function call. estimateGas is used by frontends and wallets in order to determine what to use as the gas limit for the call -- with this they will see if the number requested exceeds what is supported by the blockchain. If they attempt to broadcast a tx with a limit greater than the block size, it simply will not be mined.

Using a hardcoded value as suggested here seems unnecessary. Block sizes can change over time, different EVM networks will have different limits, and other factors could influence the actual max that could be supported -- e.g. as many warden's suggested in this contest we are switching to use safeMint, and if the receiver is a contract with any logic included in that callback it could significantly reduce the max a single block can handle.

HickupHH3 commented 2 years ago

Agree with sponsor's reasoning. Wallets (like Metamask) will have an error message if the tx will revert. If the user decides to proceed with the tx anyway, then the problem doesn't lie with the tech.

code-423n4 / 2022-08-foundation-findings