Open code423n4 opened 2 years ago
Custom errors
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.
Use != 0 instead of > 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
++i costs less than i++
Agree and will fix.
Cache Array Length Outside of Loop
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.
Use short error messages
Agree but won't fix. We use up to 64 bytes, aiming to respect the incremental cost but 32 bytes is a bit too short to provide descriptive error messages for our users.
1. Custom Errors instead of Revert Strings to save Gas
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
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.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Instances:
contracts/NFTDropCollection.sol
contracts/NFTDropCollection.sol:88: require(bytes(_baseURI).length > 0, "NFTDropCollection:
_baseURI
must be set"); contracts/NFTDropCollection.sol:93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed"); contracts/NFTDropCollection.sol:130: require(bytes(_symbol).length > 0, "NFTDropCollection:_symbol
must be set"); contracts/NFTDropCollection.sol:131: require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId
must be set"); contracts/NFTDropCollection.sol:172: require(count != 0, "NFTDropCollection:count
must be greater than 0"); contracts/NFTDropCollection.sol:179: require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); contracts/NFTDropCollection.sol:238: require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: usereveal
instead");contracts/mixins/collections/SequentialMintCollection.sol
contracts/mixins/collections/SequentialMintCollection.sol:58: require(msg.sender == owner, "SequentialMintCollection: Caller is not creator"); contracts/mixins/collections/SequentialMintCollection.sol:63: require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address"); contracts/mixins/collections/SequentialMintCollection.sol:74: require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first"); contracts/mixins/collections/SequentialMintCollection.sol:87: require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared"); contracts/mixins/collections/SequentialMintCollection.sol:88: require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase"); contracts/mixins/collections/SequentialMintCollection.sol:89: require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
contracts/NFTCollectionFactory.sol
contracts/NFTCollectionFactory.sol:173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); contracts/NFTCollectionFactory.sol:182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); contracts/NFTCollectionFactory.sol:203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); contracts/NFTCollectionFactory.sol:227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); contracts/NFTCollectionFactory.sol:262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
contracts/NFTCollection.sol
contracts/NFTCollection.sol:158: require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required"); contracts/NFTCollection.sol:263: require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); contracts/NFTCollection.sol:264: require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted"); contracts/NFTCollection.sol:268: require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted"); contracts/NFTCollection.sol:327: require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
contracts/mixins/shared/ContractFactory.sol
contracts/mixins/shared/ContractFactory.sol:22: require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory"); contracts/mixins/shared/ContractFactory.sol:31: require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
contracts/mixins/roles/AdminRole.sol contracts/mixins/roles/MinterRole.sol contracts/libraries/AddressLibrary.sol
contracts/mixins/roles/AdminRole.sol:19: require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role"); contracts/mixins/roles/MinterRole.sol:22: require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role"); contracts/libraries/AddressLibrary.sol:31: require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
References:
https://blog.soliditylang.org/2021/04/21/custom-errors/
Remediation:
I suggest replacing revert strings with custom errors.
2. !=0 instead of >0 for UINT in a
require()
statementInstances
contracts/NFTDropCollection.sol:88 contracts/NFTDropCollection.sol:130 contracts/NFTDropCollection.sol:131
contracts/NFTDropCollection.sol:88: require(bytes(_baseURI).length > 0, "NFTDropCollection:
_baseURI
must be set"); contracts/NFTDropCollection.sol:130: require(bytes(_symbol).length > 0, "NFTDropCollection:_symbol
must be set"); contracts/NFTDropCollection.sol:131: require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId
must be set");Reference:
https://twitter.com/gzeon/status/1485428085885640706
Remediation:
I suggest changing > 0 with != 0. Also, please enable the Optimizer.
3. Preincrement costs less gas as compared to Postincrement :
++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)
i++ increments i and returns initial value of i. Which means
uint i = 1; i++; // ==1 but i ==2
But ++i returns the actual incremented value:
uint i = 1; ++i; // ==2 and i ==2 , no need for temporary variable here
In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.
Instances:
contracts/NFTCollectionFactory.sol:207 contracts/NFTCollectionFactory.sol:231
contracts/NFTCollectionFactory.sol:207: versionNFTCollection++; contracts/NFTCollectionFactory.sol:231: versionNFTDropCollection++;
4. An array’s length should be cached to save gas in for-loops
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.
Instances:
Links: MarketFees.sol: L126 L198 L484 L503
contracts/mixins/shared/MarketFees.sol:126: for (uint256 i = 0; i < creatorRecipients.length; ++i) { contracts/mixins/shared/MarketFees.sol:198: for (uint256 i = 0; i < creatorShares.length; ++i) { contracts/mixins/shared/MarketFees.sol:484: for (uint256 i = 0; i < creatorRecipients.length; ++i) { contracts/mixins/shared/MarketFees.sol:503: for (uint256 i = 1; i < creatorRecipients.length; ) {
Remediation:
Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead.
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.
Instances:
contracts/NFTDropCollection.sol
contracts/NFTDropCollection.sol:88: require(bytes(_baseURI).length > 0, "NFTDropCollection:
_baseURI
must be set"); contracts/NFTDropCollection.sol:93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed"); contracts/NFTDropCollection.sol:130: require(bytes(_symbol).length > 0, "NFTDropCollection:_symbol
must be set"); contracts/NFTDropCollection.sol:131: require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId
must be set"); contracts/NFTDropCollection.sol:172: require(count != 0, "NFTDropCollection:count
must be greater than 0"); contracts/NFTDropCollection.sol:179: require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); contracts/NFTDropCollection.sol:238: require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: usereveal
instead");contracts/mixins/collections/SequentialMintCollection.sol
contracts/mixins/collections/SequentialMintCollection.sol:58: require(msg.sender == owner, "SequentialMintCollection: Caller is not creator"); contracts/mixins/collections/SequentialMintCollection.sol:63: require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address"); contracts/mixins/collections/SequentialMintCollection.sol:74: require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first"); contracts/mixins/collections/SequentialMintCollection.sol:87: require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared"); contracts/mixins/collections/SequentialMintCollection.sol:88: require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase"); contracts/mixins/collections/SequentialMintCollection.sol:89: require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
contracts/NFTCollectionFactory.sol
contracts/NFTCollectionFactory.sol:173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); contracts/NFTCollectionFactory.sol:182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); contracts/NFTCollectionFactory.sol:203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); contracts/NFTCollectionFactory.sol:227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); contracts/NFTCollectionFactory.sol:262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
contracts/NFTCollection.sol
contracts/NFTCollection.sol:158: require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required"); contracts/NFTCollection.sol:263: require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); contracts/NFTCollection.sol:264: require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted"); contracts/NFTCollection.sol:268: require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted"); contracts/NFTCollection.sol:327: require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
contracts/mixins/shared/ContractFactory.sol
contracts/mixins/shared/ContractFactory.sol:22: require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory"); contracts/mixins/shared/ContractFactory.sol:31: require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
contracts/mixins/roles/AdminRole.sol contracts/mixins/roles/MinterRole.sol contracts/libraries/AddressLibrary.sol
contracts/mixins/roles/AdminRole.sol:19: require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role"); contracts/mixins/roles/MinterRole.sol:22: require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role"); contracts/libraries/AddressLibrary.sol:31: require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.**
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source Custom Errors in Solidity: