Open code423n4 opened 2 years ago
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.
[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 EventSeverity: Low Context:
FETHNode.sol#L33
Description: Consider emitting an event inside this function with
msg.sender
andmsg.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.