QA Report

[code423n4]

[NAZ-L1] Missing Zero-address Validation

Severity: Low
Context: AdminRole.sol#L15, AdminRole.sol#L28, AdminRole.sol#L37, MinterRole.sol#L28, MinterRole.sol#L36, MinterRole.sol#L45

Description:
Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

Recommendation:
Consider adding explicit zero-address validation on input parameters of address type.

[NAZ-L2] receive() Function Should Emit An Event

Severity: Low
Context: FETHNode.sol#L33

Description:
Consider emitting an event inside this function with msg.sender and msg.value as the parameters. This would make it easier to track incoming ether transfers.

Recommendation:
Consider adding events to the receive() functions.

[NAZ-L3] Missing Events In Initialize Functions

Severity: Low
Context: NFTCollectionFactory.sol#L192, NFTDropCollection.sol#L120, NFTCollection.sol#L105, NFTDropMarket.sol#L100

Description:
None of the initialize functions emit emit init-specific events. They all however have the initializer modifier (from Initializable) so that they can be called only once. Off-chain monitoring of calls to these critical functions is not possible.

Recommendation:
It is recommended to perform validation of input parameters and emit events.

[NAZ-N1] TODOs Left In The Code

Severity: Informational
Context: MarketFees.sol#L193

Description:
There should never be any TODOs in the code when deploying.

Recommendation:
Consider finishing the TODOs before deploying.

[NAZ-N2] Use Underscores for Number Literals

Severity: Informational
Context: Constants.sol#L10, Constants.sol#L26, Constants.sol#L38, Constants.sol#L48, Constants.sol#L53, MarketFees.sol#L45

Description:
There are multiple occasions where certain numbers have been hardcoded, either in variables or in the code itself. Large numbers can become hard to read.

Recommendation:
Consider using underscores for number literals to improve its readability.

[NAZ-N3] Unindexed Event Parameters

Severity Informational
Context: NFTDropCollection.sol#L85

Description:
Parameters of certain events are expected to be indexed so that they’re included in the block’s bloom filter for faster access. Failure to do so might confuse off-chain tooling looking for such indexed events.

Recommendation:
Consider adding the indexed keyword to event parameters that should include it.

[NAZ-N4] Code Contains Empty Blocks

Severity: Informational
Context: NFTDropCollection.sol#L102, NFTCollection.sol#L96, NFTDropMarket.sol#L93

Description:
It's best practice that when there is an empty block, to add a comment in the block explaining why it's empty.

Recommendation:
Consider adding /* Comment on why */ to the empty block.

[NAZ-N5] Floating Pragma

Severity: Informational
Context: All Contracts

Description:
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Recommendation:
Consider locking the pragma version.

[NAZ-N6] Older Version Pragma

Severity: Informational
Context: All Contracts

Description:
Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs.

Recommendation:
Consider using the most recent version.

[NAZ-N7] Missing or Incomplete NatSpec

Severity: Informational
Context: All Contracts

Description:
Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Recommendation:
Add in full NatSpec comments for all functions to have complete code documentation for future use.

[HardlyDifficult]

Missing Zero-address Validation in roles

Agree but not planning to address this at this time. These files lean on the OpenZeppelin role implementation here and are just convenience wrappers -- adding requirements in these files would not be reflected in the OZ public functions which would still be available to call directly. This may be reasonable feedback to give to them and then we would inherit the change. It's not harmful as is though because even if address(0) is granted a role, that address cannot perform actions.

[NAZ-L2] receive() Function Should Emit An Event

Reasonable feedback but I don't feel it's necessary. The scenarios that can trigger this flow already have events more specific to the use case -- and FETH itself will emit transfers like this.

Missing events on initialize

Disagree - but it's a valid point. For the collection creations that information is already emitted by the factory, including it here would be redundant. For versionNFTCollection that value will be emitted when the first template is assigned. I don't believe emitting for the ReentrancyGuard is helpful or necessary.

Unresolved TODO comments

Agree, will fix.

Missing indexed event parameters

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.

Code Contains Empty Blocks

Fair feedback but I think it's clear enough already in these examples.

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.

Missing natspec comments

Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.

@inheritdoc is a natspec supported standard that effectively inlines comments from another file -- for those examples we should have complete coverage already.