Foundation Drop contest - durianSausage's results

none-critical issue

N01: NOT USING THE NAMED RETURN VARIABLES ANYWHERE IN THE FUNCTION IS CONFUSING

problem

Consider changing the variable to be an unnamed one

prof

AddressLibrary.sol, 36-37, named return variable is not used. FoundationTreasuryNode.sol, 59-61, treasuryAddress

N02: some function have annotations but some function without annotations

AddressLibrary.sol, callAndReturnContractAddress

N03: TYPOS

AdminRole.sol, 43, Checks -> Check

low risks

L01: address variable should check if it is zero

problem

MISSING CHECKS FOR ADDRESS(0X0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

prof

AdminRole.sol,28-30,

  function grantAdmin(address account) external {
    grantRole(DEFAULT_ADMIN_ROLE, account);
  }

L02:_SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE

problem

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

prof

NFTCollection.sol,130,143,159,271

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

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