Foundation Drop contest - Saw-mon_and_Natalie's results

Table of Contents

1. CollectionRoyalties.sol
2. ICollectionFactory.sol
3. ContractFactory.sol
4. NFTCollection.sol
5. NFTCollectionFactory.sol
6. NFTDropCollection.sol
7. NFTDropMarketFixedPriceSale.sol
8. FETH.sol (Out of Scope)
9. Line Width
10. Hard-coded gas limits
11. address.isContract check
12. Simplify supportsInterface check
13. Floating Solidity Pragma Version
14. Avoid Nested if Blocks

1. contracts/mixins/collections/CollectionRoyalties.sol <a id="CollectionRoyalties"></a>

On line 80, supportsInterface can be rewritten to avoid the if/esle branching:

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool interfaceSupported) {
	return (
		interfaceId == type(IRoyaltyInfo).interfaceId ||
		interfaceId == type(ITokenCreator).interfaceId ||
		interfaceId == type(IGetRoyalties).interfaceId ||
		interfaceId == type(IGetFees).interfaceId ||
		super.supportsInterface(interfaceId)
	);
}

2. contracts/interfaces/ICollectionFactory.sol <a id="ICollectionFactory"></a>

In ICollectionFactory on line 6, IProxyCall is never used and can safely be removed. Unless there is a plan to use it in the future. Maybe a comment explaining why it was imported here would be helpful.

3. contracts/mixins/shared/ContractFactory.sol <a id="ContractFactory"></a>

On line 31, there is a check for _contractFactory to see if it already has a code. I guess this is an extra check that can be removed. Since if _contractFactory calls the constructor here in its own constructor by then _contractFactory.isContract() = _contractFactory.code.length == 0. Also, it is possible that a wrong contract address is passed here, so the check would not really do anything. This will only check against accidental EOA addresses used for _contractFactory. So we could possibly remove the following lines:

 5	import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
13 	using AddressUpgradeable for address;
31	require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

If there is a stricter condition for the allowed contractFactory addresses, maybe we could use that instead. One possible idea is an array of implementation contract code hashes that we could check. Or maybe contracts that have a function similar to supportsInterface that returns a magic number which we could check here.

4. contracts/NFTCollection.sol <a id="NFTCollection"></a>

4.1 Shorter inheritance list

The inheritance contracts on line 29-40 can be consolidated into a shorter list:

contract NFTCollection is
  INFTCollectionInitializer,
  ContractFactory,
  SequentialMintCollection,
  CollectionRoyalties
{

Then you would need to adjust the overrides on lines 255 and 316

4.2 CID need to be unique per tokenID

Different tokenIDs can not share the same CID by design. Although it is possible to design the contract so that some tokens share the same CID to save storage and also server space for off-chain contents.

5. contracts/NFTCollectionFactory.sol <a id="NFTCollectionFactory"></a>

5.1 .isContract()

On lines 182, and 203 instead of checking if addr.isContract() to avoid setting the addresses to EOA by mistake it would be best to pass the code hash instead and check the code hash at those addresses. So for example:

Before:

constructor(address _rolesContract) {
	require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); 

	rolesContract = IRoles(_rolesContract);
}

After:

constructor(address _rolesContract, bytes32 codehash) {
	require(_rolesContract.codehash == codehash, "NFTCollectionFactory: RolesContract is not a contract");

	rolesContract = IRoles(_rolesContract);
}

This is a stronger requirement since it would guarantee that the addresses are contracts and also they have the required code hash. For the functions to pass the require statements you would need to make 2 mistakes, one for the address and the other for the code hash. The probability of making this mistake should be theoretically lower than just passing a wrong address.

5.2 versionNFTDropCollection

Doesn't have an initializer like versionNFTCollection

5.3 a better name can be chosen for rolesContract

rolerManager might be a better name for this immutable variable and would make it easier to remember what it does (ref. line 104).

6. contracts/NFTDropCollection.sol <a id="NFTDropCollection"></a>

6.1 supportsInterface function

We can rewrite supportsInterface function (Lines 284-294) like the following block which would make it easier to read and possibly would save some gas.

function supportsInterface(bytes4 interfaceId)
	public
	view
	override(ERC165Upgradeable, AccessControlUpgradeable, ERC721Upgradeable, CollectionRoyalties)
	returns (bool)
{
	return (
		interfaceId == type(INFTDropCollectionMint).interfaceId ||
		super.supportsInterface(interfaceId)
	);
}

6.2 The comment on line 175 needs a bit of correction

So the current comment says:

If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits

But count can not be zero if we have reached this line. Since we have already checked for a non-zero count on line 172

So we can change the comment to

// If +1 overflows then +count would also overflow, since count > 0.

6.3 Shorter inheritance list

Like NFTCollection, the inheritence list for NFTDropCollection contract on lines 28-46 can be consolidated more.

contract NFTDropCollection is
  INFTDropCollectionInitializer,
  INFTDropCollectionMint,
  ContractFactory,
  MinterRole,
  SequentialMintCollection,
  CollectionRoyalties
{

The overrides on lines 245 and 287 would also need to be modified accordingly.

7. contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol <a id="NFTDropMarketFixedPriceSale"></a>

In mintFromFixedPriceSale we can avoid the nested if blocks on lines 182-189. This would improve readability and analyze and it would have the same effect. On the plus side, it will also save gas for a reverting call where saleConfig.limitPerAccount is zero by avoiding the outer if block in the original code.

// Confirm that the collection has a sale in progress.
if (saleConfig.limitPerAccount == 0) {
	revert NFTDropMarketFixedPriceSale_Must_Have_Sale_In_Progress();
}
// Confirm that the buyer will not exceed the limit specified after minting.
if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {	
	revert NFTDropMarketFixedPriceSale_Cannot_Buy_More_Than_Limit(saleConfig.limitPerAccount);
}

contracts/FETH.sol <a id="FETH"></a> (Out of Scope)

In constructor instead of passing _lockupDuration pass _lockupInterval to save on the exact division check.

So taking that into consideration the constructor would look like this:

constructor(
	address payable _foundationMarket,
	address payable _foundationDropMarket,
	uint256 _lockupInterval
) {
	if (!_foundationMarket.isContract()) {
		revert FETH_Market_Must_Be_A_Contract();
	}
	if (!_foundationDropMarket.isContract()) {
		revert FETH_Market_Must_Be_A_Contract();
	}
	if (_lockupInterval == 0) {
		revert FETH_Invalid_Lockup_Duration();
	}

	foundationMarket = _foundationMarket;
	foundationDropMarket = _foundationDropMarket;
	lockupInterval = _lockupInterval; 
	lockupDuration = _lockupInterval * 24;
}

Also, the _lockupInterval check is moved up before the assignments to save gas in case of a revert. If there will be no revert, moving up the if block would not introduce any gas changes, since the check will be performed eventually.

9. Line Width <a id="line-width"></a>

Keep line width to max 120 characters for better readability.

10. Hard-coded gas limits <a id="gas-limit"></a>

In contracts/mixins/shared/Constants.sol we have 3 gas limit constants:

uint256 constant READ_ONLY_GAS_LIMIT = 40000;
uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;
uint256 constant SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20000;

These numbers are not future-proof as some hardforks introduce changes to gas costs. These potential future changes to gas costs might break some of the functionalities of the smart contracts that use these constants. This is something to keep in mind. If some hardfork, would break a smart contract using these numbers you would need to deploy new contracts with adjusted gas limit constants. Or you can also have these gas limits be mutable by admins on-chain. For example, all these 3 values can be stored on-chain in 1 storage slot.

11. address.isContract check <a id="is-contract"></a>

Lots of the contracts in this project import import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol" and use address.isContract() to check if an address is a contranct and not a EOA. I guess this is only a check if the deployer by mistake provides the wrong address. I think this should be double-checked off-chain. If an on-chain check is needed, there are other checks that can be done that are even more strict than just checking against EOA mistakes. For example, we can provide the contract has as the second input to the constructor and check the address's codehash against that. Here is a template as an example:

constructor(address c, bytes32 h) {
	if( c.codehash != h) {
		revert CustomError();
	}
}

Not only this checks for address with code, but also pinpoints the contract hash to a specific hash. Another type of check that can be used is to check if the provided contract address supports a specific interfaceSupport or call an endpoint of the contract expecting it to return a specific magic number.

Here is a list of places isContract has been used:

1. FETH.sol - L201
2. FETH.sol - L204
3. NFTCollectionFactory.sol - L182
4. NFTCollectionFactory.sol - L203
5. NFTCollectionFactory.sol - L227
6. PercentSplitETH.sol - L171
7. AddressLibrary.sol - L31
8. ContractFactory.sol - L31
9. FETHNode.sol - L23
10. FoundationTreasuryNode.sol - L48

12. Simplify supportsInterface check <a id="support-interface"></a>

12.1 NFTDropCollection.sol

NFTDropCollection.supportsInterface (lines 284-295) can be changed to:

function supportsInterface(bytes4 interfaceId)
	public
	view
	override(ERC165Upgradeable, AccessControlUpgradeable, ERC721Upgradeable, CollectionRoyalties)
	returns (bool)
{
	return (
		interfaceId == type(INFTDropCollectionMint).interfaceId || 
		super.supportsInterface(interfaceId);
	);
}

12.2 CollectionRoyalties.sol

CollectionRoyalties.supportsInterface (lines 80-91) can be changed to:

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
	return (
		interfaceId == type(IRoyaltyInfo).interfaceId ||
		interfaceId == type(ITokenCreator).interfaceId ||
		interfaceId == type(IGetRoyalties).interfaceId ||
		interfaceId == type(IGetFees).interfaceId ||
		interfaceSupported = super.supportsInterface(interfaceId)
	)
}

13. Floating Solidity Pragma Version <a id="floating-version"></a>

It's best to use the same compiler version across all project files/team members. So having a fixed version pragma is a good practice. Most contracts use a floating pragma which would allow the patch number to be equal or higher than the specified patch number.

14. Avoid Nested if Blocks <a id="if-blocks"></a>

For better readability and analysis it is better to avoid nested if blocks. Here is an example:

14.1 FETH.sol (lines 482-492)

After edit:

if (spenderAllowance == type(uint256).max) {
	return ;
}

if (spenderAllowance < amount) {
	revert FETH_Insufficient_Allowance(spenderAllowance);
}
// The check above ensures allowance cannot underflow.
unchecked {
	spenderAllowance -= amount;
}
accountInfo.allowance[msg.sender] = spenderAllowance;
emit Approval(from, msg.sender, spenderAllowance);

Table of Contents

1. PercentSplitETH.sol (Out of Scope)
2. BytesLibrary.sol (Out of Scope)
3. NFTCollectionFactory.sol
4. NFTDropCollection.sol
5. MarketFees.sol
6. NFTDropMarketFixedPriceSale.sol
7. SequentialMintCollection.sol
8. Use custom errors instead of require
9. CollectionRoyalties.sol
10. = 0 assignment can be avoided for default values

1. contracts/PercentSplitETH.sol (Out of Scope) <a id="PercentSplitETH.sol"></a>

Can be improved a lot. Since out of scope, did not allocate time to write a report here.

2. contracts/libraries/BytesLibrary.sol (Out of Scope) <a id="BytesLibrary.sol"></a>

2.1 replaceAtIf function

We can avoid abi.encodePacked and also the loop in replaceAtIf function, by implementing it in an assembly block. Also note that the assertion and replacement each can be done at once. So the lines 21-32 can be changed to:

assembly {
    // 0x14 = 0x20 - 0x0c = 12 index to the right of `startLocation`
    // so that `dataChunk` has the right most 20 bytes as the potential
    // `expectedAddress`.
    let dataPtr := add(data, add(0x14, startLocation))
    let dataChunk := mload(dataPtr)
    if (eq(expectedAddress, and(dataChunk, expectedAddress))) {
        // the 12 left most bytes of `maskedDataChunk` will be the same as
        // `dataChunk` and the rest would be 0.
        let maskedDataChunk = and(
            dataChunk,
            0xffffffffffffffffffffffff0000000000000000000000000000000000000000
        )
        mstore(dataPtr, or(
            maskedDataChunk,
            newAddress
        ))
    } else {
        // 0x370d3a6a00000000000000000000000000000000000000000000000000000000
        // is the bytes32 right padded signature of BytesLibrary_Expected_Address_Not_Found()
        mstore(0, 0x370d3a6a00000000000000000000000000000000000000000000000000000000)
        revert(0, 0x20)
    }
}

2.2 startsWith function

Again, we can avoid the loop, and also multiple memory reads. Here is the implementation using assembly blocks:

function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool doesStartsWith) {
  // A signature is 4 bytes long
  if (callData.length < 4) {
    return false;
  }
    
  assembly {
    // peek 4 bytes after the memory pointer
    let callDataChunk := mload(add(callData, 4))
    doesStartsWith := eq(
      and(callDataChunk, functionSig),
      functionSig
    )
  }
}

startsWith is only used in contracts/PercentSplitETH.sol for the external function proxyCall. So the gas savings would be noticed there.

3. contracts/NFTCollectionFactory.sol <a id="NFTCollectionFactory.sol"></a>

3.1 adminUpdateNFTCollectionImplementation function

The unchecked block in adminUpdateNFTCollectionImplementation can be rewritten as

unchecked {
  // Version cannot overflow 256 bits.
-  versionNFTCollection++;
+  ++versionNFTCollection;
}

Also the versionNFTCollection.toString() which is passed to INFTCollectionInitializer(_implementation).initialize can be factored out into a variable to avoid the toString() conversion twice.

string memory version = versionNFTCollection.toString();

// The implementation is initialized when assigned so that others may not claim it as their own.
  INFTCollectionInitializer(_implementation).initialize(
    payable(address(rolesContract)),
    string.concat("NFT Collection Implementation v", version),
    string.concat("NFTv", version)
  );

4. contracts/NFTDropCollection.sol <a id="NFTDropCollection.sol"></a>

The loop (L181-L186) in mintCountTo of NFTDropCollection can be gas optimized by moving the loop condition inside the loop block:

for (uint256 i = firstTokenId; ; ) { 
  _mint(to, i);
  unchecked {
    ++i;
    if (i > latestTokenId) {
      break;
    }
  }
}

By moving the condition inside and modifying it to i > latestTokenId instead of i <= latestTokenId, the compiler would translate it into only one comparison (gt) versus two (lt or eq). Therefore, it would say gas.

Note: with this change, the loop would run at least once, but this would also be the case in the original code, since

And this is because since we are passed line 172, we know that count is greater than 0.

5. contracts/mixins/shared/MarketFees.sol <a id="MarketFees.sol"></a>

The unchecked loop block in the _distributeFunds (line 126-135) can be optimized by caching creatorRecipients.length, it will avoid multiple mloads per iteration. So here is how it would look like after modification (also note the @audit comments):

unchecked {
  uint256 i; //@audit we don't need to assign 0, since 0 is the default value.
  uint256 creatorRecipientsLength = creatorRecipients.length;
  for (; i < creatorRecipientsLength; ) {
    _sendValueWithFallbackWithdraw(
      creatorRecipients[i],
      creatorShares[i],
      SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
    );
    // Sum the total creator rev from shares
    // creatorShares is in ETH so creatorRev will not overflow here.
    creatorRev += creatorShares[i];
    ++i;
  }
}

The same type of optimization can be applied to the for loop block in getFeesAndRecipients function on line 198-200

uint256 i; //@audit we don't need to assign 0, since 0 is the default value.
uint256 creatorSharesLength = creatorShares.length;
for (; i < creatorSharesLength; ) {
  creatorRev += creatorShares[i];
  ++i;
}

internalGetImmutableRoyalties function

address payable[] memory recipients, uint256[] memory splitPerRecipientInBasisPoints can be combined into 1 dynamic array, it would save memory and therefore gas. IGetRoyalties would also need to be modified.

6. contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol <a id="NFTDropMarketFixedPriceSale.sol"></a>

In mintFromFixedPriceSale we can cache saleConfig.limitPerAccount (line 180) on the stack which has been used 3 times. This would remove 2 extra mloads.

FixedPriceSaleConfig memory saleConfig = nftContractToFixedPriceSaleConfig[nftContract];
uint256 limitPerAccount = uint256(saleConfig.limitPerAccount); //@audit use this variable from this point on instead of `saleConfig.limitPerAccount`

7. contract/mixins/collections/SequentialMintCollection.sol <a id="SequentialMintCollection.sol"></a>

In _updateMaxTokenId we can change the condition in the 3rd require from

Before:

require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

To:

require(!(latestTokenId > _maxTokenId), "SequentialMintCollection: Max token ID must be >= last mint");

aka:

- latestTokenId <= _maxTokenId
+ !(latestTokenId > _maxTokenId)

Since EVM doesn't have a less than or equal opcode the compiler has to translate that into 2 comparisons LT and EQ which both require 2 stack items and both results have to be true to pass. We can save gas here by using GT and negate the result by NOT opcode which would only require the result on the stack from GT.

8. Use custom errors instead of require <a id="custom-errors"></a>

The following require statements can be turned into custom errors to save gas. The list of locations require statement can be changed to custom errors:

1. MinterRole.sol - L22
2. ContractFactory.sol - L22
3. ContractFactory.sol - L31
4. AdminRole.sol - 20
5. OZAccessControlUpgradeable.sol - L137
6. OZAccessControlUpgradeable.sol - L152
7. NFTCollection.sol - L158
8. NFTCollection.sol - L263-L264
9. NFTCollection.sol - L268
10. NFTCollection.sol - L327
11. NFTCollectionFactory.sol - L173
12. NFTCollectionFactory.sol - L182
13. NFTCollectionFactory.sol - L203
14. NFTCollectionFactory.sol - L227
15. NFTCollectionFactory.sol - L262
16. NFTDropCollection.sol - L88
17. NFTDropCollection.sol - L93
18. NFTDropCollection.sol - L130-L131
19. NFTDropCollection.sol - L172
20. NFTDropCollection.sol - L179
21. NFTDropCollection.sol - L238
22. PercentSplitETH.sol - L130-L131
23. PercentSplitETH.sol - L136
24. PercentSplitETH.sol - L148
25. PercentSplitETH.sol - L191-L195
26. PercentSplitETH.sol - L216
27. AddressLibrary.sol - L31
28. SequentialMintCollection.sol - L58
29. SequentialMintCollection.sol - L63
30. SequentialMintCollection.sol - L74
31. SequentialMintCollection.sol - L87-L89
32. AdminRole.sol - L19

9. contracts/mixins/collections/CollectionRoyalties.sol <a id="CollectionRoyalties.sol"></a>

In CollectionRoyalties, the functions getFeeRecipients, getFeeBps, getRoyalties only return arrays of length 1. It would be best to redesign some contracts so that these return values can be instead just values and not arrays. This would prevent using memory and the gas costs associated with using memory (MLOAD, MSTORE, and memory expansion gas costs).

10. = 0 assignment can be avoided for default values <a id="zero-assign"></a>

Since value types have by default the value of 0, explicit 0 assignment can be avoided which actually would require more gas.

Example:

- uint256 i = 0;
+ uint256 i;

List of locations where this change can be applied:

1. PercentSplitETH.sol - L115
2. PercentSplitETH.sol - L135
3. PercentSplitETH.sol - L320
4. BytesLibrary.sol - L25
5. BytesLibrary.sol - L44
6. MarketFees.sol - L126
7. MarketFees.sol - L198
8. MarketFees.sol - L484