code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Bot may attack first come first serve airdrop-like drop (Can also attack paid one if it is hyped). Majority of the NFT drop may be owned by bots. Bot can create unlimited wallet and call mintFromFixedPriceSale from all wallet simultaneously in the same block that creator call createFixedPriceSale. This cause 100% NFT distribute to the bot owner wallet account which is not a good idea.

This happened too many times in NFT hyped time in 2022 Q1. It also happened with the very first version of Quixotic launchpad.

Proof of Concept

  function mintFromFixedPriceSale(
    address nftContract,
    uint16 count,
    address payable buyReferrer
  ) external payable returns (uint256 firstTokenId) {
    // Validate input params.
    if (count == 0) {
      revert NFTDropMarketFixedPriceSale_Must_Buy_At_Least_One_Token();
    }

    FixedPriceSaleConfig memory saleConfig = nftContractToFixedPriceSaleConfig[nftContract];

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

    // Calculate the total cost, considering the `count` requested.
    uint256 mintCost;
    unchecked {
      // Can not overflow as 2^80 * 2^16 == 2^96 max which fits in 256 bits.
      mintCost = uint256(saleConfig.price) * count;
    }

    // The sale price is immutable so the buyer is aware of how much they will be paying when their tx is broadcasted.
    if (msg.value > mintCost) {
      // Since price is known ahead of time, if too much ETH is sent then something went wrong.
      revert NFTDropMarketFixedPriceSale_Too_Much_Value_Provided(mintCost);
    }
    // Withdraw from the user's available FETH balance if insufficient msg.value was included.
    _tryUseFETHBalance(mintCost, false);

    // Mint the NFTs.
    firstTokenId = INFTDropCollectionMint(nftContract).mintCountTo(count, msg.sender);

    // Distribute revenue from this sale.
    (uint256 totalFees, uint256 creatorRev, ) = _distributeFunds(
      nftContract,
      firstTokenId,
      saleConfig.seller,
      mintCost,
      buyReferrer
    );

    emit MintFromFixedPriceDrop(nftContract, msg.sender, firstTokenId, count, totalFees, creatorRev);
  }

To mint unlimited NFT, bot simply create a contract that execute mintFromFixedPriceSale then transfer all NFT to main wallet multiple times.

Or create multiple contracts that execute mintFromFixedPriceSale simultaneously then transfer all NFT to main wallet in the same block createFixedPriceSale is called.

Or create multiple EOA wallet and execute mintFromFixedPriceSale simultaneously in the same block createFixedPriceSale is called (Or consequence blocks).

This is economically feasible if the NFT is hyped such that there is somebody willing to buy it in the secondary market. It cause loss in the trust of that NFT project and also cause some funding loss if that project uses market maker as the bot will immediately dump the NFT to the market maker.

Tools Used

Manual review

Recommended Mitigation Steps

Only allow EOA msg.sender (require msg.sender == tx.origin)
Use signature verification from the backend if price = 0 and add some captcha on the minting page to filter bots before generating signature on the backend.

HardlyDifficult commented 2 years ago

ACK.

We believe this issue is Low risk. Botting is an inherit risk to smart contract protocols.

The thinking for this is similar to some of the remarks I made on the limitPerAccount report - ultimately the proposed solutions and what has been done in the wild can still be gamed. It's just a matter of adding some friction, but once a workaround is known it can easily be reapplied to all collections created by our factory. This is a fight we cannot win, so we must accept that botting may happen.

The 2 recommendations here seem to match what the other warden was pointing to with the link to CoolPets.

Only allow EOA msg.sender (require msg.sender == tx.origin)

This type of approach can be used to prevent contracts from interacting with the system but it comes at a cost of blocking legit users with a smart contract wallet such as Gnosis Safe.

And it does not stop bots, just smart contracts... it's easy enough for someone to spin up a script which spams transactions in order to pass the tx.origin check.

Use signature verification from the backend if price = 0 and add some captcha on the minting page to filter bots before generating signature on the backend.

This friction would be a significant degradation of the user experience for legit users. A signature is an extra approval required, which is not too bad but can be tedious especially for smart contract users. A captcha is annoying, nobody likes those.

A simple script can sign transactions, so this is still easy to bot. It's another technique that effectively just blocks smart contracts but not bot attacks.

The captcha suggestion is a compelling one -- and this would go a long ways towards preventing bots. Bots are made to break captchas as well, but the neat thing about this approach is that our frontend can change the captcha difficulty and continually upgrade to the latest.

The challenge we have with a captcha solution is compatibility. We are building a platform that we want others to use as well. So the backend signer cannot be limited to accounts that we control. Maybe some ACL could be used where the creator can opt into which backends are allowed to approve mints. This who setup significantly complicates 3rd party integrations - however it's a suggestion we may revisit in the future if we find that bots are a problem for our market.

HickupHH3 commented 2 years ago

This is a fight we cannot win, so we must accept that botting may happen.

Agree with sponsor. Attempts to preventing bot / contracts that attempt to game the sale / airdrop is a complex problem to solve. Like a game of whack-a-mole, it would only be a matter of time before someone finds a way to bypass the current defense mechanisms.

Downgrading to low-risk QA.