Gas Optimizations

[code423n4]

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: use reveal 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) {

[HardlyDifficult]

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.