Platform: Code4rena
Start Date: 16/02/2023
Pot Size: $144,750 USDC
Total HM: 17
Participants: 154
Period: 19 days
Judge: Trust
Total Solo HM: 5
Id: 216
League: ETH
Rank: 114/154
Findings: 1
Award: $61.26
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: GalloDaSballo
Also found by: 0x3b, 0xAgro, 0xSmartContract, 0xTheC0der, 0xackermann, 0xnev, 0xsomeone, ABA, BRONZEDISC, Bjorn_bug, Bnke0x0, Breeje, Co0nan, CodeFoxInc, CodingNameKiki, DadeKuma, DeFiHackLabs, IceBear, Josiah, Kaysoft, Lavishq, MohammedRizwan, PaludoX0, PawelK, Phantasmagoria, Raiders, RaymondFam, Rickard, Rolezn, Sathish9098, SleepingBugs, SuperRayss, UdarTeam, Udsen, Viktor_Cortess, arialblack14, ast3ros, bin2chen, brgltd, btk, catellatech, ch0bu, chaduke, chrisdior4, codeislight, cryptonue, delfin454000, descharre, dontonka, emmac002, fs0c, hacker-dom, hansfriese, imare, lukris02, luxartvinsec, martin, matrix_0wl, peanuts, rbserver, shark, tnevler, trustindistrust, tsvetanovv, vagrant, yongskiws, zzzitron
61.2601 USDC - $61.26
During the audit, 2 low and 12 non-critical issues were found.
| â„– | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Check multisigRoles.length in ReaperVaultV2 | Low | 1 |
| L-2 | Some events could be more informative | Low | 4 |
| NC-1 | Order of Functions | Non-Critical | 10 |
| NC-2 | Order of Layout | Non-Critical | 1 |
| NC-3 | Missing leading underscores | Non-Critical | 12 |
| NC-4 | Inconsistency when using uint and uint256 | Non-Critical | 5 |
| NC-5 | Unused named return variables | Non-Critical | 13 |
| NC-6 | Inconsistency when using the number 10000 | Non-Critical | 1 |
| NC-7 | Visibility is not set | Non-Critical | 5 |
| NC-8 | Wrong modifier order in function declaration | Non-Critical | 2 |
| NC-9 | Missing NatSpec | Non-Critical | 4 |
| NC-10 | Incomplete NatSpec | Non-Critical | 1 |
| NC-11 | No space between the control structures | Non-Critical | 3 |
| NC-12 | Maximum line length exceeded | Non-Critical | 42 |
Function __ReaperBaseStrategy_init checks that multisigRoles.length == 3 before granting roles:
require(_multisigRoles.length == 3, "Invalid number of multisig roles"); _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); _grantRole(DEFAULT_ADMIN_ROLE, _multisigRoles[0]); _grantRole(ADMIN, _multisigRoles[1]); _grantRole(GUARDIAN, _multisigRoles[2]);
But in ReaperVaultV2 constructor, there is no such check.
In ReaperVaultV2 constructor, it is better to also check the multisigRoles.length:
require(_multisigRoles.length == 3, "Invalid number of multisig roles");
Some events in setters functions emit only new set values, but it may be more convenient to emit old values as well.
emit YieldingPercentageUpdated(_collateral, _bps);emit YieldingPercentageDriftUpdated(_driftBps);emit YieldClaimThresholdUpdated(_collateral, _threshold);emit YieldDistributionParamsUpdated(_treasurySplit, _SPSplit, _stakingSplit);It is better to add info about previous values in the events. For example:
emit YieldingPercentageUpdated(_collateral, _previous_bps, _new_bps);
According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:
External functions can not go after public functions:
Public function can not go after internal function:
External functions can not go after public and internal functions:
Reorder functions where possible.
According to Order of Layout, inside each contract, library or interface, use the following order:
Place modifiers before functions.
Internal and private constants, state variables, and functions should have a leading underscore.
address stabilityPoolAddress;address gasPoolAddress;ICollSurplusPool collSurplusPool;address gasPoolAddress;ICollSurplusPool collSurplusPool;mapping (address => uint256) internal collAmount;mapping (address => uint256) internal LUSDDebt;mapping (address => uint256) internal collAmounts;uint256 internal totalLUSDDeposits;uint internal immutable deploymentStartTime;uint16 private constant LENDER_REFERRAL_CODE_NONE = 0;function roundUpDiv(uint256 x, uint256 y) internal pure returns (uint256) {Add leading underscores where needed.
Some variables is declared as uint and some as uint256.
Stick to one style.
Both named return variable(s) and return statement are used.
To improve clarity use only named return variables.
For example, change:
function functionName() returns (uint id) { return x;
to
function functionName() returns (uint id) { id = x;
In some cases, 10000 is used, and in some - 10_000.
Here 10000:
While in other cases - 10_000, for example:
Stick to one style.
address stabilityPoolAddress;address gasPoolAddress;ICollSurplusPool collSurplusPool;address gasPoolAddress;ICollSurplusPool collSurplusPool;It is better to specify visibility explicitly.
According to Style Guide, the modifier order for a function should be:
Change pure internal to internal pure.
NatSpec is missing for all functions in 4 contracts.
Add NatSpec for all functions.
Not all parameters have a description, which complicates the understanding of the code.
Add a description for _treasury, _strategists, and _multisigRoles.
According to Style Guide, there should be a single space between the control structures if, while, and for and the parenthetic block representing the conditional.
for(uint256 i = 0; i < _collaterals.length; i++) {for(uint256 i = 0; i < numCollaterals; i++) {} else if(vars.percentOfFinalBal < vars.yieldingPercentage && vars.yieldingPercentage.sub(vars.percentOfFinalBal) > yieldingPercentageDrift) {Change:
for(...) { ... }
to:
for (...) { ... }
According to Style Guide, maximum suggested line length is 120 characters. Longer lines make the code harder to read.
Make the lines shorter.
#0 - c4-judge
2023-03-09T11:12:49Z
trust1995 marked the issue as grade-b
#1 - c4-sponsor
2023-03-18T00:10:46Z
0xBebis marked the issue as sponsor confirmed