Foundation Drop contest - TomJ's results

Table of Contents

Non-critical Issues

• Use fixed compiler versions instead of floating version
• Event is Missing Indexed Fields
• Unnecessary Use of Named Returns
• Wrong NatSpec

 

Non-critical Issues

Use fixed compiler versions instead of floating version

Issue

it is best practice to lock your pragma instead of using floating pragma. the use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

./FoundationTreasuryNode.sol:3:pragma solidity ^0.8.12;
./NFTDropMarket.sol:42:pragma solidity ^0.8.12;
./NFTDropMarketFixedPriceSale.sol:3:pragma solidity ^0.8.12;
./SequentialMintCollection.sol:3:pragma solidity ^0.8.12;
./NFTDropCollection.sol:3:pragma solidity ^0.8.12;
./MarketSharedCore.sol:3:pragma solidity ^0.8.12;
./NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
./AdminRole.sol:3:pragma solidity ^0.8.12;
./MarketFees.sol:3:pragma solidity ^0.8.12;
./ContractFactory.sol:3:pragma solidity ^0.8.12;
./Gap10000.sol:3:pragma solidity ^0.8.12;
./Constants.sol:3:pragma solidity ^0.8.12;
./CollectionRoyalties.sol:3:pragma solidity ^0.8.12;
./FETHNode.sol:3:pragma solidity ^0.8.12;
./MinterRole.sol:3:pragma solidity ^0.8.12;
./NFTDropMarketCore.sol:3:pragma solidity ^0.8.12;
./NFTCollection.sol:3:pragma solidity ^0.8.12;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./NFTDropMarketFixedPriceSale.sol:79:  event CreateFixedPriceSale(address indexed nftContract, address indexed seller, uint256 price, uint256 limitPerAccount);
./NFTDropCollection.sol:85:  event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);
./MarketFees.sol:71:  event BuyReferralPaid(address indexed nftContract, uint256 indexed tokenId, address buyReferrer, uint256 buyReferrerFee, uint256 buyReferrerSellerFee);
./NFTCollection.sol:70:  event BaseURIUpdated(string baseURI);

Mitigation

Add up to 3 indexed fields when possible.

 

Unnecessary Use of Named Returns

Issue

Several function adds return statement even thought named returns variable are used. Remove unnecessary named returns variable to improve code readability. Also keeping the use of named returns or return statement consistent through out the whole project if possible is recommended.

PoC

Total of 4 issues found.

1. tokenURI() of NFTDropCollection.sol Remove returns variable "uri" https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L300-L304

2. createNFTDropCollection() of NFTCollectionFactory.sol Remove returns variable "collection" https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L294-L295

3. createNFTDropCollectionWithPaymentAddress() of NFTCollectionFactory.sol Remove returns variable "collection" https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L333-L334

4. createNFTDropCollectionWithPaymentFactory() of NFTCollectionFactory.sol Remove returns variable "collection" https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L372-L373

Mitigation

Remove unused named returns variable and keep the use of named returns or return statement consistent through out the whole project if possible.

 

Wrong NatSpec

Issue

There is mistake in following NatSpec.

PoC

1. It is supposed to be "ERC-1167" not "ERC-1165"

60: * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a NFT collection contract implementation.

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L60

 

Table of Contents

• Use Function Parameter Instead of State Variable
• Duplicate require() Checks Should be a Modifier or a Function
• Internal Function Called Only Once can be Inlined
• Constant Value of a Call to keccak256() should Use Immutable
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Unnecessary Default Value Initialization
• Store Array's Length as a Variable
• != 0 costs less gass than > 0
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

Use Function Parameter Instead of State Variable

Issue

Using function parameter value instead of SLOAD state variable saves gas.

PoC

Total of 2 issues found.

1. Use parameter _baseURI of updatePreRevealContent() of NFTDropCollection.sol Change line 242 from state variable of baseURI to parameter of _baseURI. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L242

2. Use parameter _postRevealBaseURIHash of updatePreRevealContent() of NFTDropCollection.sol Change line 242 from state variable of postRevealBaseURIHash to parameter of _postRevealBaseURIHash. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L242

Mitigation

Change state variable to parameter variable as listed in above PoC.

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

Total of 1 issue found.

./NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
./NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 3 issues found.

1. _initializeAdminRole() of AdminRole.sol Function called only once at line 148 of NFTDropCollection.sol https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/AdminRole.sol#L13 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L148

2. _tryUseFETHBalance() of FETHNode.sol Function called only once at line 204 of NFTDropMarketFixedPriceSale.sol https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FETHNode.sol#L46 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L204

3. _initializeMinterRole() of MinterRole.sol Function called only once at line 150 of NFTDropCollection.sol https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/MinterRole.sol#L26 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L150

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this constant. Link for more details: https://github.com/ethereum/solidity/issues/9232

PoC

Total of 2 issues found.

./NFTDropMarketFixedPriceSale.sol:70:  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
./MinterRole.sol:19:  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

Mitigation

Change the variable from constant to immutable.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 13 issues found.

./NFTDropMarketFixedPriceSale.sol:120:    uint80 price,
./NFTDropMarketFixedPriceSale.sol:121:    uint16 limitPerAccount
./NFTDropMarketFixedPriceSale.sol:172:    uint16 count,
./SequentialMintCollection.sol:62:  function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
./SequentialMintCollection.sol:86:  function _updateMaxTokenId(uint32 _maxTokenId) internal {
./NFTDropCollection.sol:126:    uint32 _maxTokenId,
./NFTDropCollection.sol:220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
./NFTCollectionFactory.sol:192:  function initialize(uint32 _versionNFTCollection) external initializer {
./NFTCollectionFactory.sol:291:    uint32 maxTokenId,
./NFTCollectionFactory.sol:329:    uint32 maxTokenId,
./NFTCollectionFactory.sol:368:    uint32 maxTokenId,
./NFTCollectionFactory.sol:391:    uint32 maxTokenId,
./NFTCollection.sol:251:  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

Mitigation

I suggest using uint256 instead of anything smaller or downcast where needed.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 5 issues found.

./MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
./MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./BytesLibrary.sol:25:      for (uint256 i = 0; i < 20; ++i) {
./BytesLibrary.sol:44:      for (uint256 i = 0; i < 4; ++i) {

Mitigation

I suggest removing default value initialization. For example,

• for (uint256 i; i < creatorRecipients.length; ++i) {

 

Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

Total of 4 issues found.

./MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
./MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {
./MarketFees.sol:503:      for (uint256 i = 1; i < creatorRecipients.length; ) {

Mitigation

Store array's length as a variable before looping it.

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

Total of 3 issues found.

./NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
./NFTDropCollection.sol:130:    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
./NFTDropCollection.sol:131:    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

Mitigation

I suggest changing > 0 to != 0

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

Total of 28 issues found.

./SequentialMintCollection.sol:58:    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
./SequentialMintCollection.sol:63:    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
./SequentialMintCollection.sol:74:    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
./SequentialMintCollection.sol:87:    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
./SequentialMintCollection.sol:88:    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
./SequentialMintCollection.sol:89:    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
./NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
./NFTDropCollection.sol:93:    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
./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");
./NFTCollectionFactory.sol:173:    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
./NFTCollectionFactory.sol:182:    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
./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");
./AdminRole.sol:19:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
./ContractFactory.sol:22:    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
./ContractFactory.sol:31:    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
./AddressLibrary.sol:31:    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
./MinterRole.sol:22:    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
./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");

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

Total of 28 issues found.

./SequentialMintCollection.sol:58:    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
./SequentialMintCollection.sol:63:    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
./SequentialMintCollection.sol:74:    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
./SequentialMintCollection.sol:87:    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
./SequentialMintCollection.sol:88:    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
./SequentialMintCollection.sol:89:    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
./NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
./NFTDropCollection.sol:93:    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
./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");
./NFTCollectionFactory.sol:173:    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
./NFTCollectionFactory.sol:182:    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
./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");
./AdminRole.sol:19:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
./ContractFactory.sol:22:    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
./ContractFactory.sol:31:    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
./AddressLibrary.sol:31:    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
./MinterRole.sol:22:    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
./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");

Mitigation

I suggest implementing custom errors to save gas.

 

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