Foundation Drop contest - Chom's results

Lines of code

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

Vulnerability details

Impact

limitPerAccount can be bypassed. User can mint and transfer all NFT to intermediate wallet and mint again to bypass the limitPerAccount.

Proof of Concept

Assume limitPerAccount = 5

First user mint 5 NFT.

Then user transfer all 5 NFT to an intermediate address.

Currently, user is holding 0 NFT. IERC721(nftContract).balanceOf(user) == 0

Then mint 5 NFT again

    // Confirm that the buyer will not exceed the limit specified after minting.
    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);
    }

In mintFromFixedPriceSale function IERC721(nftContract).balanceOf(msg.sender) + count == 0 + 5 <= saleConfig.limitPerAccount thus not revert and 5 NFT will be minted to the wallet again.

Now, this wallet has minted 10 NFT which is over saleConfig.limitPerAccount = 5

    uint256 currentBalance = IERC721(nftContract).balanceOf(user);
    if (currentBalance >= limitPerAccount) {
      // User has exhausted their limit.
      return 0;
    }

    uint256 availableToMint = limitPerAccount - currentBalance;
    if (availableToMint > numberOfTokensAvailableToMint) {
      // User has more tokens available than the collection has available.
      return numberOfTokensAvailableToMint;
    }

    return availableToMint;

In getAvailableCountFromFixedPriceSale return availableToMint = limitPerAccount - IERC721(nftContract).balanceOf(user) = 5 - 0 = 5 means this wallet can mint another 5 NFT although this wallet has already minted 5 NFT.

User can repeat this process to mint unlimited NFT.

Tools Used

Manual review

Recommended Mitigation Steps

Create a mapping(address => uint256) public mintedCount; to keep track of NFT minting count of each wallet.

Possibly wrong comment

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

  /**
   * @notice Allows the owner to set a max tokenID.
   * This provides a guarantee to collectors about the limit of this collection contract.
   * @dev Once this value has been set, it may be decreased but can never be increased.
   * This max may be less than the final `totalSupply` if 1 or more tokens were burned.
   * @param _maxTokenId The max tokenId to set, all NFTs must have a tokenId less than or equal to this value.
   */
  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
    _updateMaxTokenId(_maxTokenId);
  }

If 1 or more tokens were burned, totalSupply will be decreased.

It should be "This max may be more than the final totalSupply if 1 or more tokens were burned."

updateMaxTokenId doesn't emit event

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

  /**
   * @notice Allows the owner to set a max tokenID.
   * This provides a guarantee to collectors about the limit of this collection contract.
   * @dev Once this value has been set, it may be decreased but can never be increased.
   * This max may be less than the final `totalSupply` if 1 or more tokens were burned.
   * @param _maxTokenId The max tokenId to set, all NFTs must have a tokenId less than or equal to this value.
   */
  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
    _updateMaxTokenId(_maxTokenId);
  }

updateMaxTokenId is an important event, it should emit an event

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

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

require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

Can be replaced with

// declare error before contract declaration
error UseRevealInstead();

if (_postRevealBaseURIHash == bytes32(0)) revert UseRevealInstead();