Open code423n4 opened 2 years ago
G01: custom error save more 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.
G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS
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
G03: PREFIX INCREMENT SAVE MORE GAS
Agree and will fix.
G04: USING BOOLS FOR STORAGE INCURS OVERHEAD
Valid for cidToMinted
, saving ~200 gas. Not seeing any benefit for assumePrimarySale
, potentially because it's an immutable variable.
G05: resign the default value to the variables.
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
G06: USE A MORE RECENT VERSION OF SOLIDITY
Invalid - we already on 0.8.16, the latest available.
G07:FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE
Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.
G08: ++I/I++ SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR- AND WHILE-LOOPS
4 of the 5 examples listed are already unchecked -- invalid. getFeesAndRecipients
is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.
gas optimization
G01: custom error save more gas
problem
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.
prof
libraries/AddressLibrary.sol, 31, require(contractAddress.isContract(), "InternalProxyCall: did not return 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, 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, 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: usereveal
instead"); 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");G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS
problem
0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706
prof
NFTDropCollection.sol, 130, require(bytes(_symbol).length > 0, "NFTDropCollection:
_symbol
must be set"); NFTDropCollection.sol, 131, require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId
must be set");G03: PREFIX INCREMENT SAVE MORE GAS
problem
prefix increment ++i is more cheaper than postfix i++
prof
NFTCollectionFactory.sol, 207, versionNFTCollection++; NFTCollectionFactory.sol, 231, versionNFTDropCollection++;
G04: USING BOOLS FOR STORAGE INCURS OVERHEAD
problem
// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
prof
NFTCollection.sol, 53, mapping(string => bool) private cidToMinted;
G05: resign the default value to the variables.
problem
resign the default value to the variables will cost more gas.
prof
libraries/BytesLibrary.sol, 25, for (uint256 i = 0; i < 20; ++i) { libraries/BytesLibrary.sol, 44, for (uint256 i = 0; i < 4; ++i) { 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) {
G06: USE A MORE RECENT VERSION OF SOLIDITY
G07:FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE
problem
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
prof
NFTCollectionFactory.sol, adminUpdateNFTCollectionImplementation,adminUpdateNFTDropCollectionImplementation NFTDropCollection.sol,burn,reveal,selfDestruct,updateMaxTokenId,updatePreRevealContent
G08: ++I/I++ SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR- AND WHILE-LOOPS
prof
libraries/BytesLibrary.sol, 25, for (uint256 i = 0; i < 20; ++i) { libraries/BytesLibrary.sol, 44, for (uint256 i = 0; i < 4; ++i) { 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) {