Gas Optimizations

[code423n4]

Table of Contents

• Use Function Parameter Instead of State Variable
• Duplicate require() Checks Should be a Modifier or a Function
• Internal Function Called Only Once can be Inlined
• Constant Value of a Call to keccak256() should Use Immutable
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Unnecessary Default Value Initialization
• Store Array's Length as a Variable
• != 0 costs less gass than > 0
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

Use Function Parameter Instead of State Variable

Issue

Using function parameter value instead of SLOAD state variable saves gas.

PoC

Total of 2 issues found.

1. Use parameter _baseURI of updatePreRevealContent() of NFTDropCollection.sol
   Change line 242 from state variable of baseURI to parameter of _baseURI.
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L242

2. Use parameter _postRevealBaseURIHash of updatePreRevealContent() of NFTDropCollection.sol
   Change line 242 from state variable of postRevealBaseURIHash to parameter of _postRevealBaseURIHash.
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L242

Mitigation

Change state variable to parameter variable as listed in above PoC.

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once,
I recommend making these to a modifier or a function.

PoC

Total of 1 issue found.

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

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once.
Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 3 issues found.

1. _initializeAdminRole() of AdminRole.sol
   Function called only once at line 148 of NFTDropCollection.sol
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/AdminRole.sol#L13
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L148

2. _tryUseFETHBalance() of FETHNode.sol
   Function called only once at line 204 of NFTDropMarketFixedPriceSale.sol
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FETHNode.sol#L46
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L204

3. _initializeMinterRole() of MinterRole.sol
   Function called only once at line 150 of NFTDropCollection.sol
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/MinterRole.sol#L26
   https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L150

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time.
However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced.
Resulting in costing about 100 gas more on each access to this constant.
Link for more details: ethereum/solidity#9232

PoC

Total of 2 issues found.

./NFTDropMarketFixedPriceSale.sol:70:  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
./MinterRole.sol:19:  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

Mitigation

Change the variable from constant to immutable.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations
in order to reduce the elements from 32 bytes to specified size.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 13 issues found.

./NFTDropMarketFixedPriceSale.sol:120:    uint80 price,
./NFTDropMarketFixedPriceSale.sol:121:    uint16 limitPerAccount
./NFTDropMarketFixedPriceSale.sol:172:    uint16 count,
./SequentialMintCollection.sol:62:  function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
./SequentialMintCollection.sol:86:  function _updateMaxTokenId(uint32 _maxTokenId) internal {
./NFTDropCollection.sol:126:    uint32 _maxTokenId,
./NFTDropCollection.sol:220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
./NFTCollectionFactory.sol:192:  function initialize(uint32 _versionNFTCollection) external initializer {
./NFTCollectionFactory.sol:291:    uint32 maxTokenId,
./NFTCollectionFactory.sol:329:    uint32 maxTokenId,
./NFTCollectionFactory.sol:368:    uint32 maxTokenId,
./NFTCollectionFactory.sol:391:    uint32 maxTokenId,
./NFTCollection.sol:251:  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

Mitigation

I suggest using uint256 instead of anything smaller or downcast where needed.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values.
For example, 0 for uint, false for bool and address(0) for address.
Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 5 issues found.

./MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
./MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./BytesLibrary.sol:25:      for (uint256 i = 0; i < 20; ++i) {
./BytesLibrary.sol:44:      for (uint256 i = 0; i < 4; ++i) {

Mitigation

I suggest removing default value initialization.
For example,

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

 

Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop,
can save 3 gas per iteration.

PoC

Total of 4 issues found.

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

Mitigation

Store array's length as a variable before looping it.

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

Total of 3 issues found.

./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");

Mitigation

I suggest changing > 0 to != 0

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need
extra storage slot leading to more gas cost.

PoC

Total of 28 issues found.

./SequentialMintCollection.sol:58:    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
./SequentialMintCollection.sol:63:    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
./SequentialMintCollection.sol:74:    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
./SequentialMintCollection.sol:87:    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
./SequentialMintCollection.sol:88:    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
./SequentialMintCollection.sol:89:    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
./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");
./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");
./AdminRole.sol:19:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
./ContractFactory.sol:22:    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
./ContractFactory.sol:31:    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
./AddressLibrary.sol:31:    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
./MinterRole.sol:22:    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
./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");

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings.
Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

Total of 28 issues found.

./SequentialMintCollection.sol:58:    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
./SequentialMintCollection.sol:63:    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
./SequentialMintCollection.sol:74:    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
./SequentialMintCollection.sol:87:    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
./SequentialMintCollection.sol:88:    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
./SequentialMintCollection.sol:89:    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
./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");
./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");
./AdminRole.sol:19:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
./ContractFactory.sol:22:    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
./ContractFactory.sol:31:    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
./AddressLibrary.sol:31:    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
./MinterRole.sol:22:    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
./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");

Mitigation

I suggest implementing custom errors to save gas.

 

[batu-inal]

Use Function Parameter Instead of State Variable

Agree, will make some improvements here.

Duplicate require() Checks Should be a Modifier or a Function

Agreed. This is not a gas optimization; however, increases readability and maintainability.

Internal Function Called Only Once can be Inlined

Potentially valid but won't fix. While there may be trivial gas savings here we like the readability and maintainability of this pattern.

Constant Value of a Call to keccak256() should Use Immutable

Valid but won't fix. While there may be gas savings here we like the readability and maintainability of this pattern. Adding hard-coded hashes makes the code fragile and harder to read.

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Potentially valid but won't fix. This is uint32 to allow packing storage for a significant gas savings. Accepting the input as uint256 would incur additional overhead in checking for overflow before saving these values. Other inputs are logically limited to the size exposed, huge values cannot be used. Accepting the input as uint256 would incur additional overhead in checking for overflow before using these values.

Unnecessary Default Value Initialization

Invalid. This optimization technique is no longer applicable with the current version of Solidity.

Store Array's Length as a Variable

May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.

!= 0 costs less gass than > 0

Invalid. We tested the recommendation and got the following results:

createNFTDropCollection gas reporter results:
  using > 0 (current):
    - 319246  ·     319578  ·      319361
  using != 0 (recommendation):
    -  319252  ·     319584  ·      319367
  impact: +6 gas

Keep The Revert Strings of Error Messages within 32 Bytes

Invalid. The baseURI cannot be stored in bytes32 because it may be longer than 32 bytes. It's possible this technique could work for name or symbol, however in order to stay flexible in case a creator would like to use a long value here -- we will keep these as strings.

Use Custom Errors to Save Gas

Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.