Foundation Drop contest - Rohan16's results

1.OPEN TODOS

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment There is 1 instance of this issue

// Link to githubfile

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L193

// actual codes used
contracts/mixins/shared/MarketFees.sol:193:      // TODO add referral info

---

2.USE _SAFEMINT() INSTEAD OF _MINT()

_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

Instances

//Links to github file https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L271 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L182

//actual code used 
contracts/NFTCollection.sol:271:      _mint(msg.sender, tokenId);
contracts/NFTDropCollection.sol:182:      _mint(to, i);

---

3.. USE OF FLOATING PRAGMA

Recommend using fixed solidity version.This is not a critical issue but this creates unsatbility to the program so having a fixed version reduces the unstability.

Instances

// Links to github file

https://github.com/code-423n4/2022-08-foundation All the conracts in scope are valid for this issue.

//actual code used 
contracts/NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
contracts/NFTCollection.sol:3:pragma solidity ^0.8.12;
contracts/NFTDropCollection.sol:3:pragma solidity ^0.8.12;
contracts/mixins/nftDropMarket/NFTDropMarketCore.sol:3:pragma solidity ^0.8.12;
contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:3:pragma solidity ^0.8.12;
contracts/mixins/roles/AdminRole.sol:3:pragma solidity ^0.8.12;
contracts/mixins/roles/MinterRole.sol:3:pragma solidity ^0.8.12;
contracts/mixins/collections/SequentialMintCollection.sol:3:pragma solidity ^0.8.12;
contracts/mixins/collections/CollectionRoyalties.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/Gap10000.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/ContractFactory.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/MarketFees.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/FoundationTreasuryNode.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/Constants.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/FETHNode.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/MarketSharedCore.sol:3:pragma solidity ^0.8.12;
contracts/FoundationTreasury.sol:48:pragma solidity ^0.8.12;
contracts/NFTDropMarket.sol:42:pragma solidity ^0.8.12;
contracts/FETH.sol:42:pragma solidity ^0.8.12;
contracts/libraries/AddressLibrary.sol:3:pragma solidity ^0.8.12;
contracts/libraries/BytesLibrary.sol:3:pragma solidity ^0.8.12;
contracts/libraries/ArrayLibrary.sol:3:pragma solidity ^0.8.12;

---

4.MOVE REQUIRE CHECK TO TOP OF FUNCTION

The require check in comes after several state changes. Consider moving it to the top of the function to follow the checks-effects-interactions pattern.

Instances

//Link to github file https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L262 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L158 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L238

//actual codes
contracts/NFTCollectionFactory.sol:262:    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
contracts/NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
contracts/NFTDropCollection.sol:238:    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

---

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

0 is less efficient than != 0 for unsigned integers (with proof) != 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 at 10k 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

Instances

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

---

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