Foundation Drop contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

User can mint more than saleConfig.limitPerAccount.

Proof of Concept

• User can mint up to limit, transfer them to another account, then mint more.
• Limiting the number of NFTs an account can hold won’t work because users can send all NFTs to a different address. They can stockpile all of them in a single address.

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

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

Tools Used

Manual Review.

Recommended Mitigation Steps

I recommend adding logic that disables transferring NFT until after a certain time.

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Floating Pragma

A floating pragma might result in contract being tested/deployed with different or inconsistent compiler versions possibly leading to unexpected behaviour. 1 instance of this issue has been found:

[L-01] NFTCollection.sol#L3

pragma solidity ^0.8.12;

[L-02] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. 1 instance of this issue has been found:

[L-02] NFTCollectionFactory.sol#L449-L450

    return keccak256(abi.encodePacked(creator, nonce));

Non-critical Findings

[N-01] Remove TODOs

Please remove TODOs as they harm readability. 1 instance of this issue has been found:

[N-01] MarketFees.sol#L193-L194

      // TODO add referral info

[N-02] Function doesn’t currently use try/catch as spec suggests

Function can be turned into internal. 1 instance of this issue has been found:

[N-02] MarketFees.sol#L212-L223

  /**
   * @notice **For internal use only.**
   * @dev This function is external to allow using try/catch but is not intended for external use.
   * This checks the token creator.
   */
  function internalGetTokenCreator(address nftContract, uint256 tokenId)
    external
    view
    returns (address payable creator)
  {
    creator = ITokenCreator(nftContract).tokenCreator{ gas: READ_ONLY_GAS_LIMIT }(tokenId);
  }

Gas Optimisations

Gas overview per contract

Gas Optimisations

[G-01] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

1 instance of this issue has been found:

[G-01] MarketFees.sol#L198-L199

    for (uint256 i = 0; i < creatorShares.length; ++i) {

[G-02] Setting variable to default value is redundant

Setting variable to default value is redundant. 5 instances of this issue have been found:

[G-02] BytesLibrary.sol#L44-L45

      for (uint256 i = 0; i < 4; ++i) {

[G-02b] BytesLibrary.sol#L25-L26

      for (uint256 i = 0; i < 20; ++i) {

[G-02c] MarketFees.sol#L484-L485

          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-02d] MarketFees.sol#L198-L199

    for (uint256 i = 0; i < creatorShares.length; ++i) {

[G-02e] MarketFees.sol#L126-L127

      for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-03] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

4 instances of this issue have been found:

[G-03] MarketFees.sol#L503-L504

      for (uint256 i = 1; i < creatorRecipients.length; ) {

[G-03b] MarketFees.sol#L484-L485

          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-03c] MarketFees.sol#L198-L199

    for (uint256 i = 0; i < creatorShares.length; ++i) {

[G-03d] MarketFees.sol#L126-L127

      for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-04] Implementing return is redundant if function already has a named returns method implemented

Redundant return methods increase gas on deployment and execution. 5 instances of this issue have been found:

[G-04] NFTDropMarketFixedPriceSale.sol#L230-L252

    returns (uint256 numberThatCanBeMinted)
  {
    (, , uint256 limitPerAccount, uint256 numberOfTokensAvailableToMint, bool marketCanMint) = getFixedPriceSale(
      nftContract
    );
    if (!marketCanMint) {
      // No one can mint in the current state.
      return 0;
    }
    uint256 currentBalance = IERC721(nftContract).balanceOf(user);
    if (currentBalance >= limitPerAccount) {
      // User has exhausted their limit.
      return 0;
    }

    uint256 availableToMint = limitPerAccount - currentBalance;
    if (availableToMint > numberOfTokensAvailableToMint) {
      // User has more tokens available than the collection has available.
      return numberOfTokensAvailableToMint;
    }

    return availableToMint;
  }

[G-04b] MarketFees.sol#L208-L210

  function getRoyaltyRegistry() external view returns (address registry) {
    return address(royaltyRegistry);
  }

[G-04c] FoundationTreasuryNode.sol#L59-L61

  function getFoundationTreasury() public view returns (address payable treasuryAddress) {
    return treasury;
  }

[G-04d] NFTDropMarket.sol#L136-L149

    returns (address payable sellerOrOwner)
  {
    // Check the current owner first in case it has been sold.
    try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {
      if (owner != address(0)) {
        // Once an NFT has been minted, it cannot be sold through this contract.
        revert NFTDropMarket_NFT_Already_Minted();
      }
    } catch // solhint-disable-next-line no-empty-blocks
    {
      // Fall through
    }

    return super._getSellerOf(nftContract, tokenId);

[G-04e] NFTDropMarket.sol#L112-L125

    returns (address payable seller)
  {
    // Check the current owner first in case it has been sold.
    try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {
      if (owner != address(0)) {
        // If sold, return address(0) since that owner cannot sell via this market.
        return payable(address(0));
      }
    } catch // solhint-disable-next-line no-empty-blocks
    {
      // Fall through
    }

    return super._getSellerOf(nftContract, tokenId);