Foundation Drop contest - __141345__'s results

EVENT IS MISSING INDEXED FIELDS

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

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

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

Redundant inheritance

Some contracts inherit from the base contracts and child contracts at the same time. For example

contract NFTDropMarket is
  Initializable,
  FoundationTreasuryNode,
  FETHNode,
  MarketSharedCore,
  NFTDropMarketCore,
  ReentrancyGuardUpgradeable,
  SendValueWithFallbackWithdraw,
  MarketFees,
  Gap10000,
  NFTDropMarketFixedPriceSale
{}

abstract contract NFTDropMarketFixedPriceSale is MarketFees {}

It can be seen that NFTDropMarket inherits from MarketFees and NFTDropMarketFixedPriceSale, however, NFTDropMarketFixedPriceSale also inherits from MarketFees, which makes the inheritance of NFTDropMarket from MarketFees redundant.

The same applies to many base and inherited sub-base contracts here, MarketSharedCore and FETHNode is another instance. And the NFTDropCollection, NFTCollection contracts and so on..., all have this issue.

The duplicate inheritance is prone to make confusion and lower the readability for future maintenance.

Another issue is the order of the multiple inheritance introduces ambiguity called Diamond Problem: if two or more base contracts define the same function, which one should be called in the child contract? Function shadowing could cause unexpected behavior. Refer to SWC-125.

Suggestion:

• Remove the duplicate inheritance
• Adjust the order of inheritance

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

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

Suggestion: Cache the length before the loop.

    uint length = names.length;

FOR-LOOP unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

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

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

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

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

The demo of the loop gas comparison can be seen here.

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas

contracts/mixins/shared/MarketFees.sol
134:        creatorRev += creatorShares[i];
150:        totalFees += buyReferrerFee;
199:        creatorRev += creatorShares[i];
490:        totalShares += creatorShares[i];
505:        totalRoyaltiesDistributed += royalty;
527:        totalFees -= buyReferrerFee;

EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO keccak256(),` SHOULD USE IMMUTABLE RATHER THAN CONSTANT

reference: https://github.com/ethereum/solidity/issues/9232

Constant expressions are left as expressions, not constants.

a constant declared as:

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol
70:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

contracts/mixins/roles/MinterRole.sol
19:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

contracts/mixins/treasury/OperatorRole.sol

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

consequences:

• each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)
• since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

Variable re-arrangement by storage packing

In DEFAULT_ADMIN_ROLE and ROYALTY_IN_BASIS_POINTS can be placed next to each other, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

contracts/mixins/shared/Constants.sol

Reference: Layout of State Variables in Storage.

REDUCE THE SIZE OF ERROR MESSAGES

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.

contracts/NFTCollection.sol
263-264:
    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
    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/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/NFTDropCollection.sol
88:
    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
93:
    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
130-131:
    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
    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/libraries/AddressLibrary.sol
31:
    require(contractAddress.isContract(), "InternalProxyCall: did not return 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-89:
    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

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/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/treasury/AdminRole.sol
20:
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

contracts/mixins/treasury/OZAccessControlUpgradeable.sol
137:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
152:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

Many of these require() statements are in a modifier which might be frequently called, optimizing the gas can be beneficial.

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) reference

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

The demo of the gas comparison can be seen here.

Many of these require() statements are in a modifier which might be frequently called, optimizing the gas can be beneficial.

Just like in contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol, the following files have multiple require() statements can use custom errors:

contracts/NFTCollection.sol
263-264:
    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
    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/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/NFTDropCollection.sol
88:
    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
93:
    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
130-131:
    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
    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/libraries/AddressLibrary.sol
31:
    require(contractAddress.isContract(), "InternalProxyCall: did not return 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-89:
    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

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/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/treasury/AdminRole.sol
20:
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

contracts/mixins/treasury/OZAccessControlUpgradeable.sol
137:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
152:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

DUPLICATED REQUIRE() CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTION

The following require() checks are the same, they can be made into a common modifier, and providing an error message as parameter.

The followings all check hasRole():

contracts/mixins/roles/AdminRole.sol
19:
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the 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/treasury/OZAccessControlUpgradeable.sol
137:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
152:
    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

The followings all check for isContract():

contracts/NFTCollectionFactory.sol
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");

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

contracts/mixins/shared/ContractFactory.sol
31:
    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

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.

contracts/NFTCollectionFactory.sol
202:  function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
226:  function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

contracts/NFTDropCollection.sol
159:  function burn(uint256 tokenId) public override onlyAdmin {
195:  function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {
209:  function selfDestruct() external onlyAdmin {
220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
232:  function updatePreRevealContent(string calldata _baseURI, bytes32 _postRevealBaseURIHash)
    external
    validBaseURI(_baseURI)
    onlyWhileUnrevealed
    onlyAdmin

contracts/mixins/treasury/CollateralManagement.sol
36:   function withdrawFunds(address payable to, uint256 amount) external onlyAdmin {

contracts/NFTCollection.sol
230:  function selfDestruct() external onlyCreator {
238:  function updateBaseURI(string calldata baseURIOverride) external onlyCreator {
251:  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {
262:  function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {

contracts/NFTDropCollection.sol
171:  function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {

contracts/NFTCollection.sol
105-109:
      function initialize() external initializer onlyContractFactory {

contracts/NFTDropCollection.sol
120-129:
      function initialize() external initializer onlyContractFactory 

Unused modifier

onlyFoundationAdmin() and onlyFoundationOperator() are never used across the contacts and can consider remove them.

contracts/mixins/shared/FoundationTreasuryNode.sol
  modifier onlyFoundationAdmin() {
    if (!IAdminRole(treasury).isAdmin(msg.sender)) {
      revert FoundationTreasuryNode_Caller_Not_Admin();
    }
    _;
  }

  modifier onlyFoundationOperator() {
    if (!IOperatorRole(treasury).isOperator(msg.sender)) {
      revert FoundationTreasuryNode_Caller_Not_Operator();
    }
    _;
  }

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata.

The demo of the gas comparison can be seen here.

contracts/NFTCollection.sol
282:  function baseURI() external view returns (string memory uri) {

contracts/NFTCollectionFactory.sol
284-288:
  function createNFTDropCollectionWithPaymentFactory(..., CallWithoutValue memory paymentAddressFactoryCall
  ) external returns () {