Foundation Drop contest - Treasure-Seeker's results

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183-L189

Vulnerability details

Impact

In the NFTDropMarketFixedPriceSale contract, there is a configuration called limitPerAccount which is defined as "The limit of tokens an account can purchase".

The current code checks the balance of msg.sender in the mintFromFixedPriceSale function. This check can be easily bypassed by minting the amount, then transferring to another wallet and then re-minting.

Proof of Concept

if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {
  if (saleConfig.limitPerAccount == 0) {
    // Provide a more targeted error if the collection has not been listed.
    revert NFTDropMarketFixedPriceSale_Must_Have_Sale_In_Progress();
  }
  revert NFTDropMarketFixedPriceSale_Cannot_Buy_More_Than_Limit(saleConfig.limitPerAccount);
}

This code incorrectly checks the balance of msg.sender instead of internally accounting for how many the wallet has minted.

Easy POC to check is - make the limit 10, then mint 10, transfer 10, mint 10 again.

Tools Used

VS Code

Recommended Mitigation Steps

The NFT industry practice is to save the number of mints for msg.sender in storage. As an example, you can check how Azuki internally accounts for numberMinted using a storage variable.

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L182 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L271

Vulnerability details

Impact

The NFTCollection and NFTDropCollection contracts use the OZ _mint instead of _safeMint.

So, if a contract cannot hold ERC721 tokens and attempts to mint, it does not revert. Instead, the ETH is lost and so are the NFT tokens.

Proof of Concept

Easy POC is as follows:

1. Create a contract that does not implement the onERC721Received function
2. Attempt to mint via the contract
3. NFTs are minted, ETH is transferred to the contract and the transaction does not revert

Tools Used

VS Code

Recommended Mitigation Steps

The NFT industry practice is to use _safeMint instead of _mint. Blue chip NFTs such as BAYC, Azuki, Doodles, etc. use _safeMint.

Note that when we implement _safeMint, we will also need to add ReentrancyGuard to the mint function to avoid reentrancy attacks to mint over the max supply.

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170-L219

Vulnerability details

Impact

Botting NFT mints is very common for hyped projects. People create bot contracts to immediately mint as much as they can and then sell in the secondary market for a premium.

Genuine buyers miss out minting the project and there is a lot of negative publicity after that happens.

For some context on botted mints, you can check out how the Adidas mint and the Sevens mint were botted and the aftermath that happened.

Proof of Concept

There is no bot protection for mints currently in the Foundation contracts.

There is a limitPerAccount configuration but that is meant to prevent humans from minting more than needed. Even though this check can currently be bypassed (by transferring to another wallet and I've created a separate issue for that), even if it is fixed - it will not stop bot attacks (refer the Adidas' bot attack linked above - people can create subcontracts to mint, transfer to a single owner after minting and self destruct all sub contracts).

Tools Used

VS Code

Recommended Mitigation Steps

One effective way to prevent bot attacks is to use ECDSA signatures. Cool Pets implemented the protection very well and that is a contract you may refer to.