Foundation Drop contest - MiloTruck's results

Low Issues

Total: 1 instance over 1 issue

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

As seen from the Solidity Docs:

If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c").

If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred.

Compared to abi.encodePacked(), abi.encode() pads all items to 32 bytes, which helps to prevent hash collisions. Additionally, if there is only one argument to abi.encodePacked(), it can be cast to bytes() or bytes32() instead.

There is 1 instance of this issue:

contracts/NFTCollectionFactory.sol:
 449:        return keccak256(abi.encodePacked(creator, nonce));

Non-Critical Issues

Total: 6 instances over 2 issues

Floating compiler versions

Non-library/interface files should use fixed compiler versions, not floating ones.

There are 4 instances of this issue:

contracts/NFTCollectionFactory.sol:
  42:        pragma solidity ^0.8.12;

contracts/NFTDropMarket.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;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields.

There are 2 instances of this issue:

contracts/mixins/shared/MarketFees.sol:
  71:        event BuyReferralPaid(
  72:          address indexed nftContract,
  73:          uint256 indexed tokenId,
  74:          address buyReferrer,
  75:          uint256 buyReferrerFee,
  76:          uint256 buyReferrerSellerFee
  77:        );

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:
  79:        event CreateFixedPriceSale(
  80:          address indexed nftContract,
  81:          address indexed seller,
  82:          uint256 price,
  83:          uint256 limitPerAccount
  84:        );

Gas Report

Total: 121 instances over 13 issues

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be written without explicitly setting the index to 0:

for (uint256 i; i < length; ++i) {

There are 5 instances of this issue:

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

contracts/libraries/BytesLibrary.sol:
  25:        for (uint256 i = 0; i < 20; ++i) {
  44:        for (uint256 i = 0; i < 4; ++i) {

For-Loops: Cache array length outside of loops

Reading an array's length at each iteration has the following gas overheads:

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use mload (3 gas)
• calldata arrays use calldataload (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset. This would save around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

There are 4 instances of this issue:

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

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, index increments can be left unchecked to save 30-40 gas per loop iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

There are 5 instances of this issue:

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

contracts/libraries/BytesLibrary.sol:
  25:        for (uint256 i = 0; i < 20; ++i) {
  44:        for (uint256 i = 0; i < 4; ++i) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the 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 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

There are 2 instances of this issue:

contracts/NFTCollectionFactory.sol:
 207:        versionNFTCollection++;
 231:        versionNFTDropCollection++;

Arithmetics: Use != 0 instead of > 0 for unsigned integers in require statements

A variable of type uint will never go below 0. Thus, checking != 0 rather than > 0 is sufficient, which would save 6 gas per instance.

There are 3 instances of this issue:

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

Errors: Reduce the length of error messages (long revert strings)

As seen here, revert strings longer than 32 bytes would incur an additional mstore (3 gas) for each extra 32-byte chunk. Furthermore, there are additional runtime costs for memory expansion and stack operations when the revert condition is met.

Thus, shortening revert strings to fit within 32 bytes would help to reduce runtime gas costs when the revert condition is met.

There are 28 instances of this issue:

contracts/NFTCollectionFactory.sol:
 173:        require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
 182:        require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
 203:        require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
 227:        require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
 262:        require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

contracts/NFTCollection.sol:
 158:        require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
 263:        require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
 264:        require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
 268:        require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
 327:        require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

contracts/NFTDropCollection.sol:
  88:        require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
  93:        require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
 130:        require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
 131:        require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
 172:        require(count != 0, "NFTDropCollection: `count` must be greater than 0");
 179:        require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
 238:        require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

contracts/mixins/roles/MinterRole.sol:
  22:        require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

contracts/mixins/treasury/AdminRole.sol:
  20:        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

contracts/mixins/shared/ContractFactory.sol:
  22:        require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
  31:        require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

contracts/mixins/collections/SequentialMintCollection.sol:
  58:        require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
  63:        require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
  74:        require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
  87:        require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
  88:        require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
  89:        require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

contracts/libraries/AddressLibrary.sol:
  31:        require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

Duplicated require() checks should be refactored to a modifier or function

If a require() statement that is used to validate function parameters or global variables is present across multiple functions in a contract, it should be rewritten into modifier if possible. This would help to reduce code deployment size, which saves gas, and improve code readability.

There are 2 instances of this issue:

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

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors can be used rather than require statements.

Taken from Custom Errors in Solidity:

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 reduce runtime gas costs as they save about 50 gas each time they are hit by avoiding having to allocate and store the revert string. Furthermore, not definiting error strings also help to reduce deployment gas costs.

There are 28 instances of this issue:

contracts/NFTCollectionFactory.sol:
 173:        require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
 182:        require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
 203:        require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
 227:        require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
 262:        require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

contracts/NFTCollection.sol:
 158:        require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
 263:        require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
 264:        require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
 268:        require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
 327:        require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

contracts/NFTDropCollection.sol:
  88:        require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
  93:        require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
 130:        require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
 131:        require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
 172:        require(count != 0, "NFTDropCollection: `count` must be greater than 0");
 179:        require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
 238:        require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

contracts/mixins/roles/MinterRole.sol:
  22:        require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

contracts/mixins/treasury/AdminRole.sol:
  20:        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

contracts/mixins/shared/ContractFactory.sol:
  22:        require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
  31:        require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

contracts/mixins/collections/SequentialMintCollection.sol:
  58:        require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
  63:        require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
  74:        require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
  87:        require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
  88:        require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
  89:        require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

contracts/libraries/AddressLibrary.sol:
  31:        require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

State variables should be cached in stack variables rather than re-reading them from storage

If a state variable is read from storage multiple times in a function, it should be cached in a stack variable.

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 4 instances of this issue:
maxTokenId in _mint():

contracts/NFTCollection.sol:
 268:        require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

baseURI_ in _baseURI():

contracts/NFTCollection.sol:
 333:        if (bytes(baseURI_).length != 0) {
 334:        return baseURI_;

latestTokenId in mintCountTo():

contracts/NFTDropCollection.sol:
 176:        firstTokenId = latestTokenId + 1;
 178:        latestTokenId = latestTokenId + count;
 179:        require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
 181:        for (uint256 i = firstTokenId; i <= latestTokenId; ) {

maxTokenId in _updateMaxTokenId():

contracts/mixins/collections/SequentialMintCollection.sol:
  88:        require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

Using bool for storage incurs overhead

Declaring storage variables as bool is more expensive compared to uint256, as explained here:

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.

Use uint256(1) and uint256(2) rather than true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past.

There are 2 instances of this issue:

contracts/NFTCollection.sol:
  53:        mapping(string => bool) private cidToMinted;

contracts/mixins/shared/MarketFees.sol:
  61:        bool private immutable assumePrimarySale;

Use calldata instead of memory for read-only arguments in external functions

When an external function with a memory array is called, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length).

Using calldata directly instead of memory helps to save gas as values are read directly from calldata using calldataload, thus removing the need for such a loop in the contract code during runtime execution.

Also, structs have the same overhead as an array of length one.

There are 3 instances of this issue:

contracts/NFTCollectionFactory.sol:
 371:        CallWithoutValue memory paymentAddressFactoryCall

contracts/NFTCollection.sol:
 107:        string memory _name,
 108:        string memory _symbol

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

As seen from here:

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

However, this does not apply to storage values as using reduced-size types might be beneficial to pack multiple elements into a single storage slot. Thus, where appropriate, use uint256/int256 and downcast when needed.

There are 16 instances of this issue:

contracts/NFTCollectionFactory.sol:
 192:        function initialize(uint32 _versionNFTCollection) external initializer {
 291:        uint32 maxTokenId,
 329:        uint32 maxTokenId,
 368:        uint32 maxTokenId,
 391:        uint32 maxTokenId,

contracts/NFTCollection.sol:
 251:        function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

contracts/NFTDropCollection.sol:
 126:        uint32 _maxTokenId,
 171:        function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
 220:        function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

contracts/mixins/collections/SequentialMintCollection.sol:
  62:        function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
  86:        function _updateMaxTokenId(uint32 _maxTokenId) internal {

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:
  54:        uint80 price;
  58:        uint16 limitPerAccount;
 120:        uint80 price,
 121:        uint16 limitPerAccount
 172:        uint16 count,

Not using the named return variables when a function returns, wastes deployment gas

There are 19 instances of this issue:

contracts/NFTCollectionFactory.sol:
 295:        return
 296:          _createNFTDropCollection(
 297:            name,
 298:            symbol,
 299:            baseURI,
 300:            postRevealBaseURIHash,
 301:            maxTokenId,
 302:            approvedMinter,
 303:            payable(0),
 304:            nonce
 305:          );


 334:        return
 335:          _createNFTDropCollection(
 336:            name,
 337:            symbol,
 338:            baseURI,
 339:            postRevealBaseURIHash,
 340:            maxTokenId,
 341:            approvedMinter,
 342:            paymentAddress != msg.sender ? paymentAddress : payable(0),
 343:            nonce
 344:          );


 373:        return
 374:          _createNFTDropCollection(
 375:            name,
 376:            symbol,
 377:            baseURI,
 378:            postRevealBaseURIHash,
 379:            maxTokenId,
 380:            approvedMinter,
 381:            AddressLibrary.callAndReturnContractAddress(paymentAddressFactoryCall),
 382:            nonce
 383:          );

contracts/NFTDropMarket.sol:
 118:        return payable(address(0));
 125:        return super._getSellerOf(nftContract, tokenId);
 149:        return super._getSellerOf(nftContract, tokenId);

contracts/NFTDropCollection.sol:
 303:        return string.concat(baseURI, tokenId.toString(), ".json");

contracts/mixins/shared/MarketFees.sol:
 209:        return address(royaltyRegistry);
 264:        return (_recipients, recipientBasisPoints);
 321:        return (_recipients, recipientBasisPoints);
 344:        return (_recipients, recipientBasisPoints);

contracts/mixins/shared/MarketSharedCore.sol:
  21:        return _getSellerOf(nftContract, tokenId);

contracts/mixins/shared/FoundationTreasuryNode.sol:
  60:        return treasury;

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:
 237:        return 0;
 242:        return 0;
 248:        return numberOfTokensAvailableToMint;
 251:        return availableToMint;
 281:        return (payable(0), 0, 0, 0, false);

contracts/libraries/AddressLibrary.sol:
  38:        return callAndReturnContractAddress(call.target, call.callData);

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