Foundation Drop contest - ElKu's results

Non-Critical Issues

1. <ins>Non-library/interface files should use fixed compiler versions, not floating ones</ins>

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

All the files under scope use a floating pragma as shown below:

pragma solidity ^0.8.12;

Mitigation would be to fix it to a particular version. For example:

pragma solidity 0.8.12;

2. <ins>Open TODO</ins>

Open TODO should be fixed. One instance in MarketFees.sol.

// TODO add referral info

Gas Optimizations

Summary Of Gas Savings:

Detailed Report on Gas Optimization Findings:

1. <ins>NFTDropCollection.sol</ins>

In the mintCountTo function state variable latestTokenId was read 3 times and few times inside a for loop. By using a cached copy, we could save at least 200 gas.

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

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

    for (uint256 i = firstTokenId; i <= cachedlatestTokenId; ) {   //read cached variable here
      _mint(to, i);
      unchecked {
        ++i;
      }
    }
    latestTokenId = cachedlatestTokenId;  //write back to state variable at the end
  }

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 169 gas. while the max gas usage was reduced from 306257 to 305073(a saving of 1184).

2. <ins>NFTDropMarketFixedPriceSale.sol</ins>

In the createFixedPriceSale function state variable saleConfig is read to emit an event. If possible, local variables should be used in the emit.

//Original Code:
  emit CreateFixedPriceSale(nftContract, saleConfig.seller, saleConfig.price, saleConfig.limitPerAccount);

//Can be optimized into:
  emit CreateFixedPriceSale(nftContract, payable(msg.sender), price, limitPerAccount);

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 43 gas.

3. <ins>Use custom errors instead of revert strings</ins>

Another major area in which we could save deployment cost would be in converting the revert strings into custom errors. If the function does revert, you can also save on dynamic gas cost. See this example implementation to understand the dynamics of custom errors. It shows that each require string converted into a custom error saves you around 11000 gas.

Many other files in scope have already implemented custom errors. The rest are listed below:

File:     contracts/NFTCollectionFactory.sol
---------------------------------------------

Line 173:       require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
Line 182:       require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
Line 203:       require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
Line 227:       require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
Line 262:       require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

File:     contracts/libraries/AddressLibrary.sol
-------------------------------------------------

Line 031:       require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

File:     contracts/NFTDropCollection.sol
-------------------------------------------

Line 088:       require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
Line 093:       require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
Line 130:       require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
Line 131:       require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
Line 172:       require(count != 0, "NFTDropCollection: `count` must be greater than 0");
Line 179:       require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
Line 238:       require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

File:     contracts/mixins/shared/ContractFactory.sol
--------------------------------------------------------

Line 022:       require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
Line 031:       require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

File:     contracts/mixins/roles/AdminRole.sol
-----------------------------------------------

Line 019:       require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

File:     contracts/mixins/roles/MinterRole.sol
------------------------------------------------

Line 022:       require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

File:     contracts/mixins/collections/SequentialMintCollection.sol
--------------------------------------------------------------------

Line 058:       require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
Line 063:       require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
Line 074:       require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
Line 087-089:   require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
                require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
                require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");      

File:     contracts/NFTCollection.sol
--------------------------------------

Line 158:       require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
Line 263:       require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
Line 264:       require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
Line 268:       require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
Line 327:       require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
     

1. NFTCollectionFactory.sol
2. AddressLibrary.sol
3. NFTDropCollection.sol
4. ContractFactory.sol
5. AdminRole.sol
6. MinterRole.sol
7. SequentialMintCollection.sol
8. NFTCollection.sol

There are a total of 5 + 1 + 7 + 2 + 1 + 1 + 6 + 5 = 28 require strings spread out in 8 files. Converting all of them into custom errors can save us around 28 * 11000 = 308,000 gas when deploying.

Conclusions:

The above changes reduced the deployment cost by 308,000. And Dynamic cost was reduced by ‭1039‬.