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: 46/147
Findings: 2
Award: $356.40
🌟 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
318.9805 DAI - $318.98
File: /src/Kernel.sol 251: executor = target_; 253: admin = target_;
There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.
Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended. Following Solidity’s style guide, constants should be named in UPPER_CASE_WITH_UNDERSCORES format, and specific public getters should be defined to read each one of them.
File: /src/modules/RANGE.sol function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned { // Confirm spreads are within allowed values if ( wallSpread_ > 10000 || //@audit 1000 wallSpread_ < 100 || //@audit 100 cushionSpread_ > 10000 || //@audit 1000 cushionSpread_ < 100 || //@audit 100 cushionSpread_ > wallSpread_ ) revert RANGE_InvalidParams(); 264: if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();
File: /src/modules/PRICE.sol @audit: 38 90: if (exponent > 38) revert Price_InvalidParams();
File: /src/policies/Operator.sol 376: uint256 bondScale = 10 ** //@audit: 10 378: uint8( 379: 36 + scaleAdjustment + int8(reserveDecimals) - int8(ohmDecimals) - priceDecimals //@audit: 36 340: ); 430: uint256 oracleScale = 10**uint8(int8(oracleDecimals) - priceDecimals); //@audit: 10 431: uint256 bondScale = 10 ** //@audit: 10 432: uint8( 433: 36 + scaleAdjustment + int8(ohmDecimals) - int8(reserveDecimals) - priceDecimals //@audit: 36 434: ); 486: while (price_ >= 10) { // @audit: 10 487: price_ = price_ / 10; // @audit: 10 518: if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams(); //@audit: 10000 & 100 535: if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams(); //@audit: 10_000 550: if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams(); //@audit: 1000 & 100 753: 10**reserveDecimals * RANGE.price(true, false), // @audit: 10 754: 10**ohmDecimals * 10**PRICE.decimals() // @audit: 10 764: 10**ohmDecimals * 10**PRICE.decimals(), // @audit: 10 765: 10**reserveDecimals * RANGE.price(true, true) // @audit: 10 784: 10**ohmDecimals * 10**PRICE.decimals(), // @audit: 10 785: 10**reserveDecimals * RANGE.price(true, true) // @audit: 10
File: /src/policies/Governance.sol 164: if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT) // @audit: 10000 217: (totalEndorsementsForProposal[proposalId_] * 100) < //@audit: 100 268: if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) { //@audit: 100
assembly { size := extcodesize() } => uint256 size = address().code.length We can minimize the complexity of the project by avoiding using assembly where it's not necessary
File: /src/utils/KernelUtils.sol 31: function ensureContract(address target_) view { 32: uint256 size; 33: assembly { 34: size := extcodesize(target_) 35: } 36: if (size == 0) revert TargetNotAContract(target_); 37: }
Contracts are allowed to override their parents' functions and change the visibility from external to public.
File: /src/Kernel.sol 451: function revokeRole(Role role_, address addr_) public onlyAdmin { 452: if (!isRole[role_]) revert Kernel_RoleDoesNotExist(role_); 453: if (!hasRole[addr_][role_]) revert Kernel_AddressDoesNotHaveRole(addr_, role_); 455: hasRole[addr_][role_] = false; 456: 457: emit RoleRevoked(role_, addr_); 458: }
File: /src/Kernel.sol function grantRole(Role role_, address addr_) public onlyAdmin { if (hasRole[addr_][role_]) revert Kernel_AddressAlreadyHasRole(addr_, role_); ensureValidRole(role_); if (!isRole[role_]) isRole[role_] = true; hasRole[addr_][role_] = true; emit RoleGranted(role_, addr_); }
File: /src/modules/TRSRY.sol function withdrawReserves( address to_, ERC20 token_, uint256 amount_ ) public { _checkApproval(msg.sender, token_, amount_); token_.safeTransfer(to_, amount_); emit Withdrawal(msg.sender, to_, token_, amount_); }
File: /src/modules/RANGE.sol function updateMarket( bool high_, uint256 market_, uint256 marketCapacity_ ) public permissioned {
File: /src/modules/INSTR.sol 37: function getInstructions(uint256 instructionsId_) public view returns (Instruction[] memory) {
File: /src/policies/Governance.sol 145: function getMetadata(uint256 proposalId_) public view returns (ProposalMetadata memory) { 151: function getActiveProposal() public view returns (ActivatedProposal memory) {
Index 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.
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/PRICE.sol 26: event NewObservation(uint256 timestamp_, uint256 price_, uint256 movingAverage_); 27: event MovingAverageDurationChanged(uint48 movingAverageDuration_); 28: event ObservationFrequencyChanged(uint48 observationFrequency_);
File: /src/modules/INSTR.sol 11: event InstructionsStored(uint256 instructionsId);
File: /src/policies/Operator.sol 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/Heart.sol 28: event Beat(uint256 timestamp_); 29: event RewardIssued(address to_, uint256 rewardAmount_); 30: event RewardUpdated(ERC20 token_, uint256 rewardAmount_);
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);
Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those.
File: /src/modules/TRSRY.sol 51: function VERSION() external pure override returns (uint8 major, uint8 minor) { 52: return (1, 0); 53: }
File: /src/modules/RANGE.sol 115: function VERSION() external pure override returns (uint8 major, uint8 minor) { 116: return (1, 0); 117: }
File: /src/modules/PRICE.sol 113: function VERSION() external pure override returns (uint8 major, uint8 minor) { 114: return (1, 0); 115: }
File: /src/modules/VOTES.sol 27: function VERSION() external pure override returns (uint8 major, uint8 minor) { 28: return (1, 0); 29: }
File: /src/modules/INSTR.sol 28: function VERSION() public pure override returns (uint8 major, uint8 minor) { 29: return (1, 0); 30: }
File: /src/Kernel.sol //@audit: Missing @param newKernel_ 75: /// @notice Function used by kernel when migrating to a new kernel. 76: function changeKernel(Kernel newKernel_) external onlyKernel { //@audit: Missing @param action_ , @param target 234: /// @notice Main kernel function. Initiates state changes to kernel depending on Action passed in. 235: function executeAction(Actions action_, address target_) external onlyExecutor { //@audit: Missing @param role_, @param addr_ 438: /// @notice Function to grant policy-defined roles to some address. Can only be called by admin. 439: function grantRole(Role role_, address addr_) public onlyAdmin { //@audit: Missing @param role_, @param addr_ 450: /// @notice Function to revoke policy-defined roles from some address. Can only be called by admin. 451: function revokeRole(Role role_, address addr_) public onlyAdmin {
File: /src/modules/TRSRY.sol //@audit: Missing @param withdrawer_,@param token_ , @param amount_ 63: /// @notice Sets approval for specific withdrawer addresses 64: function setApprovalFor( 65: address withdrawer_, 66: ERC20 token_, 67: uint256 amount_ 68: ) external permissioned { //@audit: Missing @param token_, @param amount_ 92: function getLoan(ERC20 token_, uint256 amount_) external permissioned { //@audit: Missing @param token_, @param amount_ 105: function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant { //@audit: Missing @param withdrawer_,@param token_ , @param amount_ 122: function setDebt( 123: ERC20 token_, 124: address debtor_, 125: uint256 amount_ 126: ) external permissioned {
File: /src/modules/INSTR.sol //@audit: Missing @param instructions_, @param returns 41: /// @notice Store a list of Instructions to be executed in the future. 42: function store(Instruction[] calldata instructions_) external permissioned returns (uint256) {
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.
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/modules/PRICE.sol 205: function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_)
This is a best-practice to protect against reentrancy in other modifiers
File: /src/policies/Operator.sol function swap( ERC20 tokenIn_, uint256 amountIn_, uint256 minAmountOut_ ) external override onlyWhileActive nonReentrant returns (uint256 amountOut) {
#0 - 0xLienid
2022-09-09T02:23:57Z
Valid and will implement most outside of the 0x0 check
🌟 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
37.4238 DAI - $37.42
NB: Some functions have been truncated where neccessary to just show affected parts of the code The gas estimates are the exact results from running the tests included with an exception of internal functions(we estimate based on number of SLOADS saved) The optimizer is set to run with the default runs(200). Throught the report some places might be denoted with audit tags to show the actual place affected.
Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD. Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)
File: /src/policies/BondCallback.sol 28: IBondAggregator public aggregator;
File: /src/policies/BondCallback.sol 32: ERC20 public ohm;
File: /src/policies/Heart.sol 48: IOperator internal _operator;
Average Gas Before: 29228 Average Gas After: 28871
File: /src/policies/Heart.sol 92: function beat() external nonReentrant { 93: if (!active) revert Heart_BeatStopped(); 94: if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle(); //@audit: frequency() ... 102: // Update the last beat timestamp 103: lastBeat += frequency(); //@audit: frequency() ... 109: }
Average Gas Before: 3956 Average Gas After: 2484
File: /src/policies/PriceConfig.sol 25: function requestPermissions() 26: external 27: view 28: override 29: returns (Permissions[] memory permissions) 30: { 31: permissions = new Permissions[](3); 32: permissions[0] = Permissions(PRICE.KEYCODE(), PRICE.initialize.selector); //@audit: PRICE.KEYCODE() 33: permissions[1] = Permissions(PRICE.KEYCODE(), PRICE.changeMovingAverageDuration.selector); 34: permissions[2] = Permissions(PRICE.KEYCODE(), PRICE.changeObservationFrequency.selector); 35: }
The above can be rewriten as follows:
function requestPermissions()external view override returns (Permissions[] memory permissions){ Keycode PRICE_KEYCODE = PRICE.KEYCODE(); permissions = new Permissions[](3); permissions[0] = Permissions(PRICE_KEYCODE, PRICE.initialize.selector); //@audit: PRICE.KEYCODE() permissions[1] = Permissions(PRICE_KEYCODE, PRICE.changeMovingAverageDuration.selector); permissions[2] = Permissions(PRICE_KEYCODE, PRICE.changeObservationFrequency.selector); }
Other Instance: https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/VoterRegistration.sol#L27-L36
Average Gas Before: 2863 Average Gas After: 2105
File: /src/policies/VoterRegistration.sol 27: function requestPermissions() 28: external 29: view 30: override 31: returns (Permissions[] memory permissions) 32: { 33: permissions = new Permissions[](2); 34: permissions[0] = Permissions(VOTES.KEYCODE(), VOTES.mintTo.selector); 35: permissions[1] = Permissions(VOTES.KEYCODE(), VOTES.burnFrom.selector); 36: }
See an existing implementation already on Line 34 for how to implement the above function
function requestPermissions() external view override returns (Permissions[] memory requests) { Keycode TRSRY_KEYCODE = TRSRY.KEYCODE(); requests = new Permissions[](2); requests[0] = Permissions(TRSRY_KEYCODE, TRSRY.setApprovalFor.selector); requests[1] = Permissions(TRSRY_KEYCODE, TRSRY.setDebt.selector); }
When 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.When arguments are read-only on external functions, the data location should be calldata:
Average Gas Before: 432495 Average Gas After: 430562
File: /src/modules/PRICE.sol function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) external permissioned { if (initialized) revert Price_AlreadyInitialized(); // Cache numObservations to save gas. 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; } } // Set moving average, last observation time, and initialized flag _movingAverage = total / numObs; lastObservationTime = lastObservationTime_; initialized = true; }
startObservations_ should be declared calldata as it is readonly on this function
Average Gas Before: 6956 Average Gas After: 6842
File: /src/policies/TreasuryCustodian.sol function revokePolicyApprovals(address policy_, ERC20[] memory tokens_) external { if (Policy(policy_).isActive()) revert PolicyStillActive(); // TODO Make sure `policy_` is an actual policy and not a random address. uint256 len = tokens_.length; for (uint256 j; j < len; ) { TRSRY.setApprovalFor(policy_, tokens_[j], 0); unchecked { ++j; } } emit ApprovalRevoked(policy_, tokens_); }
ERC20[] memory tokens_ should be declared as ERC20[] calldata tokens_ as it is readonly in this function
Average Gas Before: 12729 Average Gas After: 12543
File: /src/policies/BondCallback.sol function batchToTreasury(ERC20[] memory tokens_) external onlyRole("callback_admin") { ERC20 token; uint256 balance; uint256 len = tokens_.length; for (uint256 i; i < len; ) { token = tokens_[i]; balance = token.balanceOf(address(this)); token.safeTransfer(address(TRSRY), balance); priorBalances[token] = token.balanceOf(address(this)); unchecked { ++i; } } }
ERC20[] memory tokens_ should be declared as ERC20[] calldata tokens_ as it is readonly in this function
Average Gas Before: 491657 Average Gas After: 488077
File: /src/policies/PriceConfig.sol 45: function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) 46: external 47: onlyRole("price_admin") 48: { 49: PRICE.initialize(startObservations_, lastObservationTime_); 50: }
uint256[] memory startObservations_ should be declared as uint256[] calldata startObservations_ as it is readonly in this function
Average Gas Before: 491657 Average Gas After: 488077
File: /src/policies/Governance.sol 159: function submitProposal( 160: Instruction[] calldata instructions_, 161: bytes32 title_, 162: string memory proposalURI_ 163: ) external {
string memory proposalURI_ should be declared as string calldata proposalURI_ as it is readonly in this function
The code can be optimized by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
Average Gas before:9124 Average Gas After: 9001
The gas saved ends up being higher than the estimates if we optimize the functions that are also called inside this one (~259 gas)
File: /src/modules/PRICE.sol 122: function updateMovingAverage() external permissioned { ... 129: // Get earliest observation in window 130: uint256 earliestPrice = observations[nextObsIndex]; ... 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; ... 147: }
Average Gas before:5264 Average Gas After: 5130
File: /src/modules/PRICE.sol 154: function getCurrentPrice() public view returns (uint256) { 155: if (!initialized) revert Price_NotInitialized(); ... 165: if (updatedAt < block.timestamp - 3 * uint256(observationFrequency)) 166: revert Price_BadFeed(address(_ohmEthPriceFeed)); ... 171: if (updatedAt < block.timestamp - uint256(observationFrequency)) 172: revert Price_BadFeed(address(_reserveEthPriceFeed));
Average Gas before:9137 Average Gas After: 9038
File: /src/modules/PRICE.sol 122: function updateMovingAverage() external permissioned { ... 134: // Calculate new moving average 135: if (currentPrice > earliestPrice) { 136: _movingAverage += (currentPrice - earliestPrice) / numObs; //@audit: SLOAD 1 on happy path 137: } else { 138: _movingAverage -= (earliestPrice - currentPrice) / numObs; //@audit: SLOAD 1 on sad path 139: } ... 146: emit NewObservation(block.timestamp, currentPrice, _movingAverage);//@audit: SLOAD 2 147: }
File: /src/modules/PRICE.sol 183: function getLastPrice() external view returns (uint256) { 184: if (!initialized) revert Price_NotInitialized(); 185: uint32 lastIndex = nextObsIndex == 0 ? numObservations - 1 : nextObsIndex - 1; 186: return observations[lastIndex]; 187: }
File: /src/modules/PRICE.sol 240: function changeMovingAverageDuration(uint48 movingAverageDuration_) external permissioned { 241: // Moving Average Duration should be divisible by Observation Frequency to get a whole number of observations 242: if (movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency != 0) //@audit: SLOAD 1 243: revert Price_InvalidParams(); ... 245: // Calculate the new number of observations 246: uint256 newObservations = uint256(movingAverageDuration_ / observationFrequency); //@audit: SLOAD 2
File: /src/modules/PRICE.sol 266: function changeObservationFrequency(uint48 observationFrequency_) external permissioned { 267: // Moving Average Duration should be divisible by Observation Frequency to get a whole number of observations 268: if (observationFrequency_ == 0 || movingAverageDuration % observationFrequency_ != 0) //@audit: SLOAD 1 269: revert Price_InvalidParams(); ... 271: // Calculate the new number of observations 272: uint256 newObservations = uint256(movingAverageDuration / observationFrequency_); //@audit: SLOAD 2
frequency() )Estimations without caching the frequency function: only cache lastBeat Average Gas before:29228 Average Gas After: 29188 Estimations after caching the frequency function: lastBeat and frequency Average Gas before:29228 Average Gas After: 28829
File: /src/policies/Heart.sol 92: function beat() external nonReentrant { 93: if (!active) revert Heart_BeatStopped(); 94: if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle(); //@audit: lastBeat SLOAD 1 ... 102: // Update the last beat timestamp 103: lastBeat += frequency(); //@audit: lastBeat SLOAD 2 ... 109: }
File: /src/policies/Heart.sol 111: function _issueReward(address to_) internal { 112: rewardToken.safeTransfer(to_, reward); //@audit: reward SLOAD 1 113: emit RewardIssued(to_, reward); //@audit: reward SLOAD 2 114: }
Average Gas Before: 61568 Average Gas After: 61287
File: /src/policies/Governance.sol 240: function vote(bool for_) external { ... 243: if (activeProposal.proposalId == 0) { //@audit: activeProposal.proposalId 244: revert NoActiveProposalDetected(); 245: } ... 247: if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) { //@audit: activeProposal.proposalId 248: revert UserAlreadyVoted(); 249: } ... 251: if (for_) { 252: yesVotesForProposal[activeProposal.proposalId] += userVotes; //@audit: activeProposal.proposalId 253: } else { 254: noVotesForProposal[activeProposal.proposalId] += userVotes; //@audit: activeProposal.proposalId 255: } ... 257: userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes; //@audit: activeProposal.proposalId ... 261: emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);//@audit: activeProposal.proposalId 262: }
Average Gas Before: 171376 Average Gas After: 171229
File: /src/policies/Governance.sol 265: function executeProposal() external { 266: uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] - 267: noVotesForProposal[activeProposal.proposalId]; ... 276: Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId); ... 285: emit ProposalExecuted(activeProposal.proposalId); ... 289: }
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Affected code:
File: /src/Kernel.sol 279: function _upgradeModule(Module newModule_) internal { 295: function _activatePolicy(Policy policy_) internal { 325: function _deactivatePolicy(Policy policy_) internal { 351: function _migrateKernel(Kernel newKernel_) internal { 409: function _pruneFromDependents(Policy policy_) internal {
File: /src/policies/Heart.sol 111: function _issueReward(address to_) internal { 112: rewardToken.safeTransfer(to_, reward); 113: emit RewardIssued(to_, reward); 114: }
The above function is only called on Line 106
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage somestruct = someMap[someIndex] and use it.
File: /src/modules/TRSRY.sol 105: function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant { 106: if (reserveDebt[token_][msg.sender] == 0) revert TRSRY_NoDebtOutstanding(); 108: // Deposit from caller first (to handle nonstandard token transfers) 109: uint256 prevBalance = token_.balanceOf(address(this)); 110: token_.safeTransferFrom(msg.sender, address(this), amount_); 112: uint256 received = token_.balanceOf(address(this)) - prevBalance; 114: // Subtract debt from caller 115: reserveDebt[token_][msg.sender] -= received; 116: totalDebt[token_] -= received; 118: emit DebtRepaid(token_, msg.sender, received); 119: }
File: /src/policies/Governance.sol 205: function activateProposal(uint256 proposalId_) external { 206: ProposalMetadata memory proposal = getProposalMetadata[proposalId_]; 223: if (proposalHasBeenActivated[proposalId_] == true) { 224: revert ProposalAlreadyActivated(); 225: } 233: proposalHasBeenActivated[proposalId_] = true; 236: }
File: /src/policies/Governance.sol 295: function reclaimVotes(uint256 proposalId_) external { 306: if (tokenClaimsForProposal[proposalId_][msg.sender] == true) { 307: revert VotingTokensAlreadyReclaimed(); 308: } 310: tokenClaimsForProposal[proposalId_][msg.sender] = true; 313: } 314: }
Saves 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.
File: /src/modules/TRSRY.sol 36: mapping(ERC20 => uint256) public totalDebt; 38: /// @notice Debt for particular token and debtor address 39: mapping(ERC20 => mapping(address => uint256)) public reserveDebt;
While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+
File: /src/policies/Operator.sol 372: int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2); 427: int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals / 2);
File: /src/policies/Heart.sol 103: lastBeat += frequency();
The above should be modified to
103: lastBeat = lastBeat + frequency();
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: /src/modules/PRICE.sol //@audit: uint48 lastObservationTime_ 205: function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) //@audit: uint48 movingAverageDuration_ 240: function changeMovingAverageDuration(uint48 movingAverageDuration_) external permissioned { //@audit: uint48 observationFrequency_ 266: function changeObservationFrequency(uint48 observationFrequency_) external permissioned {
File: /src/policies/Operator.sol 527: function setCushionParams( 528: uint32 duration_, 529: uint32 debtBuffer_, 530: uint32 depositInterval_ 531: ) external onlyRole("operator_policy") { 548: function setReserveFactor(uint32 reserveFactor_) external onlyRole("operator_policy") { 559: function setRegenParams( 560: uint32 wait_, 561: uint32 threshold_, 562: uint32 observe_ 563: ) external onlyRole("operator_policy") {
File: /src/policies/PriceConfig.sol //@audit: uint48 lastObservationTime_ 45: function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_) //@audit: uint48 movingAverageDuration_ 58: function changeMovingAverageDuration(uint48 movingAverageDuration_) //@audit: uint48 observationFrequency_ 69: function changeObservationFrequency(uint48 observationFrequency_)
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource
136: _movingAverage += (currentPrice - earliestPrice) / numObs;
The operation currentPrice - earliestPrice cannot underflow due to the check on Line 135 which ensures that currentPrice is greater than earliestPrice
https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L138
138: _movingAverage -= (earliestPrice - currentPrice) / numObs;
The operation earliestPrice - currentPrice cannot underflow due to the check on Line 135 which ensures that this operation would only be perfomened if earliestPrice is greter than currentPrice
185: uint32 lastIndex = nextObsIndex == 0 ? numObservations - 1 : nextObsIndex - 1;
The operation nextObsIndex - 1 cannot underflow as it would only be perfomed if nextObsIndex is not equal to 0. As nextObsIndex is a uint if it's not equal to 0 then it must be greater than 0
131: if (oldDebt < amount_) totalDebt[token_] += amount_ - oldDebt; 132: else totalDebt[token_] -= oldDebt - amount_;
The operation amount_ - oldDebt cannot underlow due the check if (oldDebt < amount_) that ensures that amount is greater than oldDebt before performng the operation
The operation oldDebt - amount_ cannot underlow due the check if (oldDebt < amount_) that ensures that this operation would only be perfomed if oldDebt is greater than amount_
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
The solidity compiler will always read the length of the array during each iteration. That is,
1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)
This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas
Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:
File: /src/policies/Governance.sol 276: Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId); 278: for (uint256 step; step < instructions.length; ) {
The above should be modified to
276: Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId); 277: uint256 length = instructions.length; 278: for (uint256 step; step < length; ) {
++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.
i++ increments i and returns the initial value of i. Which means:
uint i = 1; i++; // == 1 but i == 2
But ++i returns the actual incremented value:
uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable
In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
Instances include:
File: /src/utils/KernelUtils.sol 49: i++; 64: i++;
File: /src/policies/Operator.sol 488: decimals++;
Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:
File: /src/policies/Governance.sol 223: if (proposalHasBeenActivated[proposalId_] == true) { 306: if (tokenClaimsForProposal[proposalId_][msg.sender] == true) {
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.
See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past
Instances affected include
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/Operator.sol 63: bool public initialized; 66: bool public active;
File: /src/policies/BondCallback.sol 24: mapping(address => mapping(uint256 => bool)) public approvedMarkets;
File: /src/policies/Heart.sol 33: bool public active;
File: /src/policies/Governance.sol 105: mapping(uint256 => bool) public proposalHasBeenActivated; 117: mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;
If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.
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 89: uint32 public constant FACTOR_SCALE = 1e4;
File: /src/modules/RANGE.sol 65: uint256 public constant FACTOR_SCALE = 1e4;