Platform: Code4rena
Start Date: 25/08/2022
Pot Size: $75,000 USDC
Total HM: 35
Participants: 147
Period: 7 days
Judge: 0xean
Total Solo HM: 15
Id: 156
League: ETH
Rank: 75/147
Findings: 2
Award: $86.97
๐ Selected for report: 0
๐ Solo Findings: 0
๐ Selected for report: zzzitron
Also found by: 0x040, 0x1f8b, 0x52, 0x85102, 0xDjango, 0xNazgul, 0xNineDec, 0xSky, 0xSmartContract, 0xkatana, 8olidity, Aymen0909, Bahurum, BipinSah, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chandr, Chom, CodingNameKiki, Deivitto, DimSon, Diraco, ElKu, EthLedger, Funen, GalloDaSballo, Guardian, IllIllI, JansenC, Jeiwan, Lambda, LeoS, Margaret, MasterCookie, PPrieditis, PaludoX0, Picodes, PwnPatrol, RaymondFam, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, StevenL, The_GUILD, TomJ, Tomo, Trust, Waze, __141345__, ajtra, ak1, apostle0x01, aviggiano, bin2chen, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, cccz, ch13fd357r0y3r, cloudjunky, cryptphi, csanuragjain, d3e4, datapunk, delfin454000, devtooligan, dipp, djxploit, durianSausage, eierina, enckrish, erictee, fatherOfBlocks, gogo, grGred, hansfriese, hyh, ignacio, indijanc, itsmeSTYJ, ladboy233, lukris02, martin, medikko, mics, natzuu, ne0n, nxrblsrpr, okkothejawa, oyc_109, p_crypt0, pfapostol, prasantgupta52, rajatbeladiya, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, sorrynotsorry, tnevler, tonisives, w0Lfrum, yixxas
54.3817 DAI - $54.38
During the audit, 6 low and 13 non-critical issues were found.
| โ | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Storage variables can be packed tightly | Low | 1 |
| L-2 | Missing check | Low | 1 |
| L-3 | Large number of observations may cause out-of-gas error | Low | 1 |
| L-4 | Incorrect code or comment | Low | 1 |
| L-5 | Misleading comments | Low | 4 |
| L-6 | Check zero denominator | Low | 2 |
| NC-1 | Constructor before modifiers | Non-Critical | 6 |
| NC-2 | Events before state variables | Non-Critical | 8 |
| NC-3 | State variables after constructor | Non-Critical | 1 |
| NC-4 | Public functions before external | Non-Critical | 13 |
| NC-5 | ะกomment in the wrong place | Non-Critical | 1 |
| NC-6 | EVENTS, STATE VARIABLES, ERRORS comments not everywhere | Non-Critical | 22 |
| NC-7 | No function visibility | Non-Critical | 7 |
| NC-8 | Typo | Non-Critical | 1 |
| NC-9 | Scientific notation may be used | Non-Critical | 7 |
| NC-10 | Constants may be used | Non-Critical | 16 |
| NC-11 | Open TODOs | Non-Critical | 4 |
| NC-12 | Missing NatSpec | Non-Critical | 32 |
| NC-13 | Public functions can be external | Non-Critical | 7 |
According to docs, multiple, contiguous items that need less than 32 bytes are packed into a single storage slot if possible. It might be beneficial to use reduced-size types if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation.
/// Tokens /// @notice OHM token contract ERC20 public immutable ohm; uint8 public immutable ohmDecimals; /// @notice Reserve token contract ERC20 public immutable reserve; uint8 public immutable reserveDecimals; /// Constants uint32 public constant FACTOR_SCALE = 1e4;
Consider changing order of variables to:
/// Tokens /// @notice OHM token contract ERC20 public immutable ohm; /// @notice Reserve token contract ERC20 public immutable reserve; //PACKED uint8 public immutable ohmDecimals; uint8 public immutable reserveDecimals; /// Constants uint32 public constant FACTOR_SCALE = 1e4;
No check that observationFrequency not equal to zero (like here). Without check, constructor call will fail not indicating Price_InvalidParams() error.
if (movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency_ != 0)
Change to:
if (observationFrequency_ == 0 || movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency_ != 0)
Loops that do not have a fixed number of iterations, for example, loops that depend on storage values, have to be used carefully: Due to the block gas limit, transactions can only consume a certain amount of gas. Either explicitly or just due to normal operation, the number of iterations in a loop can grow beyond the block gas limit, which can cause the complete contract to be stalled at a certain point.
uint256 numObs = observations.length; // Check that the number of start observations matches the number expected if (startObservations_.length != numObs || lastObservationTime_ > uint48(block.timestamp)) revert Price_InvalidParams(); // Push start observations into storage and total up observations uint256 total; for (uint256 i; i < numObs; ) { if (startObservations_[i] == 0) revert Price_InvalidParams(); total += startObservations_[i]; observations[i] = startObservations_[i]; unchecked { ++i; } }
Restrict the maximum number of observations.
Comment says that the variable numObservations should be cached but there is a call to the length of the observations array.
// Cache numObservations to save gas. uint256 numObs = observations.length;
Consider changing the code or comment. The contract has similar part of the code, so it seems preferable to change the code to:
// Cache number of observations to save gas. uint32 numObs = numObservations;
The code will become more consistent.
After /* ========== INTERNAL FUNCTIONS ========== */ comment, not all internal functions are placed. There are other internal functions in the contract, and they are located in other places in the code.
The same with /* ========== VIEW FUNCTIONS ========== */ comment.
Regroup functions or formulate more specific comments.
range.cushion.low.price and range.wall.low.price may be equal to zero, since they are initialized to zero here.
Add the check to prevent function call failure.
According to Order of Layout, modifiers should be declared before constructor.
Place modifiers before constructor.
According to Order of Layout, state variables should be declared before events.
Place state variables before events.
According to Order of Layout, constructor should be declared before state variables.
Place constructor before state variables.
According to Order of Functions, external functions should be declared before public.
Place external functions before public.
Messed up comments.
/* ========== STATE VARIABLES ========== */ event ApprovalRevoked(address indexed policy_, ERC20[] tokens_); // Modules OlympusTreasury internal TRSRY;
Change to:
/* ========== EVENTS =========== */ event ApprovalRevoked(address indexed policy_, ERC20[] tokens_); /* ========== STATE VARIABLES ========== */ // Modules OlympusTreasury internal TRSRY;
Some contracts (for example, this) have /* ========== EVENTS =========== */ and /* ========== STATE VARIABLES ========== */ comments and some do not. Only one contract has /* ========== ERRORS =========== */ comment, although others also declares errors.
Contracts without /* ========== EVENTS =========== */ and /* ========== STATE VARIABLES ========== */ comments:
Contracts without /* ========== ERRORS =========== */ comment:
Add missing comments.
No visibility specified.
All 7 functions in KernelUtils.sol
Declare functions visibility.
Typo in comment.
// Cache numbe of observations to save gas.
Change to:
// Cache number of observations to save gas.
For readability, it is better to use scientific notation.
wallSpread_ > 10000 ||cushionSpread_ > 10000 ||if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();if (configParams[4] > 10000 || configParams[4] < 100) revert Operator_InvalidParams();if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams();if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)Replace, for example, 10000 with 10e4.
Constants may be used instead of literal values.
for (uint256 i = 0; i < 5; ) (for 5)for (uint256 i = 0; i < 32; ) (for 32)wallSpread_ < 100 || (for 100)cushionSpread_ < 100 || (for 100)wallSpread_ > 10000 || (for 10000)cushionSpread_ > 10000 || (for 10000)if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) (for 100 and 10000)if (exponent > 38) revert Price_InvalidParams(); (for 38)if (configParams[2] < uint32(10_000)) (for 10_000)if (configParams[4] > 10000 || configParams[4] < 100) (for 100 and 10000)if (cushionFactor_ > 10000 || cushionFactor_ < 100) (for 100 and 10000)if (debtBuffer_ < uint32(10_000)) (for 10_000)if (reserveFactor_ > 10000 || reserveFactor_ < 100) (for 100 and 10000)if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT) (for 10000)(totalEndorsementsForProposal[proposalId_] * 100) (for 100)if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) (for 100)Define constant variables, especially, for repeated values.
Operator.sol has 1 open TODO, TreasuryCustodian.sol has 3 open TODO.
/// TODO determine if this should use the last price from the MA or recalculate the current price, ideally last price is ok since it should have been just updated and should include check against secondary? /// Current price is guaranteed to be up to date, but may be a bad value if not checked?// TODO Currently allows anyone to revoke any approval EXCEPT activated policies.// TODO must reorg policy storage to be able to check for deactivated policies.// TODO Make sure `policy_` is an actual policy and not a random address.Resolve issues.
NatSpec is missing for 32 functions in 9 contracts.
Add NatSpec for all functions.
If functions are not called by the contract where they are defined, they can be declared external.
All 7 functions from KernelUtils.sol.
Make public functions external, where possible.
๐ Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x85102, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xkatana, Amithuddar, Aymen0909, Bnke0x0, CertoraInc, Chandr, CodingNameKiki, Deivitto, Dionysus, Diraco, ElKu, Fitraldys, Funen, GalloDaSballo, Guardian, IllIllI, JC, JansenC, Jeiwan, LeoS, Metatron, Noah3o6, RaymondFam, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, Saintcode_, Shishigami, Sm4rty, SooYa, StevenL, Tagir2003, The_GUILD, TomJ, Tomo, Waze, __141345__, ajtra, apostle0x01, aviggiano, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, cccz, ch0bu, chrisdior4, d3e4, delfin454000, djxploit, durianSausage, erictee, exolorkistis, fatherOfBlocks, gogo, grGred, hyh, ignacio, jag, karanctf, kris, ladboy233, lukris02, m_Rassska, martin, medikko, natzuu, ne0n, newfork01, oyc_109, peiw, rbserver, ret2basic, robee, rokinot, rvierdiiev, sikorico, simon135, tnevler, zishansami
32.5857 DAI - $32.59
During the audit, 6 gas issues were found.
Prefix increment and decrement cost less gas than postfix.
Consider using prefix increment and decrement where it is relevant.
Reading the length of an array at each iteration of the loop consumes extra gas.
for (uint256 step; step < instructions.length; ) {
Store the length of an array in a variable before the loop, and use it.
It costs gas to initialize integer variables with 0 or bool variables with false but it is not necessary.
for (uint256 i = 0; i < reqLength; ) {for (uint256 i = 0; i < 5; ) {for (uint256 i = 0; i < 32; ) {Remove initialization for default values.
For example:
for (uint256 i; i < reqLength; ) {
Using immutables is cheaper than storage-writing operations.
Use immutables where possible.
Change to ERC20 public immutable ohm;
> 0 is more expensive than =! 0if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
Use =! 0 instead of > 0, where possible.
x += y is more expensive than x = x + yreserveDebt[token_][msg.sender] += amount_;totalDebt[token_] += amount_;reserveDebt[token_][msg.sender] -= received;totalDebt[token_] -= received;if (oldDebt < amount_) totalDebt[token_] += amount_ - oldDebt;else totalDebt[token_] -= oldDebt - amount_;_movingAverage += (currentPrice - earliestPrice) / numObs;_movingAverage -= (earliestPrice - currentPrice) / numObs;total += startObservations_[i];balanceOf[from_] -= amount_;balanceOf[to_] += amount_;_amountsPerMarket[id_][0] += inputAmount_;_amountsPerMarket[id_][1] += outputAmount_;lastBeat += frequency();totalEndorsementsForProposal[proposalId_] -= previousEndorsement;totalEndorsementsForProposal[proposalId_] += userVotes;yesVotesForProposal[activeProposal.proposalId] += userVotes;noVotesForProposal[activeProposal.proposalId] += userVotes;Use x = x + y instead of x += y.
Use x = x - y instead of x -= y.