GalloDaSballo
commented
2 years ago [NAZ-L1] Signature Malleability of EVM's ecrecover()
R in this case because the malleability cannot be used for anything as the setting is idempotent (using second sig does nothing)
[NAZ-L2] Value Range Validity
L
[NAZ-L3] Missing Equivalence Checks In Setters
NC
[NAZ-L4] Missing Zero-address Validation
L
[NAZ-L5] receive() Function Should Emit An Event
I'm not convinced by this without an explanation
[NAZ-L6] Missing Events In Initialize Functions
NC
[NAZ-N1] Array length mismatch
Will cause a revert later, valid NC
[NAZ-N2] Unindexed Event Parameters
NC
[NAZ-N3] Use Underscores for Number Literals
Really minor, NC
[NAZ-N4] Code Structure Deviates From Best-Practice
I don't understand this one, I think it's best if you show how to improve the code next time
[NAZ-N5] Code Contains Empty Blocks
NC
[NAZ-N6] Line Length
R
[NAZ-N7] Function && Variable Naming Convention
R
[NAZ-N8] Spelling Errors
NC
[NAZ-N9] Multiple Solidity Pragma & Floating & Old
NC
2L 3R 7NC
[NAZ-L1] Signature Malleability of EVM's
ecrecover()Severity: Low Context:
Governor.sol#L236,ERC721Votes.sol#L167Description: The function calls the Solidity
ecrecover()function directly to verify the given signatures. However, theecrecover()EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice.
Recommendation: Use the ecrecover function from OpenZeppelin's ECDSA library for signature verification. (Ensure using a version
> 4.7.3for there was a critical bug>= 4.1.0 < 4.7.3).[NAZ-L2] Value Range Validity
Severity Low Context:
Auction.sol#L77-L78,Auction.sol#L307-L335,Governor.sol#L61-L64,Governor.sol#L564-L592,Governor.sol#L211-L231Description: These functions doesn't have any checks to ensure that the variables being set is within some kind of value range.
Recommendation: Each variable input parameter updated should have it's own value range checks to ensure their validity.
[NAZ-L3] Missing Equivalence Checks In Setters
Severity: Low Context:
MetadataRenderer.sol#L347-L367,Auction.sol#L307-L335,Governor.sol#L564-L602,Governor.sol#L211-L231Description: Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.
Recommendation: This may hinder detecting discrepancies between on-chain and off-chain states leading to flawed assumptions of on-chain state and protocol behavior.
[NAZ-L4] Missing Zero-address Validation
Severity: Low Context:
Manager.sol#L62-L66,Token.sol#L46-L47,MetadataRenderer.sol#L32,MetadataRenderer.sol#L47-L49,Auction.sol#L39,Auction.sol#L55-L57,Governor.sol#L60,Treasury.sol#L33Description: 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-L5]
receive()Function Should Emit An EventSeverity: Low Context:
Treasury.sol#L269Description: Consider emitting an event inside this function with
msg.senderandmsg.valueas the parameters. This would make it easier to track incoming ether transfers.Recommendation: Consider adding events to the
receive()functions.[NAZ-L6] Missing Events In Initialize Functions
Severity: Low Context:
Manager.sol#L80,Token.sol#L43,MetadataRenderer.sol#L45,Auction.sol#L54,Governor.sol#L57,Treasury.sol#L43Description: None of the initialize functions 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 emit events in your initialization functions.
[NAZ-N1] Array length mismatch
Severity: Informational Context:
MetadataRenderer.sol#L92-L93,Governor.sol#L99-L101,Governor.sol#L117-L119,Governor.sol#L325-L327,Treasury.sol#L102-L104,Treasury.sol#L142-L144Description: These fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.
Recommendation: Perform input validation on the arrays to verify that the lengths match.
[NAZ-N2] Unindexed Event Parameters
Severity Informational Context:
IManager.sol#L21,IManager.sol#L26,IManager.sol#L31,IToken.sol#L21,IPropertyIPFSMetadataRenderer.sol#L16-L28,IAuction.sol#L22-L50,IGovernor.sol#L29-L57,ITreasury.sol#L16-L28Description: 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-N3] Use Underscores for Number Literals
Severity: Informational Context:
Auction.sol#L354Description: 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-N4] Code Structure Deviates From Best-Practice
Severity: Informational Context:
Manager.sol#L180,Token.sol#L143,MetadataRenderer.sol#L91,Auction.sol#L244,Governor.sol#L116,Treasury.sol#L116,ERC1967Upgrade.sol#L83,UUPS.sol#L47,ERC721.sol#L54,ERC721Votes.sol#L126Description: The best-practice layout for a contract should follow the following order: state variables, events, modifiers, constructor and functions. Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered as: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private. Functions should then further be ordered with view functions coming after the non-view labeled ones.
Recommendation: Consider adopting recommended best-practice for code structure and layout.
[NAZ-N5] Code Contains Empty Blocks
Severity: Informational Context:
Manager.sol#L209,Treasury.sol#L269,ERC721.sol#L57,ERC721.sol#L239,ERC721.sol#L249Description: 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 blocks.[NAZ-N6] Line Length
Severity: Informational Context:
Manager.sol#L113,Manager.sol#L167-L170,IManager.sol#L55,Token.sol#L268,IToken.sol#L86,MetadataRenderer.sol#L55,MetadataRenderer.sol#L206,MetadataRenderer.sol#L243,MetadataRenderer.sol#L259,IPropertyIPFSMetadataRenderer.sol#L63,Governor.sol#L27,Governor.sol#L362-L363,TreasuryStorageV1.sol#L11,TreasuryStorageV1.sol#L15,ERC721.sol#L134,ERC721.sol#L166,ERC721.sol#L184,ERC721Votes.sol#L10,ERC721Votes.sol#L21,ERC721Votes.sol#L162Description: Max line length must be no more than 120 but many lines are extended past this length.
Recommendation: Consider cutting down the line length below 120.
[NAZ-N7] Function && Variable Naming Convention
Severity Informational Context:
Manager.sol#L40,Manager.sol#L43,Manager.sol#L49,ManagerStorageV1.sol#L10,TokenStorageV1.sol#L11,TokenStorageV1.sol#L15,TokenStorageV1.sol#L19,MetadataRenderer.sol#L25,MetadataRendererStorageV1.sol#L11,MetadataRendererStorageV1.sol#L14,MetadataRendererStorageV1.sol#L17,MetadataRendererStorageV1.sol#L20,Auction.sol#L28,Auction.sol#L31,AuctionStorageV1.sol#L12,Governor.sol#L34,GovernorStorageV1.sol#L11,GovernorStorageV1.sol#L15,GovernorStorageV1.sol#L19,Treasury.sol#L25,ERC721.sol#L26,ERC721.sol#L30,ERC721.sol#L38,ERC721.sol#L47,ERC721Votes.sol#L29,ERC721Votes.sol#L33,ERC721Votes.sol#L37,ERC721Votes.sol#L78Description: The linked variables do not conform to the standard naming convention of Solidity whereby functions and variable names(local and state) utilize the
mixedCaseformat unless variables are declared asconstantin which case they utilize theUPPER_CASE_WITH_UNDERSCORESformat. Private variables and functions should lead with an_underscore.Recommendation: Consider naming conventions utilized by the linked statements are adjusted to reflect the correct type of declaration according to the Solidity style guide.
[NAZ-N8] Spelling Errors
Severity: Informational Context:
Manager.sol#L113 (responsiblity=> responsibility),Token.sol#L104 (receive => receive),TokenTypesV1.sol#L13 (metadatarenderer => metadataRenderer),IGovernor.sol#L74 (calldatas => calldata)Description: Spelling errors in comments can cause confusion to both users and developers.
Recommendation: Consider checking all misspellings to ensure they are corrected.
[NAZ-N9] Multiple Solidity Pragma
Severity: Informational Context:
All ContractsDescription: It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks.
Recommendation: Consider ensuring all pragma versions are the same one.
[NAZ-N10] Floating Pragma
Severity: Informational Context:
./proxyDescription: 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-N11] Older Version Pragma
Severity: Informational Context:
All ContractsDescription: 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.