Foundation Drop contest - ladboy233's results

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.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

When minting,

we go through

  function mintFromFixedPriceSale(
    address nftContract,
    uint16 count,
    address payable buyReferrer
  ) external payable returns (uint256 firstTokenId) {

buyRefererer can be set to arbitrary address to the referral fee because when the referral fee is calcuulated.

    if (buyReferrer != address(0) && buyReferrer != msg.sender && buyReferrer != seller && buyReferrer != creator) {
      unchecked {
        buyReferrerFee = totalFees / BUY_REFERRER_PROTOCOL_FEE_DENOMINATOR;

        // buyReferrerFee is always <= totalFees
        totalFees -= buyReferrerFee;
      }
    }

Proof of Concept

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

A user Bob wants to mint nft, he set Alice to receive the referral fee, but alice is bob's wallet address.

Tools Used

Recommended Mitigation Steps

We can use another map to keep track of the referral.

 mapping(address => address) referralMap;

and the function

  function refer(address referTo) {
    referralMap[referTo] = msg.sender;
  }

and when a user is minting,

we can check if the referral address is correct.

  function mintFromFixedPriceSale(
    address nftContract,
    uint16 count,
    address payable buyReferrer
  ) external payable returns (uint256 firstTokenId) {

  require(referralMap[msg.sender] == buyReferral, 'invalid referral')

  // other logic

}

Function can be marked as external

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

getTokenCreatorPaymentAddress can be marked as external

cache the array length instead of accessing .lengh in every loop

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L126

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L484

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L503

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

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

can cache the .length,

for example,

uint256 length = array.length;

for(uint256 i = 0; i < length; ++i) {
 logic
}