Foundation Drop contest - DevABDee's results

1. Floating pragmas are considered a bad practice.

Almost all the contracts have floating Pragmas, which is considered, a bad practice. It is always recommended that pragma should be fixed (not floating) to the version that you are intending to deploy your contracts with.

For more information Check this out

So Instead of this:

pragma solidity ^0.8.12;

Use this:

pragma solidity 0.8.12;

in all the contracts.

2. Unused & Uneccassary Imports

contracts/NFTDropCollection.sol:20: import "./mixins/shared/Constants.sol";

contracts/NFTDropMarket.sol:47: import "./mixins/shared/Constants.sol";

3. Some events are not defined as Indexed, where they can be.

The indexed keyword helps you to filter the logs to find the wanted data. thus you can search for specific items instead of getting all the logs. So in some contracts, the events can be described as indexed but they are not. See:

contracts/NFTDropCollection.sol:85: event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);

contracts/NFTCollection.sol:70: event BaseURIUpdated(string baseURI);

contracts/mixins/shared/MarketFees.sol:71: event BuyReferralPaid(     // 1 more can be marked as `indexed` here
    address indexed nftContract,
    uint256 indexed tokenId,
    address buyReferrer,
    uint256 buyReferrerFee,
    uint256 buyReferrerSellerFee
  );

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol75: event CreateFixedPriceSale(    // 1 more can be marked as `indexed` here
    address indexed nftContract,
    address indexed seller,
    uint256 price,
    uint256 limitPerAccount
  );

1. Use Custom Errors instead of require statements.

Some contracts use many require statements. It makes deployment and function calling very gas expensive. As error messages stores full-length strings. So Replace those require statements with the custom errors. Check this Post for more info.

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/NFTDropCollection.sol:088: require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
contracts/NFTDropCollection.sol:093: 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/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");

2. Pack non-uint256 variables together

As we know that in the EVM, a slot is of 32bytes (256bits). So when we define a uint256 it takes the full slot. But when we have a uints that are not uint256 like uint8 & unit32 then the EVM merges them in one slot and gas gets optimized. But for that we have to make sure that both the non-uint256s are defined together. So as in the `` we have two uint32 defined, but they are not defined together, sowe cannot take the advantage of EVM's feature. So from this:

 address public implementationNFTCollection;

  /**
   * @notice The implementation version of new NFTCollections.
   * @dev This is auto-incremented each time `implementationNFTCollection` is changed.
   * @return The current NFTCollection implementation version.
   */
  uint32 public versionNFTCollection;

  /****** Slot 1 ******/
  /**
   * @notice The address of the implementation all new NFTDropCollections will leverage.
   * @dev When this is changed, `versionNFTDropCollection` is incremented.
   * @return The implementation address for NFTDropCollection.
   */
  address public implementationNFTDropCollection;

  /**
   * @notice The implementation version of new NFTDropCollections.
   * @dev This is auto-incremented each time `implementationNFTDropCollection` is changed.
   * @return The current NFTDropCollection implementation version.
   */
  uint32 public versionNFTDropCollection;

  /****** End of storage ******/

Change it into this:

 address public implementationNFTCollection;

  /****** Slot 1 ******/
  /**
   * @notice The address of the implementation all new NFTDropCollections will leverage.
   * @dev When this is changed, `versionNFTDropCollection` is incremented.
   * @return The implementation address for NFTDropCollection.
   */
  address public implementationNFTDropCollection;

  /**
   * @notice The implementation version of new NFTCollections.
   * @dev This is auto-incremented each time `implementationNFTCollection` is changed.
   * @return The current NFTCollection implementation version.
   */
  uint32 public versionNFTCollection;

  /**
   * @notice The implementation version of new NFTDropCollections.
   * @dev This is auto-incremented each time `implementationNFTDropCollection` is changed.
   * @return The current NFTDropCollection implementation version.
   */
  uint32 public versionNFTDropCollection;

  /****** End of storage ******/

3. Use uint256 instead of uint32

replace units lower than uint256 like uint32 with uint256, which will cost less gas

contracts/NFTCollectionFactory.sol:80: uint32 public versionNFTCollection;
contracts/NFTCollectionFactory.sol95: uint32 public versionNFTDropCollection;

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:54: uint80 price;
contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:58: uint16 limitPerAccount; 

4. Pre-increments costs unnecessary gas

For more info check this post Change these:

- contracts/NFTCollectionFactory.sol:207: versionNFTCollection++;
+ contracts/NFTCollectionFactory.sol:207: ++versionNFTCollection;

- contracts/NFTCollectionFactory.sol:231: versionNFTDropCollection++;
+ contracts/NFTCollectionFactory.sol:231: ++versionNFTDropCollection;

5. latestTokenId should be cached into memory. Then used inside a Loop.

In contracts/NFTDropCollection.sol there is a loop, which access latestTokenId again and again from the storage.

line 181:     for (uint256 i = firstTokenId; i <= latestTokenId; ) {

Inside the function, before the loop, define `_latestTokenId = latestTokenId; and then use that inside the loop. Something like this:

line 181:     for (uint256 i = firstTokenId; i <= _latestTokenId; ) {

6. Unused & Uneccassary Imports

contracts/NFTDropCollection.sol:20: import "./mixins/shared/Constants.sol";

contracts/NFTDropMarket.sol:47: import "./mixins/shared/Constants.sol";