Foundation Drop contest - pfapostol's results

Summary

Gas savings are estimated using yarn test-gas-stories and may differ depending on the implementation of the fix. I keep my version of the fix for each finding and can provide them if you need.

Gas Optimizations

Total: 53 instances over 4 issues

1. Use Custom Errors instead of Revert/Require Strings to save Gas (28 instances)

   Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

   Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Additional gas is saved due to the lack of defining string. https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth

   ---

   Require()/revert() strings longer than 32 bytes cost extra gas (28 instances)

   The code and gas usage estimation are merged due to the same PoC code and simplicity for gas usage estimation.

   Deployment Gas Saved: 631685

   ---

   • 2022-08-foundation/contractsNFTCollectionFactory.sol:173,182,203,227,262,

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

   • 2022-08-foundation/contractsNFTDropCollection.sol:88,93,130-131,172,179,238,

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

   • 2022-08-foundation/contractsNFTCollection.sol:158,263-264,268,327,

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

   • 2022-08-foundation/contracts/libraries/AddressLibrary.sol:31

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

   • 2022-08-foundation/contracts/mixins/roles/AdminRole.sol:19

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

   • 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:22,31

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

   • 2022-08-foundation/contracts/mixins/roles/MinterRole.sol:22

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

   • 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:58,63,74,87-89,

   58      require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
   ...
   63      require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
   ...
   74      require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
   ...
   87      require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
   88      require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
   89      require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

2. Storage variables that use more than once can be cached (8 instances)

   Deployment Gas Saved: 11257

   • contracts/NFTCollectionFactory.sol:207,213-214,231,239-240,

   207      versionNFTCollection++;
   ...
   213      string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
   214      string.concat("NFTv", versionNFTCollection.toString())
   ...
   231      versionNFTDropCollection++;
   ...
   239      string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
   240      string.concat("NFTDropV", versionNFTDropCollection.toString()),

   fix:

   uint32 _version;
   unchecked {
      // Version cannot overflow 256 bits.
      _version = ++versionNFTCollection;
   }
   ...
      string.concat("NFT Collection Implementation v", _version.toString()),
      string.concat("NFTv", _version.toString())
   );
   
   emit ImplementationNFTCollectionUpdated(_implementation, _version);

3. x = x + 1 is more efficient than increment (11 instances)

   Deployment Gas Saved: 2604

   • audit/NFTCollectionFactory.sol:207,231,

   207     versionNFTCollection++;
   ...
   231     versionNFTDropCollection++;

   • contracts/NFTDropCollection.sol184,

   184     ++i;

   • contracts/NFTCollection.sol267,

   267     tokenId = ++latestTokenId;

   • contracts/mixins/shared/MarketFees.sol126,198,484,508,

   126     for (uint256 i = 0; i < creatorRecipients.length; ++i) {
   ...
   198     for (uint256 i = 0; i < creatorShares.length; ++i) {
   ...
   484     for (uint256 i = 0; i < creatorRecipients.length; ++i) {
   ...
   508     ++i;

   • audit/BytesLibrary.sol25,44,

   25      for (uint256 i = 0; i < 20; ++i) {
   ...
   44      for (uint256 i = 0; i < 4; ++i) {

   • audit/SequentialMintCollection.sol98,

   98      ++burnCounter;

4. Usage of variable less than 256 bit (6 instances)

   Deployment gas saved: 266482 User story gas losted: -230800

   • contracts/NFTCollectionFactory.sol:80,95

   80    uint32 public versionNFTCollection;
   ...
   95    uint32 public versionNFTDropCollection;

   • audit/Constants.sol:38

   38    uint96 constant ROYALTY_IN_BASIS_POINTS = 1000;

   • audit/SequentialMintCollection.sol:27,34,40

   27    uint32 public latestTokenId;
   ...
   34    uint32 public maxTokenId;
   ...
   40    uint32 private burnCounter;