Platform: Code4rena
Start Date: 14/06/2022
Pot Size: $100,000 USDC
Total HM: 26
Participants: 59
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 9
Id: 133
League: ETH
Rank: 46/59
Findings: 1
Award: $212.99
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xf15ers, 0xmint, Bronicle, Dravee, Funen, JMukesh, Limbooo, MadWookie, Picodes, Ruhum, TerrierLover, TomJ, Tutturu, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, cccz, codexploder, cryptphi, csanuragjain, defsec, fatherOfBlocks, gzeon, hake, hansfriese, hyh, ignacio, k, nxrblsrpr, oyc_109, robee, sach1r0, saian, simon135, technicallyty, zzzitron
101.8823 USDC - $101.88
687.9945 CANTO - $111.11
The variable UniGovModAcct is referenced within a local scope prior to be declared within the global scope.
It is of good practice, for the sake of readability, to declare at the beginning of the contract all the state variables, followed by the modifiers and finally by the cosntructor and functions.
In this particular contract, this good practice is unfollowed.
This yields to certain head-scratchers when reading the code, such as in the particular case of declaring the modifier OnlyUniGov (which uses in its local scope the UniGovModAcct state variable) before the UniGovModAcct state variable.
Consider following the best practices: declare first the state variables, followed by modifieras and finally constructor and functions.
Proposal-Store.sol functionsThe file Proposal-Store.sol, which contains the ProposalStore contract, lacks documentation around its functions.
For the sake of readability, it would be an improvement to add some additional natspec around, for example, AddProposal and QueryProp, which are the contract's functions.
Consider adding comments/natspec documenting these functions.
There is a comment which states exactly the contrary that is declared on the codebase.
On line 43 of BaseV1-core.sol, there is a comment stating that the stable state variable is NOT immutable. However, this variable is in fact explicitly declared as immutable. Is it an mistake in the implementation or in the codebase?
Consider revising this part of the contract, and depending on the contract specs, either re-write the comment or declare the stable variable as mutable.
Stale comments pollute the codebase with old code which quickly becomes deprecated in new versions.
It is of good practice not to have stale comments, as commented code quickly becomes deprecated because it does not evolve with the codebase. Thus, uncommenting it in later versions may lead to error or unexpected behaviors.
Consider deleting stale comments
It is of good practice to declare all dependencies, even if they are smart contracts themselves, prior to their usage. For the sake of readability.
The BaseV1Factory contract type is referenced within the BaseV1Pair, but it is declared much later. This makes more difficult to understand the codebase, as it doesn't follow the good practices.
Consider defining the BaseV1Factory contract right after the interface definitions.
Solidity allows different return styles: implicitly returning variables by assgning them the same name as in the returns() part of the function declaration, or explicitly calling return() on a variable. For the sake of readability, similar functions should not mix these styles up.
Several (custom) getter functions are declared within BaseV1Pair, but not all of them follow the same return style.
This might look confusing at first.
Consider choosing one style and stick with it during the entire codebase, to improve readability.
That means, for example, declare getReserves() as follows:
function getReserves() public view returns (uint _reserve0, uint _reserve1, uint _blockTimestampLast) { return(reserve0, reserve1, blockTimestampLast); }
Require statements should usually provide error strings explaining why the revert has occurred. These error strings should be easy to interpret.
Error string should be present on important require checks, and they should be easy to interpret.
However within the BaseV1Pair, some functions like mint, contain error strings that are explained in the comments present in the codebase. A user facing a revert by triggering it from mint's require statement, would get an error string stating "ILM" instead of a more readable alternative like "INSUFFICIENT LIQUIDITY MINTED" (which is present as a comment).
Consider writing more error strings, and improving the readability of some of the existent error strings.
It is of good practice to modularize code instead of hard-coding it over and over among several files within a same project. This helps mantainability and avoids unexpected behaviors due to typos.
Files BaseV1-periphery.sol and BaseV1-core.sol declare the same interfaces and libraries.
These declarations should be made on a separate file, and then BaseV1-periphery.sol and BaseV1-core.sol could just import said file, for the sake of mantainability and security.
Consider deleting the shared definitions from their original codebases, creating a interfaces.sol and a library.sol files to group these definitions together, and then importing them into the appropriate codebases.
Overflow/underflow were common points of failure of smart contracts. However, since solc version 0.8.0, these checks happen in automatically by the compiler. Thus, safemath-like functions are no longer necessary.
The GovernorBravoDelegate.sol declares the add256() and sub256() functions, which perform overflow/underflow checks. Since the pragma declared is pragma solidity ^0.8.10;, they aren'treally needed because this compiler version does the overflow/underflow by default. In addition to that, they aren't used within any unchecked blocks. Removing these functions and using ordinary addition/subtraction operators is safe, and more gas efficient.
Remove these function declarations and substitute them for the addition/subtraction operators.
Contract contains a function that is not used in its codebase.
The sub256 function is declared but not used (in addition to not being necessary due to issue number 9 of this report).
Remove this function declaration from the codebase
#0 - GalloDaSballo
2022-08-02T20:01:17Z
Valid Ref
Valid NC
Valid Ref
Valid R
Disagree as the factory is using the Pair, the current order logically makes more sense (and this argument cuts both ways
Valid R
NC
Valid R
Valid R
Valid R
7R 2NC