Foundation Drop contest - shenwilly's results

Lines of code

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

Vulnerability details

Impact

Minters can bypass the limitPerAccount and mint as many token as they want during a drop sale.

Vulnerability Details

In mintFromFixedPriceSale, there is a check to ensure that an address can only mint as any token as determined in limitPerAccount: NFTDropMarketFixedPriceSale.sol#L182-L189

// Confirm that the buyer will not exceed the limit specified after minting.
if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {
  ...
  revert NFTDropMarketFixedPriceSale_Cannot_Buy_More_Than_Limit(saleConfig.limitPerAccount);
}

However, the contract only checks the current balance of the buyer. Therefore, a buyer can transfer their newly minted token to bypass the check, and mint another batch of tokens. This is especially bad in airdrop-style drops with 0 price, as a single malicious user can potentially mint out the whole collection.

Proof of Concept

• Alice calls createFixedPriceSale to start the sale for her NFTDropCollection with 1 as the limitPerAccount.
• Bob mints 1 token, and immediately transfers the token to another address.
• Bob can now mint again and repeat the previous step indefinitely until supply runs out.

Recommended Mitigation Steps

It is impossible to enforce a minting limit on a free-for-all sale without resorting to some sort of allowlists/snapshots. In the short term, we can make it more difficult to exploit by:

• Storing the mint amount of each address and check it instead of balanceOf, preventing users from reusing the same address to mint, and
• Adding a check to disallow smart contract minter. This would prevent a malicious user from minting a lot of tokens in one transaction via smart contract.

Overview

Findings

[L-01] Unnecessary referrer checks

MarketFees.sol#L522-L529

if (buyReferrer != address(0) && buyReferrer != msg.sender && buyReferrer != seller && buyReferrer != creator) {
  ...
}

When processing fees, there is a check to ensure that the inputted buyReferrer is not the buyer itself, nor it is the seller or the creator themselves.

However, a motivated user (buyer/seller/creator) can easily bypass this check by using a fresh address as the referrer. We recommend to remove this check to save gas, and enforce this constraint on the front-end level instead.

[L-02] Unnecessary gas limit when distributing funds to creatorRecipients

MarketFees.sol#L124-L136

// Pay the creator(s)
unchecked {
  for (uint256 i = 0; i < creatorRecipients.length; ++i) {
    _sendValueWithFallbackWithdraw(
      creatorRecipients[i],
      creatorShares[i],
      SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
    );
    // Sum the total creator rev from shares
    // creatorShares is in ETH so creatorRev will not overflow here.
    creatorRev += creatorShares[i];
  }
}

When distributing funds to creatorRecipients, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS is used as the gas limit to ensure that enough gas is sent to cover 5 recipients. However, as the transfers are being done individually in a for-loop, using SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS will inflate the amount of gas needed to be prepared by the msg.sender.

Replace SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS with SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT.

[L-03] Total royalty shares can be greater than BASIS_POINTS

MarketFees.sol#L484-L491

for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  if (creatorShares[i] > BASIS_POINTS) {
    // If the numbers are >100% we ignore the fee recipients and pay just the first instead
    totalShares = 0;
    break;
  }
  totalShares += creatorShares[i];
}

When calculating totalShares, there is a check to handle the case where at least one creator has a share of more than 100 percent: creatorShares[i] > BASIS_POINTS. However, it is still possible that the total shares of all creators will be greater than BASIS_POINTS.

Therefore, we recommend to just check for the final value of totalShares, so that:

• Every creator share is guaranteed to be less than BASIS_POINTS.
• The total shares will not be greater than BASIS_POINTS.
• It will reduce gas on happy paths as the check is only called once after the loop.

Update the code to:

for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  totalShares += creatorShares[i];
}
if (totalShares > BASIS_POINTS) {
    totalShares = 0;
}

[L-04] Inaccurate _getSellerOrOwnerOf NatSpec

MarketSharedCore.sol#L24-L36

/**
  * @notice Checks who the seller for an NFT is if listed in this market.
  */
function _getSellerOf(address nftContract, uint256 tokenId) internal view virtual returns (address payable seller);

/**
  * @notice Checks who the seller for an NFT is if listed in this market.
  */
function _getSellerOrOwnerOf(address nftContract, uint256 tokenId)
  internal
  view
  virtual
  returns (address payable sellerOrOwner);

_getSellerOrOwnerOf has the same @notice comment as _getSellerOf. Update it to include owner.

[L-05] ERC-1165 Typo

NFTCollectionFactory.sol#L60

* @dev This creates and initializes an ERC-1165 minimal proxy pointing to a NFT collection contract implementation.

Minimal proxy should be ERC-1167.

Non-critical Vulnerabilities

[N-01] Inconsistent event emission placement

NFTCollectionFactory.sol#L226-L247 In adminUpdateNFTDropCollectionImplementation, the event is emitted in the middle of the function. As every other functions in the contract place the emission at the end of the function, it will be more consistent to place the ImplementationNFTDropCollectionUpdated event emission at the end as well.

[N-02] Missing NFTDropCollection name validation

NFTDropCollection.sol#L130 During the initialisation of NFTDropCollection, there is a check to ensure that _symbol is not empty. However, the same check is missing for _name.