Foundation Drop contest - yixxas's results

Lines of code

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

Vulnerability details

Impact

Calling balanceOf(msg.sender) to check if an address has minted more than allowed is bad since buyer can always transfer away the NFT after minting, and hence mint more than allowed, making it unfair for other parties. This is important when we want to only allow certain addresses to mint, and we want to limit this address from minting more than allowed.

Proof of Concept

L183 checks for (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount), which can easily be bypassed by transferring NFT to another address before calling mintFromFixedPriceSale.

NFTDropMarketFixedPriceSale.sol#L183-L189

function mintFromFixedPriceSale(
    address nftContract,
    uint16 count,
    address payable buyReferrer
  ) external payable returns (uint256 firstTokenId) {
    ...
    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);
    }
...
}

Tools Used

Recommended Mitigation Steps

Consider using mapping(address => uint) to track number of NFTs an account has minted instead of using balanceOf.