Foundation Drop contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

It was observed that any user can bypass saleConfig.limitPerAccount and mint any number of NFT without caring about limitPerAccount. This is happening due to unrestricted transferFrom.

Proof of Concept

1. User A has minted say 10 NFT and saleConfig.limitPerAccount is also 10

2. Now User A calls mintFromFixedPriceSale and try to mint 10 more NFT

3. The below condition fails since user has already reached threshold of saleConfig.limitPerAccount

 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);
    }

4. User A simply use transferFrom function at NFTDropCollection.sol to transfer 10 NFT to his another owned address (Contract implements ERC721Upgradeable which allows transferFrom)

5. Now User A new balance becomes 0 and he can simply mint NFT using Step 2

6. Once User A has minted 10 NFT, he again uses transferFrom to transfer back his NFT from his another owned address to his own adddress. This means User A has now 20 NFT even though limit per account was set as 10

7. This way using 2 address any user can simply bypass saleConfig.limitPerAccount

Recommended Mitigation Steps

Override the transferFrom/safeTransferFrom function (or beforetokenTransfer) and check if post transfer balanceOf(msg.sender)+amount <=saleConfig.limitPerAccount, if not revert

Token Id existence not checked

Contract: https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L298

Issue: In getTokenCreatorPaymentAddress function, It is not checked whether tokenId actually exists. Even if it does not exist creatorPaymentAddress will be returned as owner. This seem to be used by CollectionRoyalties.sol to calculate Royalties, fees. CollectionRoyalties seem to be exposing API which means that this will give incorrect data.

Recommendation: Revise the function to include below checks:

require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

Unused function

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

Issue: _burn: This function is marked internal and does not seem to be used.

Recommendation: Remove the function if not required