Gas Optimizations

[code423n4]

Overview

Risk Rating	Number of issues
Gas Issues	11

Codebase Impressions

Overall, the code is pretty optimized:

• Using clones to deploy contracts is an excellent call
• The unchecked statements are well used and only one suggestion was made (albeit for an external view function)
• Storage variables are tightly packed and fetching structs are well done using the memory keyword (FixedPriceSaleConfig has 3 variables but takes 1 slot so using memory is relevant)

Just one particular teaching is to be reminded:

• The revert strings are too long across the project. Please try to make them fit in 32 bytes (use the first letters of the contract as a prefix, as an example, like NFTCF instead of NFTCollectionFactory)

Due to some inconsistencies with the gas-stories.txt, I decided to not attach it to this report.

Table of Contents:

• 1. Caching storage values in memory
• 2. Avoid emitting a storage variable when a memory value is available
• 3. Unchecking arithmetics operations that can't underflow/overflow
• 4. Use calldata instead of memory
• 5. Reduce the size of error messages (Long revert Strings)
• 6. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 7. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers
• 8. <array>.length should not be looked up in every loop of a for-loop
• 9. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 10. Increments/decrements can be unchecked in for-loops
• 11. Use Custom Errors instead of Revert Strings to save Gas

1. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• Saving 3 SLOADs (+ a pre-increment is cheaper, but this is counter-balanced with the memory variable).

File: NFTCollectionFactory.sol
202:   function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
203:     require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
204:     implementationNFTCollection = _implementation;
+ 204:     uint32 _versionNFTCollection;
205:     unchecked {
206:       // Version cannot overflow 256 bits.
- 207:       versionNFTCollection++;
+ 207:       _versionNFTCollection = ++versionNFTCollection;
208:     }
209: 
210:     // The implementation is initialized when assigned so that others may not claim it as their own.
211:     INFTCollectionInitializer(_implementation).initialize(
212:       payable(address(rolesContract)),
- 213:       string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
+ 213:       string.concat("NFT Collection Implementation v", _versionNFTCollection.toString()),
- 214:       string.concat("NFTv", versionNFTCollection.toString())
+ 214:       string.concat("NFTv", _versionNFTCollection.toString())
215:     );
216: 
- 217:     emit ImplementationNFTCollectionUpdated(_implementation, versionNFTCollection);
+ 217:     emit ImplementationNFTCollectionUpdated(_implementation, _versionNFTCollection);
218:   }

• Saving 3 SLOADs (+ a pre-increment is cheaper, but this is counter-balanced with the memory variable).

File: NFTCollectionFactory.sol
226:   function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
227:     require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
228:     implementationNFTDropCollection = _implementation;
+ 228:         uint32 _versionNFTDropCollection;
229:     unchecked {
230:       // Version cannot overflow 256 bits.
- 231:       versionNFTDropCollection++;
+ 231:       _versionNFTDropCollection = ++versionNFTDropCollection;
232:     }
233: 
- 234:     emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection);
+ 234:     emit ImplementationNFTDropCollectionUpdated(_implementation, _versionNFTDropCollection);
235: 
236:     // The implementation is initialized when assigned so that others may not claim it as their own.
237:     INFTDropCollectionInitializer(_implementation).initialize(
238:       payable(address(this)),
- 239:       string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
- 240:       string.concat("NFTDropV", versionNFTDropCollection.toString()),
+ 239:       string.concat("NFT Drop Collection Implementation v", _versionNFTDropCollection.toString()),
+ 240:       string.concat("NFTDropV", _versionNFTDropCollection.toString()),
241:       "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
242:       0x1337000000000000000000000000000000000000000000000000000000001337,
243:       1,
244:       address(0),
245:       payable(0)
246:     );
247:   }

• Saving 1 SLOAD. If we're optimistic towards the presence of a baseURI_ string here, this should be cached:

File: NFTCollection.sol
332:   function _baseURI() internal view override returns (string memory) {
- 333:     if (bytes(baseURI_).length != 0) {
+ 333:     string memory memBaseURI = baseURI_;
+ 333:     if (bytes(memBaseURI).length != 0) {
- 334:       return baseURI_;
+ 334:       return memBaseURI;
335:     }
336:     return "ipfs://";
337:   }

2. Avoid emitting a storage variable when a memory value is available

When they are the same, consider emitting the memory value instead of the storage value:

File: NFTDropCollection.sol
232:   function updatePreRevealContent(string calldata _baseURI, bytes32 _postRevealBaseURIHash)
233:     external
234:     validBaseURI(_baseURI)
235:     onlyWhileUnrevealed
236:     onlyAdmin
237:   {
238:     require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
239: 
240:     postRevealBaseURIHash = _postRevealBaseURIHash;
241:     baseURI = _baseURI;
- 242:     emit URIUpdated(baseURI, postRevealBaseURIHash);
+ 242:     emit URIUpdated(_baseURI, _postRevealBaseURIHash);
243:   }

File: NFTDropMarketFixedPriceSale.sol
152:     // Save the sale details.
153:     saleConfig.seller = payable(msg.sender);
154:     saleConfig.price = price;
155:     saleConfig.limitPerAccount = limitPerAccount;
- 156:     emit CreateFixedPriceSale(nftContract, saleConfig.seller, saleConfig.price, saleConfig.limitPerAccount);
+ 156:     emit CreateFixedPriceSale(nftContract, payable(msg.sender), price, limitPerAccount);

3. Unchecking arithmetics operations that can't underflow/overflow

While this is inside an external view function, consider wrapping this in an unchecked statement so that external contracts calling this might save some gas:

• L245 can be unchecked due to L240

File: NFTDropMarketFixedPriceSale.sol
240:     if (currentBalance >= limitPerAccount) {
241:       // User has exhausted their limit.
242:       return 0;
243:     }
244: 
- 245:     uint256 availableToMint = limitPerAccount - currentBalance;
+ 245:     uint256 availableToMint;
+ 245:     unchecked { availableToMint = limitPerAccount - currentBalance; }

4. Use calldata instead of memory

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly bypasses this loop.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gas-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Affected code (around 60 gas to be saved):

File: NFTCollectionFactory.sol
363:   function createNFTDropCollectionWithPaymentFactory(
364:     string calldata name,
365:     string calldata symbol,
366:     string calldata baseURI,
367:     bytes32 postRevealBaseURIHash,
368:     uint32 maxTokenId,
369:     address approvedMinter,
370:     uint256 nonce,
- 371:     CallWithoutValue memory paymentAddressFactoryCall
+ 371:     CallWithoutValue calldata paymentAddressFactoryCall
372:   ) external returns (address collection) {

5. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

libraries/AddressLibrary.sol:31:    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
mixins/collections/SequentialMintCollection.sol:58:    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
mixins/collections/SequentialMintCollection.sol:63:    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
mixins/collections/SequentialMintCollection.sol:74:    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
mixins/collections/SequentialMintCollection.sol:87:    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
mixins/collections/SequentialMintCollection.sol:88:    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
mixins/collections/SequentialMintCollection.sol:89:    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
mixins/roles/AdminRole.sol:19:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
mixins/roles/MinterRole.sol:22:    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
mixins/shared/ContractFactory.sol:22:    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
mixins/shared/ContractFactory.sol:31:    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
NFTCollection.sol:263:    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
NFTCollection.sol:264:    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
NFTCollection.sol:268:      require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
NFTCollection.sol:327:    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
NFTCollectionFactory.sol:173:    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
NFTCollectionFactory.sol:182:    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:262:    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
NFTDropCollection.sol:93:    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
NFTDropCollection.sol:130:    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
NFTDropCollection.sol:131:    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
NFTDropCollection.sol:172:    require(count != 0, "NFTDropCollection: `count` must be greater than 0");
NFTDropCollection.sol:179:    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
NFTDropCollection.sol:238:    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

Consider shortening the revert strings to fit in 32 bytes.

6. Duplicated conditions should be refactored to a modifier or function to save deployment costs

NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

7. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers

Up until Solidity 0.8.13: != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

As the Solidity version used here is 0.8.12, consider changing > 0 with != 0 here:

NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
NFTDropCollection.sol:130:    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
NFTDropCollection.sol:131:    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

Also, please enable the Optimizer.

8. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

mixins/shared/MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
mixins/shared/MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
mixins/shared/MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {
mixins/shared/MarketFees.sol:503:      for (uint256 i = 1; i < creatorRecipients.length; ) {

9. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

NFTCollectionFactory.sol:207:      versionNFTCollection++;
NFTCollectionFactory.sol:231:      versionNFTDropCollection++;

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

10. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

mixins/shared/MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
mixins/shared/MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existent for uint256 here.

11. Use Custom Errors instead of Revert Strings to save Gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Additionally, custom errors can be used inside and outside of contracts (including interfaces and libraries).

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:

NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

[kartoonjoy]

Per Dravee, this gas report is to be withdrawn since a secondary gas report was completed instead. Please only consider the issue 122, only. Thanks