QA Report

[code423n4]

22-08-foundation

abstract contract ContractFactory is superfluous

Risk	low
Affects	NFTCollection, NFTDropCollection

onlyContractFactory modifier is superfluous and therefore the abstract contract ContractFactory can be deleted in order to save some gas, this modifier is only ever used by the initialize function in contracts NFTCollection and NFTDropCollection, in order to prevent other addresses from calling that function, i.e. the allowed msg.sender address must be declared at deployment.

Resolution

Instead of passing down the address of the factory contract at deployment in the constructor, it is also possible to call the initialize function directly within the function responsible for deploying the clone in the factory contract; the atomicity of the transaction will ensure that initialize is called immediately after the contract has been deployed, thus removing the need for knowing the factory's address beforehand. An example of such function is shown below, in this case the function is deploying a payment splitter contract clone and immediately calling the initialize function:

function deployClone(
    address[] calldata _payees,
    uint256[] calldata _shares
) external returns (address clone) {
    clone = master.clone();

    PaymentSplitter(payable(clone)).initialize(_payees, _shares);

    emit NewClone(clone, msg.sender);
}

Once that's done, the abstract contract ContractFactory can be deleted, then any references to it must be removed in NFTCollection and NFTDropCollection, such as from inherited contracts and from the constructor. This may also eliminate the need for the constructor, ensure that this doesn't cause any security issues first.

Salted (deterministic) contract creation not needed in factory contract

Risk	low
Affects	PercentSplitETH, NFTCollectionFactory

Salted contract creations / create2 are used as part of OpenZeppelin's cloneDeterministic function, inherited by PercentSplitETH and NFTCollectionFactory contracts.

Although this choice may have been intentional by Foundation, i.e. using cloneDeterministic rather than the simpler clone function, we couldn't think of any practical reason for this choice.

Our rationale is that cloneDeterministic should only be used if the address of the to-be-deployed-contract is needed by another contract or application before the transaction is broadcast to the Ethereum network.

For example, an erc721 factory contract might have an external function needed to do three things at once: deploy the clone, initialize the clone, mint the first token. In this scenario the contract's address might need to be written somewhere in the IPFS metadata file, such as OpenSea's external_url property.

Resolution

Unless strictly needed, replace the cloneDeterministic function with clone in the PercentSplitETH and NFTCollectionFactory contracts, this will save gas to end users.

Duplicated event data

Risk	low
Affects	NFTCollection

The indexedTokenCID is emitted by the Minted event, indexed to enable watching for mint events by the tokenCID. However, the same exact parameter is emitted with a different variable name as tokenCID, on line 273:

emit Minted(msg.sender, tokenId, tokenCID, tokenCID);

There is no need to emit the same data twice, only the indexed parameter is needed when searching for its content.

Furthermore, the Minted event itself is superfluous, since the same exact data is emitted by the Transfer event in ERC721Upgradeable:

event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);

Since the address when minting is always 0x0, it is possible to discern minting event from regular tranfers by fitering for address indexed from parameter equal to address 0x0.

Resolution

Remove the tokenCID parameter from the Minted event

Ideally remove the Minted event completely and rely on the Transfer event instead.

[HardlyDifficult]

abstract contract ContractFactory is superfluous

Disagree. The purpose of this pattern is to ensure all contracts using this template will have been created by the same factory. The rec here wouldn't provide the same guarantee.

Salted (deterministic) contract creation not needed in factory contract

Disagree. This approach allows the creator to choose from a list of possible addresses, or generate a vanity address of their choosing. It also allows a self-destructed collection to be redeployed to the same address after making a correction.

Duplicated event data with tokenCID

Disagree. This approach was done to allow our frontend to filter based on the tokenCID topic. However since this is a string that topic would not emit the complete data -- so we have that included as a separate param so that complete information needed is available in the event. This approach works for various frontend / backend requirements.

[HickupHH3]

agree with sponsor's reasoning for all issues. Every point mentioned (and thus the whole report) was disputed, hence the invalidation.