Foundation Drop contest - Dravee's results

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. _safeMint() should be used rather than _mint() wherever possible
  • 1.2. Uninitialized Upgradeable contract
  • 1.3. Unclear Code Comment
  • 1.4. Incorrect Code Comment
  • 1.5. Inconsistent use of __gap[...]
  • 1.6. Inconsistent use of Symbol is required check
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. A magic value should be documented and explained. Use a constant instead
  • 2.3. Open TODOS
  • 2.4. Default Visibility
  • 2.5. Use bytes.concat()
  • 2.6. Adding a return statement when the function defines a named return variable, is redundant
  • 2.7. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver, see the WARNING L278:

File: ERC721Upgradeable.sol
275:     /**
276:      * @dev Mints `tokenId` and transfers it to `to`.
277:      *
278:      * WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
...
286:      */
287:     function _mint(address to, uint256 tokenId) internal virtual {

Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

Consider replacing with _safeMint() here:

NFTCollection.sol:271:      _mint(msg.sender, tokenId);
NFTDropCollection.sol:182:      _mint(to, i); 

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful.

As an example, here, the CEIP isn't respected:

File: NFTCollection.sol
154:   function mintWithCreatorPaymentAddress(string calldata tokenCID, address payable tokenCreatorPaymentAddress)
155:     public
156:     returns (uint256 tokenId)
157:   {
158:     require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
159:     tokenId = _mint(tokenCID); //@audit CEIP not respected (if we assume this will be replaced with safeMint, it's important)
160:     tokenIdToCreatorPaymentAddress[tokenId] = tokenCreatorPaymentAddress;
161:     emit TokenCreatorPaymentAddressSet(address(0), tokenCreatorPaymentAddress, tokenId);
162:   }

As tokenId is needed L160, the lines L159 and L160 can't be swapped. The only mitigation here is to use a nonReentrant modifier on state-updating functions.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.2. Uninitialized Upgradeable contract

Similar issue in the past: here

Upgradeable dependencies should be initialized. While not causing any harm at the moment, suppose OZ someday decide to upgrade this contract and the sponsor uses the new version: it is possible that the contract will not work anymore. Consider calling the init() here:

• NFTDropCollection.sol:

File: NFTDropCollection.sol
028: contract NFTDropCollection is
...
037:   ContextUpgradeable,
038:   ERC165Upgradeable,
039:   AccessControlUpgradeable,
...
042:   ERC721Upgradeable,
043:   ERC721BurnableUpgradeable,
...
120:   function initialize(
...
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
...
134:     __ERC721_init(_name, _symbol);
+ 135:     __Context_init();
+ 135:     __AccessControl_init();
+ 135:     __ERC165_init();
+ 135:     __ERC721Burnable_init();

• NFTCollection.sol:

File: NFTCollection.sol
028: contract NFTCollection is
...
036:   ERC165Upgradeable,
037:   ERC721Upgradeable,
038:   ERC721BurnableUpgradeable,
...
105:   function initialize(
...
109:   ) external initializer onlyContractFactory {
110:     __ERC721_init(_name, _symbol); //@audit missing __ERC165_init(); and __ERC721Burnable_init;
+ 110:     __ERC165_init();
+ 110:     __ERC721Burnable_init();
...
112:   }

1.3. Unclear Code Comment

Issue with comment

The following deserves to be clarified:

File: NFTDropCollection.sol
120:   function initialize(
121:     address payable _creator,
122:     string calldata _name,
123:     string calldata _symbol,
124:     string calldata _baseURI,
125:     bytes32 _postRevealBaseURIHash,
126:     uint32 _maxTokenId,
127:     address _approvedMinter,
128:     address payable _paymentAddress
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
...
137:     // Initialize royalties
138:     if (_paymentAddress != address(0)) {
139:       // If no payment address was defined, use the creator's address. //@audit should clarify better, here we're thinking that something is missing
140:       paymentAddress = _paymentAddress;
141:     }

Here, the comment can make us believe that the state variable paymentAddress should be set to the _creator argument if the initialize function didn't pass a _paymentAddress argument. We could even think that a else statement is missing. However, what it means here is that if paymentAddress == address(0) is true in the solution, it will let the owner variable (which corresponds to the creator) take precedence, as seen in getTokenCreatorPaymentAddress():

File: NFTDropCollection.sol
252:   function getTokenCreatorPaymentAddress(
253:     uint256 /* tokenId */
254:   ) public view override returns (address payable creatorPaymentAddress) {
255:     creatorPaymentAddress = paymentAddress;
256:     if (creatorPaymentAddress == address(0)) {
257:       creatorPaymentAddress = owner;
258:     }
259:   }

The comment can be clearer, maybe like this: If no payment address was defined, the owner (aka the creator's address) will be returned in getters later on

1.4. Incorrect Code Comment

Issue with comment

// SafeMath is not required when dividing by a non-zero constant.

Here, while what is meant is understandable, the solidity version used is 0.8.12 and the SafeMath lib isn't imported:

contracts/mixins/shared/MarketFees.sol:
  3: pragma solidity ^0.8.12;

  409      unchecked {
  410:       // SafeMath is not required when dividing by a non-zero constant. 
  411        totalFees = price / PROTOCOL_FEE_DENOMINATOR;
  412      }

  469          unchecked {
  470:           // SafeMath is not required when dividing by a non-zero constant. 
  471            creatorRev = price / CREATOR_ROYALTY_DENOMINATOR;
  472          }

Consider replacing the comment with something along the line of Can't overflow when dividing by a non-zero constant

1.5. Inconsistent use of __gap[...]

While some contracts inherit from Gap10000.sol, others declare their own private __gap :

contracts/NFTCollectionFactory.sol:
  62: contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000 {

contracts/NFTDropMarket.sol:
  63  contract NFTDropMarket is
....
  72:   Gap10000,

contracts/mixins/nftDropMarket/NFTDropMarketCore.sol:
  17:   uint256[1000] private __gap;

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:
  313:   uint256[1000] private __gap;

contracts/mixins/roles/AdminRole.sol:
  56:   uint256[1000] private __gap;

contracts/mixins/shared/FoundationTreasuryNode.sol:
  68:   uint256[2000] private __gap;

contracts/mixins/shared/MarketFees.sol:
  537:   uint256[1000] private __gap;

contracts/mixins/shared/MarketSharedCore.sol:
  43:   uint256[500] private __gap;

contracts/mixins/shared/SendValueWithFallbackWithdraw.sol:
  57:   uint256[999] private __gap;

Furthermore, NFTDropCollection and NFTCollection don't have their own private __gap and don't inherit from Gap10000, while they are upgradable.

Consider correcting these inconsistencies.

1.6. Inconsistent use of Symbol is required check

For NFTCollection, the check is made in NFTCollectionFactory.createNFTCollection().

• NFTCollectionFactory.sol#L262

File: NFTCollectionFactory.sol
257:   function createNFTCollection(
...
261:   ) external returns (address collection) {
262:     require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
263: 
264:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
265:     collection = implementationNFTCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
266: 
267:     INFTCollectionInitializer(collection).initialize(payable(msg.sender), name, symbol);
268: 
269:     emit NFTCollectionCreated(collection, msg.sender, versionNFTCollection, name, symbol, nonce);
270:   }

However, for NFTDropCollection, the check is made way further, after even the contract's creation (during the initialization):

• NFTDropCollection.sol#L130

File: NFTDropCollection.sol
120:   function initialize(
...
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
130:     require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

Consider moving the check in NFTCollectionFactory._createNFTDropCollection() for consistency:

• NFTCollectionFactory.sol#L396

File: NFTCollectionFactory.sol
386:   function _createNFTDropCollection(
...
395:   ) private returns (address collection) {
396:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
+ 396:     require(bytes(symbol).length ! 0, "NFTDropCollection: `symbol` must be set");
397:     collection = implementationNFTDropCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
398: 
399:     INFTDropCollectionInitializer(collection).initialize(
400:       payable(msg.sender),
401:       name,
402:       symbol,
403:       baseURI,
404:       postRevealBaseURIHash,
405:       maxTokenId,
406:       approvedMinter,
407:       paymentAddress
408:     );

While more consistent, this would also save gas in case of revert.

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

• contracts/NFTCollectionFactory.sol (inconsistent with L217 where the event is indeed emitted after the initialization, as it should):

  234:     emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection); //@audit-issue low: do like L217 and emit after the following .initialize() call
  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()),
  241        "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
  242        0x1337000000000000000000000000000000000000000000000000000000001337,
  243        1,
  244        address(0), 
  245        payable(0)
  246      );
  247    }

• contracts/mixins/shared/MarketFees.sol:

  144      // Pay the buy referrer fee
  145      if (buyReferrerFee != 0) {
  146        _sendValueWithFallbackWithdraw(buyReferrer, buyReferrerFee, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);
  147:       emit BuyReferralPaid(nftContract, tokenId, buyReferrer, buyReferrerFee, 0);
  148        unchecked {
  149          // Add the referrer fee back into the total fees so that all 3 return fields sum to the total price for events
  150          totalFees += buyReferrerFee;
  151        }
  152      }
  153    }

2.2. A magic value should be documented and explained. Use a constant instead

contracts/NFTCollectionFactory.sol:
  241:       "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
  242:       0x1337000000000000000000000000000000000000000000000000000000001337,

Consider using constant variables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).

2.3. Open TODOS

The codebase still contains TODO statements. Consider resolving and removing the TODOs before deploying.

mixins/shared/MarketFees.sol:193:      // TODO add referral info

2.4. Default Visibility

The following constants are using the default visibility:

mixins/shared/Constants.sol:10:uint256 constant BASIS_POINTS = 10000;
mixins/shared/Constants.sol:15:bytes32 constant DEFAULT_ADMIN_ROLE = 0x00;
mixins/shared/Constants.sol:21:uint256 constant MAX_ROYALTY_RECIPIENTS = 5;
mixins/shared/Constants.sol:26:uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;
mixins/shared/Constants.sol:32:uint256 constant READ_ONLY_GAS_LIMIT = 40000;
mixins/shared/Constants.sol:38:uint96 constant ROYALTY_IN_BASIS_POINTS = 1000;
mixins/shared/Constants.sol:43:uint256 constant ROYALTY_RATIO = BASIS_POINTS / ROYALTY_IN_BASIS_POINTS;
mixins/shared/Constants.sol:48:uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;
mixins/shared/Constants.sol:53:uint256 constant SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20000;

For readability, consider explicitly declaring the visibility.

2.5. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

Consider using it here:

NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
NFTCollectionFactory.sol:449:    return keccak256(abi.encodePacked(creator, nonce));

Please notice that Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>)) and that it's extensively used in the solution already, which is a good thing:

contracts/NFTCollection.sol:
  329:     uri = string.concat(_baseURI(), _tokenCIDs[tokenId]);

contracts/NFTCollectionFactory.sol:
  213:       string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
  214:       string.concat("NFTv", versionNFTCollection.toString())
  239:       string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
  240:       string.concat("NFTDropV", versionNFTDropCollection.toString()),

contracts/NFTDropCollection.sol:
  303:     return string.concat(baseURI, tokenId.toString(), ".json");

2.6. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code (using both):

• contracts/NFTCollectionFactory.sol:

  294:   ) external returns (address collection) {
  333:   ) external returns (address collection) {
  372:   ) external returns (address collection) {

• contracts/NFTDropCollection.sol:

  300:   function tokenURI(uint256 tokenId) public view override returns (string memory uri) { 

• contracts/NFTDropMarket.sol:

  112:     returns (address payable seller)
  115:     try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {
  136:     returns (address payable sellerOrOwner)
  139:     try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {

• contracts/libraries/AddressLibrary.sol:

  36:     returns (address payable contractAddress)

• contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:

  230:     returns (uint256 numberThatCanBeMinted)

• contracts/mixins/shared/FoundationTreasuryNode.sol:

  59:   function getFoundationTreasury() public view returns (address payable treasuryAddress) {

• contracts/mixins/shared/MarketSharedCore.sol:

  20:   function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {
  27:   function _getSellerOf(address nftContract, uint256 tokenId) internal view virtual returns (address payable seller);

2.7. Non-library/interface files should use fixed compiler versions, not floating ones

mixins/collections/CollectionRoyalties.sol:3:pragma solidity ^0.8.12;
mixins/collections/SequentialMintCollection.sol:3:pragma solidity ^0.8.12;
mixins/nftDropMarket/NFTDropMarketCore.sol:3:pragma solidity ^0.8.12;
mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:3:pragma solidity ^0.8.12;
mixins/roles/AdminRole.sol:3:pragma solidity ^0.8.12;
mixins/roles/MinterRole.sol:3:pragma solidity ^0.8.12;
mixins/shared/Constants.sol:3:pragma solidity ^0.8.12;
mixins/shared/ContractFactory.sol:3:pragma solidity ^0.8.12;
mixins/shared/FETHNode.sol:3:pragma solidity ^0.8.12;
mixins/shared/FoundationTreasuryNode.sol:3:pragma solidity ^0.8.12;
mixins/shared/Gap10000.sol:3:pragma solidity ^0.8.12;
mixins/shared/MarketFees.sol:3:pragma solidity ^0.8.12;
mixins/shared/MarketSharedCore.sol:3:pragma solidity ^0.8.12;
mixins/shared/OZERC165Checker.sol:3:pragma solidity ^0.8.12;
mixins/shared/SendValueWithFallbackWithdraw.sol:3:pragma solidity ^0.8.12;
NFTCollection.sol:3:pragma solidity ^0.8.12;
NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
NFTDropCollection.sol:3:pragma solidity ^0.8.12;
NFTDropMarket.sol:42:pragma solidity ^0.8.12;

Overview

Codebase Impressions

Overall, the code is pretty optimized:

• Using clones to deploy contracts is an excellent call
• The unchecked statements are well used
• Storage variables are tightly packed

Just one particular finding was present across the whole project:

• The revert strings are too long. 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), or use Custom Errors consistently

Due to some inconsistencies with the gas-stories.txt file, I unfortunately did not attach it.

Table of Contents:

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

1. Check for bytes(_symbol).length > 0 before calling NFTDropCollection.initialize(), like it's done for NFTCollection.initialize()

This could save a lot of gas if the revert condition is met.

For NFTCollection, the check is made in NFTCollectionFactory.createNFTCollection().

• NFTCollectionFactory.sol#L262

File: NFTCollectionFactory.sol
257:   function createNFTCollection( //@audit-ok OK function
258:     string calldata name,
259:     string calldata symbol,
260:     uint256 nonce
261:   ) external returns (address collection) {
262:     require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required"); //@audit why is the check inconsistently done ? Here it's in this function but for drop it's in the initialize() function. Chose 1, I'd advise this one style to save gas.
263: 
264:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
265:     collection = implementationNFTCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
266: 
267:     INFTCollectionInitializer(collection).initialize(payable(msg.sender), name, symbol);
268: 
269:     emit NFTCollectionCreated(collection, msg.sender, versionNFTCollection, name, symbol, nonce);
270:   }

However, for NFTDropCollection, the check is made way further, after even the contract's creation (during the initialization):

• NFTDropCollection.sol#L130

File: NFTDropCollection.sol
120:   function initialize(
121:     address payable _creator,
122:     string calldata _name,
123:     string calldata _symbol,
124:     string calldata _baseURI,
125:     bytes32 _postRevealBaseURIHash,
126:     uint32 _maxTokenId,
127:     address _approvedMinter,
128:     address payable _paymentAddress
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
130:     require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
131:     require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

Consider moving the check in NFTCollectionFactory._createNFTDropCollection():

• NFTCollectionFactory.sol#L396

File: NFTCollectionFactory.sol
386:   function _createNFTDropCollection(
387:     string calldata name,
388:     string calldata symbol,
389:     string calldata baseURI,
390:     bytes32 postRevealBaseURIHash,
391:     uint32 maxTokenId,
392:     address approvedMinter,
393:     address payable paymentAddress,
394:     uint256 nonce
395:   ) private returns (address collection) {
396:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
+ 396:     require(bytes(symbol).length ! 0, "NFTDropCollection: `symbol` must be set");
397:     collection = implementationNFTDropCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
398: 
399:     INFTDropCollectionInitializer(collection).initialize(
400:       payable(msg.sender),
401:       name,
402:       symbol,
403:       baseURI,
404:       postRevealBaseURIHash,
405:       maxTokenId,
406:       approvedMinter,
407:       paymentAddress
408:     );

This would save the deployment cost of an impossible to initialize contract (which would further need to be destroyed before being redeployed).

2. 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 many SLOADs (including in a for-loop):

File: NFTDropCollection.sol
171:   function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) { //@audit-ok
172:     require(count != 0, "NFTDropCollection: `count` must be greater than 0");
173: 
+ 173:      uint32 _latestTokenId = latestTokenId;
174:     unchecked {
175:       // If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits
- 176:       firstTokenId = latestTokenId + 1; //@audit gas: SLOAD 1 (latestTokenId)
+ 176:       firstTokenId = _latestTokenId + 1;
177:     }
- 178:     latestTokenId = latestTokenId + count; //@audit gas: SLOAD 2 (latestTokenId)
+ 178:     _latestTokenId = _latestTokenId + count;
+ 178:     latestTokenId = _latestTokenId;
- 179:     require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); //@audit gas: SLOAD 3 (latestTokenId)
+ 179:     require(_latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
180: 
- 181:     for (uint256 i = firstTokenId; i <= latestTokenId; ) {  //@audit gas: SLOAD "latestTokenId - firstTokenId + 1" (latestTokenId)
+ 181:     for (uint256 i = firstTokenId; i <= _latestTokenId; ) {
182:       _mint(to, i);
183:       unchecked {
184:         ++i;
185:       }
186:     }
187:   }

• 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:   }

3. 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:

• NFTDropCollection.sol#L242

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:   }

• NFTDropMarketFixedPriceSale.sol#L156

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

4. 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; }

5. 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) {

6. 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.

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

8. Redundant check

The following require statement is redundant:

• SequentialMintCollection.sol#L63

File: SequentialMintCollection.sol
62:   function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
- 63:     require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address"); //@audit gas: this is redundant as only factory can init and always pass good result
64: 
65:     owner = _creator;
66:     maxTokenId = _maxTokenId;
67:   }

This is due to the fact that the initialize() methods have the onlyContractFactory modifier already, and that calls to initialize from the factory are not using address(0) (and hardly ever will in the future of the solution). See these initializations where the first argument is creator:

contracts/NFTCollectionFactory.sol:
  211:     INFTCollectionInitializer(_implementation).initialize(
  212        payable(address(rolesContract)),

  237:     INFTDropCollectionInitializer(_implementation).initialize(
  238        payable(address(this)),

  267:     INFTCollectionInitializer(collection).initialize(payable(msg.sender), name, symbol);

  399:     INFTDropCollectionInitializer(collection).initialize(
  400        payable(msg.sender),

Consider removing this check.

9. 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.

10. <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; ) {

11. ++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).

12. 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.

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