code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

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

we use IERC721(nftContract).balanceOf(msg.sender) to check if the user bypass the limitPerAccount restriction,

https://stackoverflow.com/questions/72604383/how-to-create-a-limit-in-minting-using-balanceof-over-contract but user can buy nft, transfer nft out to another account, and buy more nft to bypass the restriction.

If you use the wallet balance you may not be able to correctly check the amount of mints made by a wallet, as the balance only tells you how many tokens the wallet currently has.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

For example, we set saleConfig.limitPerAccount to 2,

first, user buy two nft, second, user transfer two nft to his another wallet. third, user can buy two more nft.

Tools Used

Recommended Mitigation Steps

I think you should create a mapping to store the amount of mints of each wallet. In your smart contract create a public mapping of mint amount of each wallet:

mapping(address => uint) public walletMints;

And then when the user mints you need add this amount to the address:

walletMints[msg.sender] = walletMints[msg.sender] + quantity_

When you create a public mapping the smart contract automatically creates a getter of this mapping that you can use in your javascript code and check the amount of mints made by a wallet.

ghost commented 2 years ago

My issue here https://github.com/code-423n4/2022-08-foundation-findings/issues/59 with a proof of concept test in foundry but I disagree with severity as there are no loss of funds.

There are also too many dups for this issue so I won't list all of them down. Please check the de-dupe tool shared by kartoonjoy.

HardlyDifficult commented 2 years ago

Dupe of https://github.com/code-423n4/2022-08-foundation-findings/issues/59

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