QA Report

[code423n4]

QA REPORT

[LOW] Add timelock for the following functions

Using a timelock in the following type of functions is common among defi protocols.

Example: FixedPriceDrop.sol#L27

[LOW] Payable functions that should not be payable

The following functions are payable but doesn't record the sender transaction. Consider making them not payable instead.

Proof of concept:

• PercentSplitETH.sol#L156
• FETH.sol#L292
• FETH.sol#L254

[LOW] Not verified input

At the following functions you should verify the parameters that are being assigned to a state variable.

Proof of concept:

• ContractFactory.sol#L30
• FoundationTreasuryNode.sol#L47
• NFTCollectionFactory.sol#L202

[LOW] Missing nonReentrancy modifier

The following functions allows attackers to try reentrancy since they are calling to external contracts / transferring eth. Consider adding a nonReentrancy modifier.

Proof of concept:

• NFTCollectionFactory.sol#L226
• NFTCollectionFactory.sol#L261
• FixedPriceDrop.sol#L27

[LOW] The project is compiled with different solidity versions

[NON CRITICAL] Floating pragma

Floating pragma is a bad practice, since it does not guaranty the same version at future deployments.

Proof of concept:

• SequentialMintCollection.sol
• MinterRole.sol
• OZAccessControlUpgradeable.sol

[NON CRITICAL] Consider emitting an event at the following functions

Proof of concept:

• ContractFactory.sol#L30
• FoundationTreasuryNode.sol#L47
• NFTDropCollection.sol#L266

[NON CRITICAL] Missing function spec comments

Proof of concept:

• FETH.sol#L514
• PercentSplitETH.sol#L156
• NFTCollectionFactory.sol#L202

[NON CRITICAL] The following events are not indexed

Proof of concept:

• NFTDropCollection.sol#L85
• NFTCollection.sol#L70

[NON CRITICAL] Unused function parameters should have name removed

If for any reason the following unused parameters are necessary then remove their naming (since only the type matters for function signature)

Proof of concept:

• NFTDropMarket.sol#L94
• NFTDropCollection.sol#L103
• NFTCollection.sol#L97

[HardlyDifficult]

[LOW] Add timelock for the following functions

Fair point and something we'll consider in the future.

[LOW] Payable functions that should not be payable

Invalid. These are payable by design and require it.

[LOW] Not verified input

Invalid. All those examples confirm .isContract - it's not clear what else you are suggesting here.

[LOW] Missing nonReentrancy modifier

Without a POC on a potential risk, we don't believe that a reentrancy guard is required here.

[LOW] The project is compiled with different solidity versions

Invalid, all contracts are compiled with 0.8.16

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.

[NON CRITICAL] Consider emitting an event at the following functions

I don't believe events are required for these constructors.

[NON CRITICAL] Missing function spec comments

invalid. FETH & PercentSplitETH were out of scope. The other example has a full natspec included already.

[NON CRITICAL] The following events are not indexed

I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.

[NON CRITICAL] Unused function parameters should have name removed

Invalid - these params are required here due to the inheritance used.