Foundation Drop contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

Selling a NFT with NFTDropMarketFixedPriceSale.mintFromFixedPriceSale distributes the revenue from the sale to various recipients with the MarketFees._distributeFunds function.

Recipients:

• NFT creator(s)
• NFT seller
• Protocol
• Buy referrer (optional)

It is possible to have multiple NFT creators. Sale revenue will be distributed to each NFT creator address. Revenue distribution is done by calling SendValueWithFallbackWithdraw._sendValueWithFallbackWithdraw and providing an appropriate gas limit to prevent consuming too much gas. For the revenue distribution to the seller, protocol and the buy referrer, a gas limit of SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20_000 is used. However, for the creators, a limit of SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210_000 is used. This higher amount of gas is used if PercentSplitETH is used as a recipient.

A maximum of MAX_ROYALTY_RECIPIENTS = 5 NFT creator recipients are allowed.

For example, a once honest NFT collection and its 5 royalty creator recipients could turn "malicious" and could "steal" gas from NFT buyers on each NFT sale and therefore grief NFT sales. On each NFT sell, the 5 creator recipients (smart contracts) could consume the full amount of SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210_000 forwarded gas. Totalling 5 * 210_000 = 1_050_000 gas. With a gas price of e.g. 20 gwei, this equals to additional gas costs of 21_000_000 gwei = 0.028156 eth, with a ETH price of 2000, this would total to ~56.31 $ additional costs.

Proof of Concept

mixins/shared/MarketFees.sol#L130

/**
  * @notice Distributes funds to foundation, creator recipients, and NFT owner after a sale.
  */
function _distributeFunds(
  address nftContract,
  uint256 tokenId,
  address payable seller,
  uint256 price,
  address payable buyReferrer
)
  internal
  returns (
    uint256 totalFees,
    uint256 creatorRev,
    uint256 sellerRev
  )
{
  address payable[] memory creatorRecipients;
  uint256[] memory creatorShares;

  uint256 buyReferrerFee;
  (totalFees, creatorRecipients, creatorShares, sellerRev, buyReferrerFee) = _getFees(
    nftContract,
    tokenId,
    seller,
    price,
    buyReferrer
  );

  // Pay the creator(s)
  unchecked {
    for (uint256 i = 0; i < creatorRecipients.length; ++i) {
      _sendValueWithFallbackWithdraw(
        creatorRecipients[i],
        creatorShares[i],
        SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS // @audit-info A higher amount of gas is forwarded to creator recipients
      );
      // Sum the total creator rev from shares
      // creatorShares is in ETH so creatorRev will not overflow here.
      creatorRev += creatorShares[i];
    }
  }

  // Pay the seller
  _sendValueWithFallbackWithdraw(seller, sellerRev, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);

  // Pay the protocol fee
  _sendValueWithFallbackWithdraw(getFoundationTreasury(), totalFees, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);

  // Pay the buy referrer fee
  if (buyReferrerFee != 0) {
    _sendValueWithFallbackWithdraw(buyReferrer, buyReferrerFee, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);
    emit BuyReferralPaid(nftContract, tokenId, buyReferrer, buyReferrerFee, 0);
    unchecked {
      // Add the referrer fee back into the total fees so that all 3 return fields sum to the total price for events
      totalFees += buyReferrerFee;
    }
  }
}

Tools Used

Manual review

Recommended mitigation steps

Consider only providing a higher amount of gas (SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS) for the first creator recipient. For all following creator recipients, only forward the reduced amount of gas SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT.

QA Report

Table of Contents

• Low Risk
  • [L-01] selfdestruct will be a noop in the future
  • [L-02] NFT buyer and NFT mint receiver can be a contract with no onERC721Received method, freezing NFT

Low Risk

[L-01] selfdestruct will be a noop in the future

Description

selfdestruct is a native Solidity function used to delete contracts on the blockchain. When a contract executes a self-destruct operation, the remaining ether on the contract account will be sent to a specified target, and its storage and code are erased.

After EIP-4758, the SELFDESTRUCT op code will no longer be available. According to the EIP, "The only use that breaks is where a contract is re-created at the same address using CREATE2 (after a SELFDESTRUCT)".

As the protocol does not necessarily depend on re-deploying contracts to the same address (however, a user an still deploy NFT contracts to the same address via NFTCollectionFactory calls), this will not break the protocol. It will simply render the selfDestruct function useless.

Findings

mixins/collections/SequentialMintCollection.sol#L77

function _selfDestruct() internal {
  require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

  emit SelfDestruct(msg.sender);
  selfdestruct(payable(msg.sender));
}

Called by NFTCollection.sol#L230

/**
  * @notice Allows the collection creator to destroy this contract only if
  * no NFTs have been minted yet or the minted NFTs have been burned.
  * @dev Once destructed, a new collection could be deployed to this address (although that's discouraged).
  */
function selfDestruct() external onlyCreator {
  _selfDestruct();
}

and NFTDropCollection.sol#L210

/**
  * @notice Allows a collection admin to destroy this contract only if
  * no NFTs have been minted yet or the minted NFTs have been burned.
  * @dev Once destructed, a new collection could be deployed to this address (although that's discouraged).
  */
function selfDestruct() external onlyAdmin {
  _selfDestruct();
}

Recommended mitigation steps

Consider removing the SequentialMintCollection._selfDestruct function and the other caller functions.

[L-02] NFT buyer and NFT mint receiver can be a contract with no onERC721Received method, freezing NFT

Description

If the NFT drop collection buyer or the receiver of a NFT mint is a smart-contract that does not implement the onERC721Received method, in the current implementation, by using _mint(), a minted NFT can get stuck in a recipients contract.

Findings

NFTCollection.sol#L271

function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
    unchecked {
        // Number of tokens cannot overflow 256 bits.
        tokenId = ++latestTokenId;
        require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
        cidToMinted[tokenCID] = true;
        _tokenCIDs[tokenId] = tokenCID;
        _mint(msg.sender, tokenId);
        emit Minted(msg.sender, tokenId, tokenCID, tokenCID);
    }
}

NFTDropCollection.sol#L182

function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
    require(count != 0, "NFTDropCollection: `count` must be greater than 0");

    unchecked {
      // If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits
      firstTokenId = latestTokenId + 1;
    }
    latestTokenId = latestTokenId + count;
    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

    for (uint256 i = firstTokenId; i <= latestTokenId; ) {
      _mint(to, i);
      unchecked {
        ++i;
      }
    }
}

Recommended mitigation steps

Its is recommended to use _safeMint whenever possible.

_safeMint(to, tokenId);