QA Report

[code423n4]

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

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

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

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

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

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

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

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 (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

Keep line width to max 120 characters for better readability.

10. Hard-coded gas limits

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

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

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

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

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

[HardlyDifficult]

Very detailed and thoughtful feedback -- thank you!

supportsInterface can be rewritten to avoid the if/esle branching:

I think I do like this style more, will consider the change.

contracts/interfaces/ICollectionFactory.sol

Agree, fixed.

contracts/mixins/shared/ContractFactory.sol

Not sure I'm following this suggestion. There does not appear to be another .code.length type check included at the moment. Considering a stricter check is compelling but since this is an admin function call I think that may be overkill here.

Shorter inheritance list

True but for top-level contracts I like to expand all inherited contracts to make it clear what all the dependencies are and the lineriazation order they are included in.

4.2 CID need to be unique per tokenID

Agree. This is a primary goal of the NFTDropCollection. As you note there are other more flexible ways we could run with this type of approach and we may consider those in the future.

5.1 .isContract()

Fair feedback. Considering a stricter check is compelling but since this is an admin function call I think that may be overkill here.

5.2 versionNFTDropCollection

By design - the default value of 0 is correct there. NFTCollections were previously created by a different factory contract, we wanted the new factory to pick up version where that left off. But drops are new so starting at 0 is correct.

5.3 a better name can be chosen for rolesContract

Agree, I like that name more and will update.

6.2 The comment on line 175 needs a bit of correction

Good catch -- this was missed after adding a require against count == 0. Will fix.

7. contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

Although minor, this approach was used to save gas for the happy case scenario since it avoids a second if condition.

contracts/FETH.sol (Out of Scope)

Fair feedback, but I think the current approach is easier to reason about. And saving admin-only gas is not a goal for us.

9. Line Width

Our linter is configured to require 120... although maybe you mean we are adding new lines too early in some instances (?)

10. Hard-coded gas limits

Fair feedback. However the use case requires some gas limit to be defined and it's not clear there is a viable alternative here.

11. address.isContract check

This is good feedback. ATM these checks are there to help avoid simple errors by the admin. I'm not sure that the stricter check is worth the complexity to maintain.

Use fixed pragma

Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.

14. Avoid Nested if Blocks

(out of scope) I agree that style is better, will fix.

[liveactionllama]

Per discussion with @HickupHH3 (judge), regarding severity and validity of above items:

"Slightly disagree with #3. Agree with sponsor that the suggestion isn't clear. The rest is ok!"