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: 8/147
Findings: 5
Award: $2,358.52
š Selected for report: 1
š Solo Findings: 1
š Selected for report: IllIllI
1905.4132 DAI - $1,905.41
While users are incentivized to call the heartbeat, the incentive may be removed later, or it may be more profitable to use old prices, so users may not call the heartbeat during unfavorable prices, leading to the TWAP price being incorrect, and users getting the wrong price for their assets.
A similar case of an incomplete TWAP algorithm was found to be of Medium risk
A TWAP is a Time-Weighted average price, but the algorithm below does not take into account the time between observations:
File: /src/modules/PRICE.sol #1 134 // Calculate new moving average 135 if (currentPrice > earliestPrice) { 136 _movingAverage += (currentPrice - earliestPrice) / numObs; 137 } else { 138 _movingAverage -= (earliestPrice - currentPrice) / numObs; 139 } 140 141 // Push new observation into storage and store timestamp taken at 142 observations[nextObsIndex] = currentPrice; 143 lastObservationTime = uint48(block.timestamp); 144: nextObsIndex = (nextObsIndex + 1) % numObs;
While the Heart policy enforces an upper bound on how frequently updates are added to the average, there is no guarantee that users call beat() in a timely manner:
File: /src/policies/Heart.sol #2 92 function beat() external nonReentrant { 93 if (!active) revert Heart_BeatStopped(); 94: if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();
The incentive may be set to too low an amount:
File: /src/policies/Heart.sol #3 140 function setRewardTokenAndAmount(ERC20 token_, uint256 reward_) 141 external 142 onlyRole("heart_admin") 143 { 144 rewardToken = token_; 145 reward = reward_; 146 emit RewardUpdated(token_, reward_); 147: }
Or users may find it more profitable to skip a particular update, or front-run an unfavorable update, with a transaction that trades assets at the old price
Code inspection
Always call an internal version of beat() that doesn't revert, in functions that swap user assets. The code should also track the timestamps of when each beat() is called, and include the amount of time that has passed since the last beat, in the TWAP calculation
#0 - Oighty
2022-09-08T17:52:27Z
The referenced issue is a bit different than our use case since we will be using a much longer duration moving average. The goal is to get an approximate moving average over a certain period of time (e.g. 120 days) vs. an exact number since, as you say, the time of each observation cannot be guaranteed to be at a specific time. We believe that using a long duration with a sufficient number of observations will make this value close enough to the true value it is approximating, and prevents actors from manipulating the value by waiting to provide a specific value (1 out of ~360 obs doesn't move the needle). The use of the "TWAP" term may be semantically inaccurate.
As for not guaranteeing that the update will be called or issues with several observations close to each other, see comments on #405 and #79.
The mitigations suggested do not seem to provide a solution that improves the system. Calling beat() on user actions would not have the observations roughly evenly spaced. Tracking timestamps is possible, but I don't see how it improves the data.
#1 - 0xean
2022-09-19T13:47:12Z
@Oighty - I think the warden is suggesting that the call to beat() in the user actions would do more to ensure that the "TWAP" stays up to date. If the call isn't past the correct period, it would just return and make no change (costing some amount of gas, ofc).
I do think it may be worth considering, that way no user action can take place without the TWAP being as up to date as possible and no additional calls to the contract may be necessary if users are interacting with the contract frequently enough.
While this is related to #79 - I think the points raised here and the mitigation is sufficiently different to warrant this issue to stand alone.
#2 - Oighty
2022-09-22T16:00:39Z
That's a fair point. One issue with calling beat on user actions, e.g. Operator.swap, is that it would update the wall price that the user is swapping at. Therefore, the call could fail due to the slippage check. This could be confusing behavior and may have unintended consequences of DOS'ing the system. Additionally, the gas cost of beat is highly variable (sometimes up to 600k gas when opening a bond market) and would cause some users to unexpectedly pay a lot more gas for a swap.
I'll discuss with the team, but I don't think the pros exceed the cons.
š Selected for report: GalloDaSballo
347.2615 DAI - $347.26
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/VoterRegistration.sol#L45-L56 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/PriceConfig.sol#L45-L74 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/TreasuryCustodian.sol#L71-L87 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/TreasuryCustodian.sol#L42-L48 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L140-L147 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L149-L152 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L586-L589
The introduction of roles drastically expands the attack surface of accounts that may be compromised in order to attack the protocol.
Even if the admin or role members are benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation.
There are many different ways to rug users, depending on which role is assigned.
voter_admins can give themselves votes, or take away votes from other users, making them able to win any vote:
File: /src/policies/VoterRegistration.sol #1 45 function issueVotesTo(address wallet_, uint256 amount_) external onlyRole("voter_admin") { 46 // Issue the votes in the VOTES module 47 VOTES.mintTo(wallet_, amount_); 48 } 49 ... 53 function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") { 54 // Revoke the votes in the VOTES module 55 VOTES.burnFrom(wallet_, amount_); 56: }
price_admins can repeatedly re-initialize the moving averages, which will cause the calls to fetch the price to always revert, not allowing users to redeem their assets:
File: /src/policies/PriceConfig.sol #2 45 function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) 46 external 47 onlyRole("price_admin") 48 { 49 PRICE.initialize(startObservations_, lastObservationTime_); 50 } 51 ... 58 function changeMovingAverageDuration(uint48 movingAverageDuration_) 59 external 60 onlyRole("price_admin") 61 { 62 PRICE.changeMovingAverageDuration(movingAverageDuration_); 63 } 64 ... 69 function changeObservationFrequency(uint48 observationFrequency_) 70 external 71 onlyRole("price_admin") 72 { 73 PRICE.changeObservationFrequency(observationFrequency_); 74: }
custodians can change the debt arbitrarily, changing the price for which users can redeem their assets:
File: /src/policies/TreasuryCustodian.sol #3 71 function increaseDebt( 72 ERC20 token_, 73 address debtor_, 74 uint256 amount_ 75 ) external onlyRole("custodian") { 76 uint256 debt = TRSRY.reserveDebt(token_, debtor_); 77 TRSRY.setDebt(token_, debtor_, debt + amount_); 78 } 79 80 function decreaseDebt( 81 ERC20 token_, 82 address debtor_, 83 uint256 amount_ 84 ) external onlyRole("custodian") { 85 uint256 debt = TRSRY.reserveDebt(token_, debtor_); 86 TRSRY.setDebt(token_, debtor_, debt - amount_); 87: }
heart_admins can change the reward to zero, causing there to be no incentive to call the heartbeat, so the TWAP becomes stale and reverts, or can set a reward token that always reverts, causing heartbeats to always revert:
File: /src/policies/Heart.sol #4 140 function setRewardTokenAndAmount(ERC20 token_, uint256 reward_) 141 external 142 onlyRole("heart_admin") 143 { 144 rewardToken = token_; 145 reward = reward_; 146 emit RewardUpdated(token_, reward_); 147: }
operator_admin can influence prices by setting a malicious contract as the auctioneer, or can assign a non-zero invalid address, so function calls always revert:
File: /src/policies/Operator.sol #5 586 function setBondContracts(IBondAuctioneer auctioneer_, IBondCallback callback_) 587 external 588 onlyRole("operator_admin") 589 { 590 if (address(auctioneer_) == address(0) || address(callback_) == address(0)) 591 revert Operator_InvalidParams(); 592 /// Set contracts 593 auctioneer = auctioneer_; 594 callback = callback_; 595: }
Code inspection
Only have one admin user which can only be assigned to a DAO, and only give the DAO permissions to do things via proposals and votes
#0 - 0xLienid
2022-09-08T17:35:57Z
true but comes with the territory of being a permissioned system. Roles will be granted to DAO associated multisigs or policies.
#1 - 0xean
2022-09-20T23:43:16Z
Some of these are intentionally designed by the system, the one that may or may not be is the ability to manipulate total supply of votes in a way that then completely controls the passing of proposals. I am going to mark this as a duplicate of #380 for that reason. The rest is QA.
11.0311 DAI - $11.03
If the oracle price feeds are insufficiently validated, there will be pricing errors leading to the miss-pricing of assets/risk
If either price becomes negative, either because of a Chainlink bug, or due to the actual price being negative (as has been seen recently for the price of oil, and with European interest rates), the unchecked cast to uint256 will result in the value being used overflowing, causing a very large value to be used:
File: /src/modules/PRICE.sol #1 161 (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData(); 162 // Use a multiple of observation frequency to determine what is too old to use. 163 // Price feeds will not provide an updated answer if the data doesn't change much. 164 // This would be similar to if the feed just stopped updating; therefore, we need a cutoff. 165 if (updatedAt < block.timestamp - 3 * uint256(observationFrequency)) 166 revert Price_BadFeed(address(_ohmEthPriceFeed)); 167 ohmEthPrice = uint256(ohmEthPriceInt); 168 169 int256 reserveEthPriceInt; 170 (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData(); 171 if (updatedAt < block.timestamp - uint256(observationFrequency)) 172 revert Price_BadFeed(address(_reserveEthPriceFeed)); 173: reserveEthPrice = uint256(reserveEthPriceInt);
Code inspection
require() that the price is non-negative, or use int256 as the price type, rather than uint256
#0 - Oighty
2022-09-06T18:54:46Z
Duplicate. See comment on #441.
š 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.3308 DAI - $54.33
| Issue | Instances | |
|---|---|---|
| [Lā01] | Function should revert if no valid action is found | 1 |
| [Lā02] | safeApprove() is deprecated | 2 |
| [Lā03] | Missing checks for address(0x0) when assigning values to address state variables | 2 |
| [Lā04] | Open TODOs | 4 |
Total: 9 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Contracts will stop working when block.timestamp overflows uint48 | 2 |
| [Nā02] | The nonReentrant modifier should occur before all other modifiers | 1 |
| [Nā03] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 2 |
| [Nā04] | public functions not called by the contract should be declared external instead | 9 |
| [Nā05] | Non-assembly method available | 1 |
| [Nā06] | constants should be defined rather than using magic numbers | 65 |
| [Nā07] | Missing event and or timelock for critical parameter change | 3 |
| [Nā08] | Constant redefined elsewhere | 4 |
| [Nā09] | Lines are too long | 8 |
| [Nā10] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 18 |
| [Nā11] | Typos | 4 |
| [Nā12] | File is missing NatSpec | 2 |
| [Nā13] | NatSpec is incomplete | 7 |
| [Nā14] | Event is missing indexed fields | 30 |
| [Nā15] | Not using the named return variables anywhere in the function is confusing | 14 |
| [Nā16] | Avoid the use of sensitive terms | 1 |
Total: 171 instances over 16 issues
There is no final else, so ActionExecuted is incorrectly emitted if the action is unknown (incorrect state handling)
There is 1 instance of this issue:
File: /src/Kernel.sol 254 } else if (action_ == Actions.MigrateKernel) { 255 ensureContract(target_); 256 _migrateKernel(Kernel(target_)); 257 } 258 259: emit ActionExecuted(action_, target_);
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.
There are 2 instances of this issue:
File: src/policies/BondCallback.sol 57: ohm.safeApprove(address(MINTR), type(uint256).max);
File: src/policies/Operator.sol 167: ohm.safeApprove(address(MINTR), type(uint256).max);
address(0x0) when assigning values to address state variablesThere are 2 instances of this issue:
File: src/Kernel.sol 251: executor = target_; 253: admin = target_;
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There are 4 instances of this issue:
File: src/policies/Operator.sol 657: /// 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?
File: src/policies/TreasuryCustodian.sol 51: // TODO Currently allows anyone to revoke any approval EXCEPT activated policies. 52: // TODO must reorg policy storage to be able to check for deactivated policies. 56: // TODO Make sure `policy_` is an actual policy and not a random address.
block.timestamp overflows uint48This will be in ~9 million years. The DAO is supposed to live forever, right? RIGHT?!
There are 2 instances of this issue:
File: /src/modules/PRICE.sol 143: lastObservationTime = uint48(block.timestamp);
File: /src/policies/Operator.sol 210: uint48(block.timestamp) >= RANGE.lastActive(true) + uint48(config_.regenWait) &&
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There is 1 instance of this issue:
File: src/policies/Operator.sol 276: ) external override onlyWhileActive nonReentrant returns (uint256 amountOut) {
override function arguments that are unused should have the variable name removed or commented out to avoid compiler warningsThere are 2 instances of this issue:
File: src/modules/VOTES.sol 45: function transfer(address to_, uint256 amount_) public pure override returns (bool) {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 9 instances of this issue:
File: src/Kernel.sol 439: function grantRole(Role role_, address addr_) public onlyAdmin { 451: function revokeRole(Role role_, address addr_) public onlyAdmin {
File: src/modules/INSTR.sol 37: function getInstructions(uint256 instructionsId_) public view returns (Instruction[] memory) {
File: src/modules/MINTR.sol 33: function mintOhm(address to_, uint256 amount_) public permissioned { 37: function burnOhm(address from_, uint256 amount_) public permissioned {
File: src/modules/RANGE.sol 215 function updateMarket( 216 bool high_, 217 uint256 market_, 218 uint256 marketCapacity_ 219: ) public permissioned {
File: src/modules/TRSRY.sol 75 function withdrawReserves( 76 address to_, 77 ERC20 token_, 78: uint256 amount_
File: src/policies/Governance.sol 145: function getMetadata(uint256 proposalId_) public view returns (ProposalMetadata memory) { 151: function getActiveProposal() public view returns (ActivatedProposal memory) {
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length
There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary
There is 1 instance of this issue:
File: src/utils/KernelUtils.sol 34: size := extcodesize(target_)
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 65 instances of this issue:
File: src/modules/PRICE.sol /// @audit 38 90: if (exponent > 38) revert Price_InvalidParams(); /// @audit 3 165: if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
File: src/modules/RANGE.sol /// @audit 3 80: uint256[3] memory rangeParams_ // [thresholdFactor, cushionSpread, wallSpread] /// @audit 10000 245: wallSpread_ > 10000 || /// @audit 100 246: wallSpread_ < 100 || /// @audit 10000 247: cushionSpread_ > 10000 || /// @audit 100 248: cushionSpread_ < 100 || /// @audit 10000 /// @audit 100 264: if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();
File: src/policies/BondCallback.sol /// @audit 4 71: requests = new Permissions[](4); /// @audit 3 75: requests[3] = Permissions(MINTR_KEYCODE, MINTR.burnOhm.selector);
File: src/policies/Governance.sol /// @audit 10000 164: if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT) /// @audit 100 217: (totalEndorsementsForProposal[proposalId_] * 100) < /// @audit 100 268: if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
File: src/policies/Operator.sol /// @audit 8 97: uint32[8] memory configParams // [cushionFactor, cushionDuration, cushionDebtBuffer, cushionDepositInterval, reserveFactor, regenWait, regenThreshold, regenObserve] /// @audit 7 103: if (configParams[1] > uint256(7 days) || configParams[1] < uint256(1 days)) /// @audit 10_000 106: if (configParams[2] < uint32(10_000)) revert Operator_InvalidParams(); /// @audit 3 /// @audit 3 108: if (configParams[3] < uint32(1 hours) || configParams[3] > configParams[1]) /// @audit 4 /// @audit 10000 /// @audit 4 /// @audit 100 111: if (configParams[4] > 10000 || configParams[4] < 100) revert Operator_InvalidParams(); /// @audit 5 114: configParams[5] < 1 hours || /// @audit 6 /// @audit 7 115: configParams[6] > configParams[7] || /// @audit 7 116: configParams[7] == uint32(0) /// @audit 7 130: observations: new bool[](configParams[7]) /// @audit 3 137: cushionDepositInterval: configParams[3], /// @audit 4 138: reserveFactor: configParams[4], /// @audit 5 139: regenWait: configParams[5], /// @audit 6 140: regenThreshold: configParams[6], /// @audit 7 141: regenObserve: configParams[7] /// @audit 3 147: emit CushionParamsChanged(configParams[1], configParams[2], configParams[3]); /// @audit 4 148: emit ReserveFactorChanged(configParams[4]); /// @audit 5 /// @audit 6 /// @audit 7 149: emit RegenParamsChanged(configParams[5], configParams[6], configParams[7]); /// @audit 4 155: dependencies = new Keycode[](4); /// @audit 3 159: dependencies[3] = toKeycode("MINTR"); /// @audit 3 164: MINTR = OlympusMinter(getModuleAddress(dependencies[3])); /// @audit 9 176: requests = new Permissions[](9); /// @audit 3 180: requests[3] = Permissions(RANGE_KEYCODE, RANGE.regenerate.selector); /// @audit 4 181: requests[4] = Permissions(RANGE_KEYCODE, RANGE.setSpreads.selector); /// @audit 5 182: requests[5] = Permissions(RANGE_KEYCODE, RANGE.setThresholdFactor.selector); /// @audit 6 183: requests[6] = Permissions(TRSRY_KEYCODE, TRSRY.setApprovalFor.selector); /// @audit 7 184: requests[7] = Permissions(MINTR_KEYCODE, MINTR.mintOhm.selector); /// @audit 8 185: requests[8] = Permissions(MINTR_KEYCODE, MINTR.burnOhm.selector); /// @audit 36 378: 36 + scaleAdjustment + int8(reserveDecimals) - int8(ohmDecimals) - priceDecimals /// @audit 36 433: 36 + scaleAdjustment + int8(ohmDecimals) - int8(reserveDecimals) - priceDecimals /// @audit 10000 /// @audit 100 518: if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams(); /// @audit 7 533: if (duration_ > uint256(7 days) || duration_ < uint256(1 days)) /// @audit 10_000 535: if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams(); /// @audit 10000 /// @audit 100 550: if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams();
File: src/policies/PriceConfig.sol /// @audit 3 31: permissions = new Permissions[](3);
File: src/utils/KernelUtils.sol /// @audit 5 43: for (uint256 i = 0; i < 5; ) { /// @audit 0x41 /// @audit 0x5A 46: if (char < 0x41 || char > 0x5A) revert InvalidKeycode(keycode_); // A-Z only /// @audit 32 58: for (uint256 i = 0; i < 32; ) { /// @audit 0x61 /// @audit 0x7A /// @audit 0x5f /// @audit 0x00 60: if ((char < 0x61 || char > 0x7A) && char != 0x5f && char != 0x00) {
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There are 3 instances of this issue:
File: src/Kernel.sol 126 function setActiveStatus(bool activate_) external onlyKernel { 127 isActive = activate_; 128: }
File: src/policies/BondCallback.sol 190 function setOperator(Operator operator_) external onlyRole("callback_admin") { 191 if (address(operator_) == address(0)) revert Callback_InvalidParams(); 192 operator = operator_; 193: }
File: src/policies/Operator.sol 586 function setBondContracts(IBondAuctioneer auctioneer_, IBondCallback callback_) 587 external 588 onlyRole("operator_admin") 589 { 590 if (address(auctioneer_) == address(0) || address(callback_) == address(0)) 591 revert Operator_InvalidParams(); 592 /// Set contracts 593 auctioneer = auctioneer_; 594 callback = callback_; 595: }
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 4 instances of this issue:
File: src/modules/RANGE.sol /// @audit seen in src/modules/MINTR.sol 68: ERC20 public immutable ohm;
File: src/policies/Operator.sol /// @audit seen in src/modules/RANGE.sol 82: ERC20 public immutable ohm; /// @audit seen in src/modules/RANGE.sol 85: ERC20 public immutable reserve; /// @audit seen in src/modules/RANGE.sol 89: uint32 public constant FACTOR_SCALE = 1e4;
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There are 8 instances of this issue:
File: src/modules/PRICE.sol 31: /// @dev Price feeds. Chainlink typically provides price feeds for an asset in ETH. Therefore, we use two price feeds against ETH, one for OHM and one for the Reserve asset, to calculate the relative price of OHM in the Reserve asset.
File: src/modules/RANGE.sol 40: uint256 spread; // Spread of the band (increase/decrease from the moving average to set the band prices), percent with 2 decimal places (i.e. 1000 = 10% spread) 46: uint256 capacity; // Amount of tokens that can be used to defend the side of the range. Specified in OHM tokens on the high side and Reserve tokens on the low side. 61: /// @notice Threshold factor for the change, a percent in 2 decimals (i.e. 1000 = 10%). Determines how much of the capacity must be spent before the side is taken down.
File: src/policies/interfaces/IOperator.sol 15: uint32 cushionDebtBuffer; // Percentage over the initial debt to allow the market to accumulate at any one time. Percent with 3 decimals, e.g. 1_000 = 1 %. See IBondAuctioneer for more info. 90: /// @param debtBuffer_ - Percentage over the initial debt to allow the market to accumulate at any one time. Percent with 3 decimals, e.g. 1_000 = 1 %. See IBondAuctioneer for more info.
File: src/policies/Operator.sol 97: uint32[8] memory configParams // [cushionFactor, cushionDuration, cushionDebtBuffer, cushionDepositInterval, reserveFactor, regenWait, regenThreshold, regenObserve] 657: /// 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?
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 18 instances of this issue:
File: src/policies/BondCallback.sol 29: OlympusTreasury public TRSRY; 30: OlympusMinter public MINTR; 68: Keycode TRSRY_KEYCODE = TRSRY.KEYCODE(); 69: Keycode MINTR_KEYCODE = MINTR.KEYCODE();
File: src/policies/Governance.sol 56: OlympusInstructions public INSTR; 57: OlympusVotes public VOTES;
File: src/policies/Heart.sol 45: OlympusPrice internal PRICE;
File: src/policies/Operator.sol 69: OlympusPrice internal PRICE; 70: OlympusRange internal RANGE; 71: OlympusTreasury internal TRSRY; 72: OlympusMinter internal MINTR; 172: Keycode RANGE_KEYCODE = RANGE.KEYCODE(); 173: Keycode TRSRY_KEYCODE = TRSRY.KEYCODE(); 174: Keycode MINTR_KEYCODE = MINTR.KEYCODE();
File: src/policies/PriceConfig.sol 11: OlympusPrice internal PRICE;
File: src/policies/TreasuryCustodian.sol 20: OlympusTreasury internal TRSRY; 35: Keycode TRSRY_KEYCODE = TRSRY.KEYCODE();
File: src/policies/VoterRegistration.sol 10: OlympusVotes public VOTES;
There are 4 instances of this issue:
File: src/interfaces/IBondCallback.sol /// @audit _callback 13: /// @dev Should check that the quote tokens have been transferred to the contract in the _callback function
File: src/modules/PRICE.sol /// @audit numbe 126: // Cache numbe of observations to save gas.
File: src/policies/Operator.sol /// @audit deactive 295: /// If wall is down after swap, deactive the cushion as well /// @audit deactive 326: /// If wall is down after swap, deactive the cushion as well
There are 2 instances of this issue:
File: src/policies/TreasuryCustodian.sol
File: src/utils/KernelUtils.sol
There are 7 instances of this issue:
File: src/modules/RANGE.sol /// @audit Missing: '@return' 279 /// @notice Get the capacity for a side of the range. 280 /// @param high_ - Specifies the side of the range to get capacity for (true = high side, false = low side). 281: function capacity(bool high_) external view returns (uint256) { /// @audit Missing: '@return' 289 /// @notice Get the status of a side of the range (whether it is active or not). 290 /// @param high_ - Specifies the side of the range to get status for (true = high side, false = low side). 291: function active(bool high_) external view returns (bool) { /// @audit Missing: '@return' 300 /// @param wall_ - Specifies the band to get the price for (true = wall, false = cushion). 301 /// @param high_ - Specifies the side of the range to get the price for (true = high side, false = low side). 302: function price(bool wall_, bool high_) external view returns (uint256) { /// @audit Missing: '@return' 318 /// @notice Get the spread for the wall or cushion band. 319 /// @param wall_ - Specifies the band to get the spread for (true = wall, false = cushion). 320: function spread(bool wall_) external view returns (uint256) { /// @audit Missing: '@return' 328 /// @notice Get the market ID for a side of the range. 329 /// @param high_ - Specifies the side of the range to get market for (true = high side, false = low side). 330: function market(bool high_) external view returns (uint256) { /// @audit Missing: '@return' 338 /// @notice Get the timestamp when the range was last active. 339 /// @param high_ - Specifies the side of the range to get timestamp for (true = high side, false = low side). 340: function lastActive(bool high_) external view returns (uint256) {
File: src/policies/interfaces/IOperator.sol /// @audit Missing: '@return' 141 /// @dev Calculates the capacity to deploy for a wall based on the amount of reserves owned by the treasury and the reserve factor. 142 /// @param high_ - Whether to return the full capacity for the high or low wall 143: function fullCapacity(bool high_) external view returns (uint256);
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 30 instances of this issue:
File: src/Kernel.sol 203 event PermissionsUpdated( 204 Keycode indexed keycode_, 205 Policy indexed policy_, 206 bytes4 funcSelector_, 207 bool granted_ 208: );
File: src/modules/INSTR.sol 11: event InstructionsStored(uint256 instructionsId);
File: src/modules/PRICE.sol 26: event NewObservation(uint256 timestamp_, uint256 price_, uint256 movingAverage_); 27: event MovingAverageDurationChanged(uint48 movingAverageDuration_); 28: event ObservationFrequencyChanged(uint48 observationFrequency_);
File: src/modules/RANGE.sol 20: event WallUp(bool high_, uint256 timestamp_, uint256 capacity_); 21: event WallDown(bool high_, uint256 timestamp_, uint256 capacity_); 22: event CushionUp(bool high_, uint256 timestamp_, uint256 capacity_); 23: event CushionDown(bool high_, uint256 timestamp_); 24 event PricesChanged( 25 uint256 wallLowPrice_, 26 uint256 cushionLowPrice_, 27 uint256 cushionHighPrice_, 28 uint256 wallHighPrice_ 29: ); 30: event SpreadsChanged(uint256 cushionSpread_, uint256 wallSpread_); 31: event ThresholdFactorChanged(uint256 thresholdFactor_);
File: src/modules/TRSRY.sol 20: event ApprovedForWithdrawal(address indexed policy_, ERC20 indexed token_, uint256 amount_); 27: event DebtIncurred(ERC20 indexed token_, address indexed policy_, uint256 amount_); 28: event DebtRepaid(ERC20 indexed token_, address indexed policy_, uint256 amount_); 29: event DebtSet(ERC20 indexed token_, address indexed policy_, uint256 amount_);
File: src/policies/Governance.sol 86: event ProposalSubmitted(uint256 proposalId); 87: event ProposalEndorsed(uint256 proposalId, address voter, uint256 amount); 88: event ProposalActivated(uint256 proposalId, uint256 timestamp); 89: event WalletVoted(uint256 proposalId, address voter, bool for_, uint256 userVotes); 90: event ProposalExecuted(uint256 proposalId);
File: src/policies/Heart.sol 28: event Beat(uint256 timestamp_); 29: event RewardIssued(address to_, uint256 rewardAmount_); 30: event RewardUpdated(ERC20 token_, uint256 rewardAmount_);
File: src/policies/Operator.sol 45 event Swap( 46 ERC20 indexed tokenIn_, 47 ERC20 indexed tokenOut_, 48 uint256 amountIn_, 49 uint256 amountOut_ 50: ); 51: event CushionFactorChanged(uint32 cushionFactor_); 52: event CushionParamsChanged(uint32 duration_, uint32 debtBuffer_, uint32 depositInterval_); 53: event ReserveFactorChanged(uint32 reserveFactor_); 54: event RegenParamsChanged(uint32 wait_, uint32 threshold_, uint32 observe_);
File: src/policies/TreasuryCustodian.sol 17: event ApprovalRevoked(address indexed policy_, ERC20[] tokens_);
Consider changing the variable to be an unnamed one
There are 14 instances of this issue:
File: src/modules/INSTR.sol /// @audit major /// @audit minor 28: function VERSION() public pure override returns (uint8 major, uint8 minor) {
File: src/modules/MINTR.sol /// @audit major /// @audit minor 25: function VERSION() external pure override returns (uint8 major, uint8 minor) {
File: src/modules/PRICE.sol /// @audit major /// @audit minor 113: function VERSION() external pure override returns (uint8 major, uint8 minor) {
File: src/modules/RANGE.sol /// @audit major /// @audit minor 115: function VERSION() external pure override returns (uint8 major, uint8 minor) {
File: src/modules/TRSRY.sol /// @audit major /// @audit minor 51: function VERSION() external pure override returns (uint8 major, uint8 minor) {
File: src/modules/VOTES.sol /// @audit major /// @audit minor 27: function VERSION() external pure override returns (uint8 major, uint8 minor) {
File: src/policies/BondCallback.sol /// @audit in_ /// @audit out_ 173 function amountsForMarket(uint256 id_) 174 external 175 view 176 override 177: returns (uint256 in_, uint256 out_)
Use alternative variants, e.g. allowlist/denylist instead of whitelist/blacklist
There is 1 instance of this issue:
File: /src/policies/BondCallback.sol 83: function whitelist(address teller_, uint256 id_)
š 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
40.4947 DAI - $40.49
| Issue | Instances | |
|---|---|---|
| [Gā01] | State variables should only be updated once in a function | 1 |
| [Gā02] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 1 |
| [Gā03] | State variables only set in the constructor should be declared immutable | 11 |
| [Gā04] | State variables can be packed into fewer storage slots | 1 |
| [Gā05] | Using calldata instead of memory for read-only arguments in external functions saves gas | 5 |
| [Gā06] | Using storage instead of memory for structs/arrays saves gas | 4 |
| [Gā07] | State variables should be cached in stack variables rather than re-reading them from storage | 7 |
| [Gā08] | Multiple accesses of a mapping/array should use a local variable cache | 1 |
| [Gā09] | The result of function calls should be cached rather than re-calling the function | 3 |
| [Gā10] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 3 |
| [Gā11] | internal functions only called once can be inlined to save gas | 9 |
| [Gā12] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 1 |
| [Gā13] | <array>.length should not be looked up in every loop of a for-loop | 1 |
| [Gā14] | Optimize names to save gas | 20 |
| [Gā15] | Using bools for storage incurs overhead | 11 |
| [Gā16] | Use a more recent version of solidity | 3 |
| [Gā17] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 7 |
| [Gā18] | Using private rather than public for constants, saves gas | 11 |
| [Gā19] | Don't compare boolean expressions to boolean literals | 2 |
| [Gā20] | Division by two should use bit shifting | 2 |
| [Gā21] | Superfluous event fields | 7 |
| [Gā22] | Functions guaranteed to revert when called by normal users can be marked payable | 36 |
Total: 147 instances over 22 issues
The source diffs can be directly applied to the code by putting the diff block into a file then doing git apply <file>
totalEndorsementsForProposal is updated twice in this function, but it could be optimized to only update the difference between previousEndorsement and userVotes instead. Futher optimizations would be to use a storage variable rather than looking up the hash of totalEndorsementsForProposal[proposalId_] each time, and to use x = x + a rather than x += a
There is 1 instance of this issue:
File: /src/policies/Governance.sol 192 // undo any previous endorsement the user made on these instructions 193 uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender]; 194 totalEndorsementsForProposal[proposalId_] -= previousEndorsement; 195 196 // reapply user endorsements with most up-to-date votes 197 userEndorsementsForProposal[proposalId_][msg.sender] = userVotes; 198: totalEndorsementsForProposal[proposalId_] += userVotes;
diff --git a/src/policies/Governance.sol b/src/policies/Governance.sol index 8829e3b..c0e783f 100644 --- a/src/policies/Governance.sol +++ b/src/policies/Governance.sol @@ -191,11 +191,14 @@ contract OlympusGovernance is Policy { // undo any previous endorsement the user made on these instructions uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender]; - totalEndorsementsForProposal[proposalId_] -= previousEndorsement; // reapply user endorsements with most up-to-date votes userEndorsementsForProposal[proposalId_][msg.sender] = userVotes; - totalEndorsementsForProposal[proposalId_] += userVotes; + if (previousEndorsement > userVotes) { + totalEndorsementsForProposal[proposalId_] -= (previousEndorsement - userVotes); + } else { + totalEndorsementsForProposal[proposalId_] += (userVotes - previousEndorsement); + } emit ProposalEndorsed(proposalId_, msg.sender, userVotes); }
Note that the numbers below are an underreporting of the gas changes due to this forge issue
diff --git a/tmp/gas_before b/tmp/gas_after index c793374..828386f 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -289,7 +289,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 1638243 ā 8250 ā ā ā ā ā +ā 1642043 ā 8269 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -297,7 +297,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā configureDependencies ā 47868 ā 48513 ā 47868 ā 51868 ā 31 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā endorseProposal ā 6874 ā 39015 ā 30774 ā 52674 ā 43 ā +ā endorseProposal ā 6476 ā 38636 ā 30376 ā 52276 ā 43 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā executeProposal ā 1850 ā 171376 ā 238748 ā 238748 ā 7 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There is 1 instance of this issue:
File: src/policies/Governance.sol 96 mapping(uint256 => ProposalMetadata) public getProposalMetadata; 97 98 /// @notice Return the total endorsements for a proposal id. 99 mapping(uint256 => uint256) public totalEndorsementsForProposal; 100 101 /// @notice Return the number of endorsements a user has given a proposal id. 102 mapping(uint256 => mapping(address => uint256)) public userEndorsementsForProposal; 103 104 /// @notice Return whether a proposal id has been activated. Once this is true, it should never be flipped false. 105 mapping(uint256 => bool) public proposalHasBeenActivated; 106 107 /// @notice Return the total yes votes for a proposal id used in calculating net votes. 108 mapping(uint256 => uint256) public yesVotesForProposal; 109 110 /// @notice Return the total no votes for a proposal id used in calculating net votes. 111 mapping(uint256 => uint256) public noVotesForProposal; 112 113 /// @notice Return the amount of votes a user has applied to a proposal id. This does not record how the user voted. 114 mapping(uint256 => mapping(address => uint256)) public userVotesForProposal; 115 116 /// @notice Return the amount of tokens reclaimed by a user after voting on a proposal id. 117: mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
There are 11 instances of this issue:
File: src/policies/BondCallback.sol /// @audit aggregator (constructor) 43: aggregator = aggregator_; /// @audit aggregator (access) 91: (, , ERC20 payoutToken, , , ) = aggregator.getAuctioneer(id_).getMarketInfoForPurchase(id_); /// @audit aggregator (access) 109: (, , ERC20 payoutToken, ERC20 quoteToken, , ) = aggregator /// @audit ohm (constructor) 44: ohm = ohm_; /// @audit ohm (access) 57: ohm.safeApprove(address(MINTR), type(uint256).max); /// @audit ohm (access) 94: if (address(payoutToken) != address(ohm)) { /// @audit ohm (access) 118: if (quoteToken == payoutToken && quoteToken == ohm) { /// @audit ohm (access) 125: } else if (quoteToken == ohm) { /// @audit ohm (access) 131: } else if (payoutToken == ohm) {
File: src/policies/Heart.sol /// @audit _operator (constructor) 60: _operator = operator_; /// @audit _operator (access) 100: _operator.operate();
diff --git a/src/policies/BondCallback.sol b/src/policies/BondCallback.sol index 4da1a3a..4383f7e 100644 --- a/src/policies/BondCallback.sol +++ b/src/policies/BondCallback.sol @@ -25,11 +25,11 @@ contract BondCallback is Policy, ReentrancyGuard, IBondCallback { mapping(uint256 => uint256[2]) internal _amountsPerMarket; mapping(ERC20 => uint256) public priorBalances; - IBondAggregator public aggregator; + IBondAggregator immutable public aggregator; OlympusTreasury public TRSRY; OlympusMinter public MINTR; Operator public operator; - ERC20 public ohm; + ERC20 immutable public ohm; /*////////////////////////////////////////////////////////////// POLICY INTERFACE diff --git a/src/policies/Heart.sol b/src/policies/Heart.sol index 7693dba..b0a46f2 100644 --- a/src/policies/Heart.sol +++ b/src/policies/Heart.sol @@ -45,7 +45,7 @@ contract OlympusHeart is IHeart, Policy, ReentrancyGuard { OlympusPrice internal PRICE; // Policies - IOperator internal _operator; + IOperator immutable internal _operator; /*////////////////////////////////////////////////////////////// POLICY INTERFACE
Note that the numbers below are an underreporting of the gas changes due to this forge issue
diff --git a/tmp/gas_before b/tmp/gas_after index c793374..1cea2f8 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -24,7 +24,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā allKeycodes ā 706 ā 706 ā 706 ā 706 ā 2 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā executeAction ā 649 ā 156798 ā 94565 ā 595110 ā 767 ā +ā executeAction ā 649 ā 156789 ā 94565 ā 595110 ā 767 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā executor ā 2393 ā 2393 ā 2393 ā 2393 ā 1 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -258,7 +258,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 1408325 ā 6934 ā ā ā ā ā +ā 1417471 ā 7248 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -268,9 +268,9 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā batchToTreasury ā 4111 ā 12729 ā 12068 ā 22668 ā 4 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā callback ā 7678 ā 98980 ā 85627 ā 187927 ā 17 ā +ā callback ā 7678 ā 97563 ā 85333 ā 183633 ā 17 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā configureDependencies ā 73564 ā 73564 ā 73564 ā 73564 ā 63 ā +ā configureDependencies ā 73464 ā 73464 ā 73464 ā 73464 ā 63 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā isActive ā 395 ā 395 ā 395 ā 395 ā 63 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -282,7 +282,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā setOperator ā 8227 ā 23461 ā 23865 ā 23865 ā 65 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā whitelist ā 8221 ā 39608 ā 34675 ā 67005 ā 46 ā +ā whitelist ā 8221 ā 38018 ā 32143 ā 62802 ā 46 ā ā°āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā“āāāāāāāāāāāāāāāāāā“āāāāāāāā“āāāāāāāāā“āāāāāāāāā“āāāāāāāāāāÆ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¬āāāāāāāāāāāāāāāāāā¬āāāāāāāāā¬āāāāāāāāā¬āāāāāāāāā¬āāāāāāāāāā® ā src/policies/Governance.sol:OlympusGovernance contract ā ā ā ā ā ā @@ -336,13 +336,13 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā 934119 ā 4277 ā ā ā ā ā +ā 914213 ā 4290 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā active ā 323 ā 989 ā 323 ā 2323 ā 3 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā beat ā 5429 ā 29228 ā 18552 ā 61386 ā 8 ā +ā beat ā 5429 ā 28154 ā 17478 ā 59238 ā 8 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā configureDependencies ā 24123 ā 24123 ā 24123 ā 24123 ā 11 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ @@ -397,7 +397,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā isActive ā 439 ā 439 ā 439 ā 439 ā 63 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā operate ā 387 ā 122263 ā 37958 ā 640609 ā 104 ā +ā operate ā 387 ā 121697 ā 37958 ā 636406 ā 104 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā regenerate ā 3772 ā 17622 ā 21791 ā 29292 ā 6 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -577,7 +577,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā getFee ā 872 ā 3538 ā 4872 ā 4872 ā 6 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā purchase ā 228899 ā 239488 ā 239488 ā 250077 ā 2 ā +ā purchase ā 228739 ā 239261 ā 239261 ā 249783 ā 2 ā ā°āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā“āāāāāāāāāāāāāāāāāā“āāāāāāāāā“āāāāāāāāā“āāāāāāāāā“āāāāāāāāāāÆ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¬āāāāāāāāāāāāāāāāāā¬āāāāāāāā¬āāāāāāāāā¬āāāāāāāā¬āāāāāāāāāā® ā src/test/mocks/KernelTestMocks.sol:MockModule contract ā ā ā ā ā ā
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper
There is 1 instance of this issue:
File: src/policies/Heart.sol /// @audit Variable ordering with 5 slots instead of the current 6: /// uint256(32):lastBeat, uint256(32):reward, user-defined(20):rewardToken, bool(1):active, address(20):PRICE, address(20):_operator 33: bool public active;
diff --git a/src/policies/Heart.sol b/src/policies/Heart.sol index 7693dba..ced0bcb 100644 --- a/src/policies/Heart.sol +++ b/src/policies/Heart.sol @@ -29,9 +29,6 @@ contract OlympusHeart is IHeart, Policy, ReentrancyGuard { event RewardIssued(address to_, uint256 rewardAmount_); event RewardUpdated(ERC20 token_, uint256 rewardAmount_); - /// @notice Status of the Heart, false = stopped, true = beating - bool public active; - /// @notice Timestamp of the last beat (UTC, in seconds) uint256 public lastBeat; @@ -41,6 +38,9 @@ contract OlympusHeart is IHeart, Policy, ReentrancyGuard { /// @notice Reward token address that users are sent for beating the Heart ERC20 public rewardToken; + /// @notice Status of the Heart, false = stopped, true = beating + bool public active; + // Modules OlympusPrice internal PRICE;
Note that the numbers below are an underreporting of the gas changes due to this forge issue
diff --git a/tmp/gas_before b/tmp/gas_after index c793374..9682b00 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -336,13 +336,13 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā 934119 ā 4277 ā ā ā ā ā +ā 917440 ā 4309 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā active ā 323 ā 989 ā 323 ā 2323 ā 3 ā +ā active ā 340 ā 1006 ā 340 ā 2340 ā 3 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā beat ā 5429 ā 29228 ā 18552 ā 61386 ā 8 ā +ā beat ā 5443 ā 28492 ā 18566 ā 59400 ā 8 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā configureDependencies ā 24123 ā 24123 ā 24123 ā 24123 ā 11 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ @@ -362,7 +362,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā setRewardTokenAndAmount ā 8222 ā 13892 ā 13892 ā 19562 ā 2 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā toggleBeat ā 1400 ā 7187 ā 8455 ā 10440 ā 4 ā +ā toggleBeat ā 1427 ā 8416 ā 9577 ā 13083 ā 4 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā withdrawUnspentRewards ā 8201 ā 19621 ā 19621 ā 31041 ā 2 ā ā°āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā“āāāāāāāāāāāāāāāāāā“āāāāāāāā“āāāāāāāāā“āāāāāāāā“āāāāāāāāāāÆ
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 5 instances of this issue:
File: src/modules/PRICE.sol /// @audit startObservations_ 205 function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) 206 external 207: permissioned
File: src/policies/BondCallback.sol /// @audit tokens_ 152: function batchToTreasury(ERC20[] memory tokens_) external onlyRole("callback_admin") {
File: src/policies/Governance.sol /// @audit proposalURI_ 159 function submitProposal( 160 Instruction[] calldata instructions_, 161 bytes32 title_, 162: string memory proposalURI_
File: src/policies/PriceConfig.sol /// @audit startObservations_ 45 function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) 46 external 47: onlyRole("price_admin")
File: src/policies/TreasuryCustodian.sol /// @audit tokens_ 53: function revokePolicyApprovals(address policy_, ERC20[] memory tokens_) external {
diff --git a/src/modules/PRICE.sol b/src/modules/PRICE.sol index 55d85d3..5a620d7 100644 --- a/src/modules/PRICE.sol +++ b/src/modules/PRICE.sol @@ -202,7 +202,7 @@ contract OlympusPrice is Module { /// @param lastObservationTime_ - Unix timestamp of last observation being provided (in seconds). /// @dev This function must be called after the Price module is deployed to activate it and after updating the observationFrequency /// or movingAverageDuration (in certain cases) in order for the Price module to function properly. - function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) + function initialize(uint256[] calldata startObservations_, uint48 lastObservationTime_) external permissioned { diff --git a/src/policies/BondCallback.sol b/src/policies/BondCallback.sol index 4da1a3a..902bcfc 100644 --- a/src/policies/BondCallback.sol +++ b/src/policies/BondCallback.sol @@ -149,7 +149,7 @@ contract BondCallback is Policy, ReentrancyGuard, IBondCallback { /// @notice Send tokens to the TRSRY in a batch /// @param tokens_ - Array of tokens to send - function batchToTreasury(ERC20[] memory tokens_) external onlyRole("callback_admin") { + function batchToTreasury(ERC20[] calldata tokens_) external onlyRole("callback_admin") { ERC20 token; uint256 balance; uint256 len = tokens_.length; diff --git a/src/policies/Governance.sol b/src/policies/Governance.sol index 8829e3b..a39dad0 100644 --- a/src/policies/Governance.sol +++ b/src/policies/Governance.sol @@ -159,7 +159,7 @@ contract OlympusGovernance is Policy { function submitProposal( Instruction[] calldata instructions_, bytes32 title_, - string memory proposalURI_ + string calldata proposalURI_ ) external { if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT) revert NotEnoughVotesToPropose(); diff --git a/src/policies/PriceConfig.sol b/src/policies/PriceConfig.sol index 78887fd..214e0ab 100644 --- a/src/policies/PriceConfig.sol +++ b/src/policies/PriceConfig.sol @@ -42,7 +42,7 @@ contract OlympusPriceConfig is Policy { /// @param lastObservationTime_ Unix timestamp of last observation being provided (in seconds). /// @dev This function must be called after the Price module is deployed to activate it and after updating the observationFrequency /// or movingAverageDuration (in certain cases) in order for the Price module to function properly. - function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) + function initialize(uint256[] calldata startObservations_, uint48 lastObservationTime_) external onlyRole("price_admin") { diff --git a/src/policies/TreasuryCustodian.sol b/src/policies/TreasuryCustodian.sol index 1c12f2e..a240cd1 100644 --- a/src/policies/TreasuryCustodian.sol +++ b/src/policies/TreasuryCustodian.sol @@ -50,7 +50,7 @@ contract TreasuryCustodian is Policy { // Anyone can call to revoke a deactivated policy's approvals. // TODO Currently allows anyone to revoke any approval EXCEPT activated policies. // TODO must reorg policy storage to be able to check for deactivated policies. - function revokePolicyApprovals(address policy_, ERC20[] memory tokens_) external { + function revokePolicyApprovals(address policy_, ERC20[] calldata tokens_) external { if (Policy(policy_).isActive()) revert PolicyStillActive(); // TODO Make sure `policy_` is an actual policy and not a random address.
Note that the numbers below are an underreporting of the gas changes due to this forge issue
diff --git a/tmp/gas_before b/tmp/gas_after index c793374..513078c 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -108,7 +108,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 1117743 ā 6851 ā ā ā ā ā +ā 1101930 ā 6772 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -128,7 +128,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā getMovingAverage ā 544 ā 812 ā 544 ā 2420 ā 7 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā initialize ā 4063 ā 432495 ā 512316 ā 886622 ā 24 ā +ā initialize ā 2327 ā 430562 ā 510516 ā 883159 ā 24 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā initialized ā 340 ā 955 ā 340 ā 2340 ā 13 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -258,7 +258,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 1408325 ā 6934 ā ā ā ā ā +ā 1386305 ā 6824 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -266,7 +266,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā approvedMarkets ā 685 ā 685 ā 685 ā 685 ā 2 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā batchToTreasury ā 4111 ā 12729 ā 12068 ā 22668 ā 4 ā +ā batchToTreasury ā 3800 ā 12543 ā 11920 ā 22533 ā 4 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā callback ā 7678 ā 98980 ā 85627 ā 187927 ā 17 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -289,7 +289,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 1638243 ā 8250 ā ā ā ā ā +ā 1645850 ā 8288 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -317,7 +317,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā setActiveStatus ā 777 ā 1228 ā 777 ā 3577 ā 31 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā submitProposal ā 11529 ā 176506 ā 187247 ā 187247 ā 25 ā +ā submitProposal ā 11364 ā 176510 ā 187258 ā 187258 ā 25 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā tokenClaimsForProposal ā 684 ā 684 ā 684 ā 684 ā 1 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -430,7 +430,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 639600 ā 3262 ā ā ā ā ā +ā 620181 ā 3165 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -440,7 +440,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā configureDependencies ā 24144 ā 24144 ā 24144 ā 24144 ā 5 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā initialize ā 10107 ā 491657 ā 524236 ā 895872 ā 7 ā +ā initialize ā 8371 ā 486274 ā 519133 ā 885906 ā 7 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā isActive ā 317 ā 317 ā 317 ā 317 ā 5 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -453,7 +453,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāŖāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā 739696 ā 3762 ā ā ā ā ā +ā 719277 ā 3660 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ @@ -469,7 +469,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā requestPermissions ā 2061 ā 2477 ā 2061 ā 4561 ā 6 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ -ā revokePolicyApprovals ā 6956 ā 6956 ā 6956 ā 6956 ā 1 ā +ā revokePolicyApprovals ā 6842 ā 6842 ā 6842 ā 6842 ā 1 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāā¼āāāāāāāāā¼āāāāāāāā¼āāāāāāāāāā¤ ā setActiveStatus ā 733 ā 733 ā 733 ā 733 ā 6 ā ā°āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā“āāāāāāāāāāāāāāāāāā“āāāāāāāā“āāāāāāāāā“āāāāāāāā“āāāāāāāāāāÆ
storage instead of memory for structs/arrays saves gasWhen fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
There are 4 instances of this issue:
File: src/policies/Operator.sol 206: Config memory config_ = _config; 385: Config memory config_ = _config; 440: Config memory config_ = _config; 666: Regen memory regen = _status.low;
diff --git a/src/policies/Operator.sol b/src/policies/Operator.sol index 7573526..e3c5c53 100644 --- a/src/policies/Operator.sol +++ b/src/policies/Operator.sol @@ -203,7 +203,7 @@ contract Operator is IOperator, Policy, ReentrancyGuard { _updateCapacity(false, 0); /// Cache config in memory - Config memory config_ = _config; + Config storage config_ = _config; /// Check if walls can regenerate capacity if ( @@ -437,7 +437,7 @@ contract Operator is IOperator, Policy, ReentrancyGuard { uint256 minimumPrice = invCushionPrice.mulDiv(bondScale, oracleScale); /// Cache config struct to avoid multiple SLOADs - Config memory config_ = _config; + Config storage config_ = _config; /// Calculate market capacity from the cushion factor uint256 marketCapacity = range.low.capacity.mulDiv(config_.cushionFactor, FACTOR_SCALE); @@ -663,7 +663,7 @@ contract Operator is IOperator, Policy, ReentrancyGuard { /// Update low side regen status with a new observation /// Observation is positive if the current price is greater than the MA uint32 observe = _config.regenObserve; - Regen memory regen = _status.low; + Regen storage regen = _status.low; if (currentPrice >= movingAverage) { if (!regen.observations[regen.nextObservation]) { _status.low.observations[regen.nextObservation] = true;
diff --git a/tmp/gas_before b/tmp/gas_after index c793374..0d24e75 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -371,7 +371,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāŖāāāāāāāāāāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāŖāāāāāāāāāā” ā Deployment Cost ā Deployment Size ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā 4679925 ā 25703 ā ā ā ā ā +ā 4594613 ā 25277 ā ā ā ā ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā Function Name ā min ā avg ā median ā max ā # calls ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ @@ -397,7 +397,7 @@ āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā isActive ā 439 ā 439 ā 439 ā 439 ā 63 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ -ā operate ā 387 ā 122263 ā 37958 ā 640609 ā 104 ā +ā operate ā 387 ā 118525 ā 34414 ā 636359 ā 104 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤ ā regenerate ā 3772 ā 17622 ā 21791 ā 29292 ā 6 ā āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā¼āāāāāāāāāāāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāā¼āāāāāāāāāā¤
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 7 instances of this issue:
File: src/modules/PRICE.sol /// @audit _movingAverage on line 138 146: emit NewObservation(block.timestamp, currentPrice, _movingAverage); /// @audit nextObsIndex on line 185 185: uint32 lastIndex = nextObsIndex == 0 ? numObservations - 1 : nextObsIndex - 1; /// @audit numObservations on line 97 100: observations = new uint256[](numObservations); /// @audit observationFrequency on line 165 171: if (updatedAt < block.timestamp - uint256(observationFrequency)) /// @audit observationFrequency on line 242 246: uint256 newObservations = uint256(movingAverageDuration_ / observationFrequency); /// @audit movingAverageDuration on line 268 272: uint256 newObservations = uint256(movingAverageDuration / observationFrequency_);
File: src/policies/Heart.sol /// @audit reward on line 112 113: emit RewardIssued(to_, reward);
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata
There is 1 instance of this issue:
File: src/Kernel.sol /// @audit moduleDependents[keycode] on line 309 310: getDependentIndex[keycode][policy_] = moduleDependents[keycode].length - 1;
The instances below point to the second+ call of the function within a single function
There are 3 instances of this issue:
File: src/policies/PriceConfig.sol /// @audit PRICE.KEYCODE() on line 32 33: permissions[1] = Permissions(PRICE.KEYCODE(), PRICE.changeMovingAverageDuration.selector); /// @audit PRICE.KEYCODE() on line 32 34: permissions[2] = Permissions(PRICE.KEYCODE(), PRICE.changeObservationFrequency.selector);
File: src/policies/VoterRegistration.sol /// @audit VOTES.KEYCODE() on line 34 35: permissions[1] = Permissions(VOTES.KEYCODE(), VOTES.burnFrom.selector);
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 3 instances of this issue:
File: src/modules/PRICE.sol 136: _movingAverage += (currentPrice - earliestPrice) / numObs; 138: _movingAverage -= (earliestPrice - currentPrice) / numObs;
File: src/policies/Heart.sol 103: lastBeat += frequency();
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 9 instances of this issue:
File: src/Kernel.sol 266: function _installModule(Module newModule_) internal { 279: function _upgradeModule(Module newModule_) internal { 295: function _activatePolicy(Policy policy_) internal { 325: function _deactivatePolicy(Policy policy_) internal { 351: function _migrateKernel(Kernel newKernel_) internal { 378: function _reconfigurePolicies(Keycode keycode_) internal { 409: function _pruneFromDependents(Policy policy_) internal {
File: src/policies/Heart.sol 111: function _issueReward(address to_) internal {
File: src/policies/Operator.sol 652 function _addObservation() internal { 653 /// Get latest moving average from the price module 654: uint256 movingAverage = PRICE.getMovingAverage();
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There is 1 instance of this issue:
File: src/modules/PRICE.sol /// @audit if-condition on line 135 136: _movingAverage += (currentPrice - earliestPrice) / numObs;
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There is 1 instance of this issue:
File: src/policies/Governance.sol 278: for (uint256 step; step < instructions.length; ) {
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 20 instances of this issue:
File: src/interfaces/IBondCallback.sol /// @audit callback(), amountsForMarket(), whitelist() 6: interface IBondCallback {
File: src/Kernel.sol /// @audit changeKernel() 62: abstract contract KernelAdapter { /// @audit KEYCODE(), VERSION(), INIT() 84: abstract contract Module is KernelAdapter { /// @audit setActiveStatus(), configureDependencies(), requestPermissions() 111: abstract contract Policy is KernelAdapter { /// @audit executeAction(), grantRole(), revokeRole() 149: contract Kernel {
File: src/modules/INSTR.sol /// @audit getInstructions(), store() 10: contract OlympusInstructions is Module {
File: src/modules/MINTR.sol /// @audit mintOhm(), burnOhm() 8: contract OlympusMinter is Module {
File: src/modules/PRICE.sol /// @audit updateMovingAverage(), getCurrentPrice(), getLastPrice(), getMovingAverage(), initialize(), changeMovingAverageDuration(), changeObservationFrequency() 22: contract OlympusPrice is Module {
File: src/modules/RANGE.sol /// @audit updateCapacity(), updatePrices(), regenerate(), updateMarket(), setSpreads(), setThresholdFactor(), range(), capacity(), active(), price(), spread(), market(), lastActive() 16: contract OlympusRange is Module {
File: src/modules/TRSRY.sol /// @audit getReserveBalance(), setApprovalFor(), withdrawReserves(), getLoan(), repayLoan(), setDebt() 17: contract OlympusTreasury is Module, ReentrancyGuard {
File: src/modules/VOTES.sol /// @audit mintTo(), burnFrom() 11: contract OlympusVotes is Module, ERC20 {
File: src/policies/BondCallback.sol /// @audit batchToTreasury(), setOperator() 17: contract BondCallback is Policy, ReentrancyGuard, IBondCallback {
File: src/policies/Governance.sol /// @audit getMetadata(), getActiveProposal(), submitProposal(), endorseProposal(), activateProposal(), vote(), executeProposal(), reclaimVotes() 51: contract OlympusGovernance is Policy {
File: src/policies/Heart.sol /// @audit beat(), frequency(), resetBeat(), toggleBeat(), setRewardTokenAndAmount(), withdrawUnspentRewards() 21: contract OlympusHeart is IHeart, Policy, ReentrancyGuard {
File: src/policies/interfaces/IHeart.sol /// @audit beat(), frequency(), resetBeat(), toggleBeat(), setRewardTokenAndAmount(), withdrawUnspentRewards() 6: interface IHeart {
File: src/policies/interfaces/IOperator.sol /// @audit operate(), swap(), getAmountOut(), setSpreads(), setThresholdFactor(), setCushionFactor(), setCushionParams(), setReserveFactor(), setRegenParams(), setBondContracts(), initialize(), regenerate(), toggleActive(), fullCapacity(), status(), config() 8: interface IOperator {
File: src/policies/Operator.sol /// @audit bondPurchase(), setSpreads(), setThresholdFactor(), setCushionFactor(), setCushionParams(), setReserveFactor(), setRegenParams(), setBondContracts(), initialize(), regenerate(), toggleActive(), getAmountOut() 30: contract Operator is IOperator, Policy, ReentrancyGuard {
File: src/policies/PriceConfig.sol /// @audit initialize(), changeMovingAverageDuration(), changeObservationFrequency() 7: contract OlympusPriceConfig is Policy {
File: src/policies/TreasuryCustodian.sol /// @audit grantApproval(), revokePolicyApprovals(), increaseDebt(), decreaseDebt() 15: contract TreasuryCustodian is Policy {
File: src/policies/VoterRegistration.sol /// @audit issueVotesTo(), revokeVotesFrom() 9: contract VoterRegistration is Policy {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 11 instances of this issue:
File: src/Kernel.sol 113: bool public isActive; 181: mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions; 194: mapping(address => mapping(Role => bool)) public hasRole; 197: mapping(Role => bool) public isRole;
File: src/modules/PRICE.sol 62: bool public initialized;
File: src/policies/BondCallback.sol 24: mapping(address => mapping(uint256 => bool)) public approvedMarkets;
File: src/policies/Governance.sol 105: mapping(uint256 => bool) public proposalHasBeenActivated; 117: mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;
File: src/policies/Heart.sol 33: bool public active;
File: src/policies/Operator.sol 63: bool public initialized; 66: bool public active;
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 3 instances of this issue:
File: src/interfaces/IBondCallback.sol 2: pragma solidity >=0.8.0;
File: src/policies/interfaces/IHeart.sol 2: pragma solidity >=0.8.0;
File: src/policies/interfaces/IOperator.sol 2: pragma solidity >=0.8.0;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 7 instances of this issue:
File: src/policies/Operator.sol 488: decimals++; 670: _status.low.count++; 675: _status.low.count--; 686: _status.high.count++; 691: _status.high.count--;
File: src/utils/KernelUtils.sol 49: i++; 64: i++;
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 11 instances of this issue:
File: src/modules/PRICE.sol 59: uint8 public constant decimals = 18;
File: src/modules/RANGE.sol 65: uint256 public constant FACTOR_SCALE = 1e4;
File: src/policies/Governance.sol 121: uint256 public constant SUBMISSION_REQUIREMENT = 100; 124: uint256 public constant ACTIVATION_DEADLINE = 2 weeks; 127: uint256 public constant GRACE_PERIOD = 1 weeks; 130: uint256 public constant ENDORSEMENT_THRESHOLD = 20; 133: uint256 public constant EXECUTION_THRESHOLD = 33; 137: uint256 public constant EXECUTION_TIMELOCK = 3 days;
File: src/policies/Operator.sol 83: uint8 public immutable ohmDecimals; 86: uint8 public immutable reserveDecimals; 89: uint32 public constant FACTOR_SCALE = 1e4;
... See the rest this report here