Olympus DAO contest - Guardian's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L240 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L295

Vulnerability details

Impact

Firstly, since the VOTES module token is non-transferrable, it is not necessary to "lock" VOTES in the Governance contract -- although this might change if the VOTES module were to be upgraded to a transferrable token.

If the locking logic is kept, there should be added checks in the vote function to prevent users from accidentally voting with 0 votes or with potential votes locked up in a past proposal. If a user were to accidentally vote without reclaiming their past locked votes, they wouldn’t be able to exercise their full voting power – as once a nonzero value is recorded in the userVotesForProposal, the user can not vote again for that proposal, even if they reclaim more votes.

Additionally, there is no check to prevent user's from calling the vote function while they have 0 VOTES in their wallet.

Proof of Concept

User A has 5 VOTES tokens.

User A votes on proposal 1.

User A's 5 VOTES tokens are locked in the governance contract.

User A receives 5 more VOTES tokens.

Proposal 1 reaches the execution threshold and is executed.

Proposal 2 is submitted and activated for voting.

User A votes on proposal 2.

User A's votes for proposal 2 are recorded as just 5. But User A had 10 total tokens, with 5 of them unclaimed from the governance contract.

User A realizes they didn't exercise their full voting power, but they cannot vote again -- even after reclaiming their locked votes.

Recommended Mitigation Steps

There are a few ways this might be resolved:

• Remove the locking functionality, as VOTES is not currently a transferrable token.
• Record the locked amount of each user, and revert if they have a locked amount when calling vote OR transfer their locked amount to count towards the current proposal automatically in the votes function.
• Add a check to prevent users from accidentally calling vote when they have no VOTES tokens in their wallet.
• Allow users to vote multiple times, transferring their VOTES tokens that they voted with to the governance contract each time.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L161 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L170

Vulnerability details

Impact

Extra validation checks should be added on the result from Chainlink to ensure non-stale data. The price from the data feed influences the moving average and consequently range data so it is imperative the data is up to date and correct. If a price less than 0 was returned for ohmEthPriceInt or reserveEthPriceInt and then converted to a uint, the resulting price would be massive.

Tools Used

Remix

Recommended Mitigation Steps

https://docs.chain.link/docs/price-feeds-api-reference/#latestrounddata-1 Add the following checks:

require(answer > 0)  // answer is int256
require(updatedAt != 0)
require(answeredInRound >= roundId)

• Heart policy can allow for Operator update, just as it allows for update of the reward token and reward amount. Otherwise, if the Operator is deactivated then the Heart is still calling the stale Operator. (Or should Operator be a module, that way upgrades happen smoothly because they are already supported by logic in the kernel)

• BondCallback.sol::160 priorBalances[token] = token.balanceOf(address(this)); could be set to priorBalances[token] = 0; as the whole balance has been transferred in line 159.

• Oftentimes bonds are negative ROI, but users still use them as opportunities to bypass slippage and do an “OTC” deal of sorts with the treasury. An OHM-OHM bond cannot be used as an “OTC” deal since you are bonding OHM for OHM, but there may be other reasons why a user might take a negative ROI OHM bond. However, in BondCallback.sol::120 the callback function will revert if the outputAmount_ is less than the inputAmount_. Therefore it is not possible to take a negative ROI OHM-OHM bond, which may be unexpected behavior.

• PRICE.sol::59 decimals should follow constant variable naming conventions

• PriceConfig.sol::54-54 The comment implies that a smaller duration should not clear the data in the current window and require re-initialization. However, changeMovingAverageDuration in PRICE.sol clears the current data no matter if the MA duration is smaller or larger.

  • If a smaller duration is provided, there is no need to clear all data and require initialization once again. Instead, new numObservations would get calculated (smaller or equal to old duration), and those last numObservations can be kept and other variables adjusted accordingly. This would allow for the protocol to keep initialized=True. Consequently, functions such as getCurrentPrice, getMovingAverage, getLastPrice would continue to work for the modules and policies that rely on them such as the Operator and the RANGE.

• PRICE.sol::240, 266 movingAverageDuration_ and observationFrequency_ should be checked that they differ from the previous values. No need to perform reset if the values are the same.

• PRICE.sol::126 typo numbe

• Redundant boolean checks: Governance.sol::223,306

• BondCallback.sol::160 The remaining balance will always be 0, therefore the priorBalances can simply be set to 0

• Governance.sol::193 The previousEndorsement variable is only used once, the following line can simply read from userEndorsementsForProposal without declaring a stack variable.

• INSTR.sol::48 this check can be moved up to the second line of the function, to avoid spending extra gas in the case of a revert.

• TRSRY.sol::122 in the setDebt function, instead of storing oldDebt and using an if to conditionally handle the debt change, Simply subtract the oldDebt from the totalDebt and add the new debt amount to the totalDebt.

• PRICE.sol::177 no need to declare a currentPrice variable only to return it on the next line, instead just return the computed value directly.

• Operator.sol::299 Rather than transferring ohm from the msg.sender to the operator contract and then burning it from the operator contract, the MINTR can burn the ohm directly from the msg.sender.

• Variables should not be initialized to defaults (uint256 default is 0): Kernel.sol::397 => for (uint256 i = 0; i < reqLength; ) { utils/KernelUtils.sol::43 => for (uint256 i = 0; i < 5; ) { utils/KernelUtils.sol::58 => for (uint256 i = 0; i < 32; ) {

• Length of array should be computed outside of for-loop: Kernel.sol::300 => getPolicyIndex[policy_] = activePolicies.length - 1; Kernel.sol::304 => uint256 depLength = dependencies.length; Kernel.sol::310 => getDependentIndex[keycode][policy_] = moduleDependents[keycode].length - 1; Kernel.sol::334 => Policy lastPolicy = activePolicies[activePolicies.length - 1]; Kernel.sol::352 => uint256 keycodeLen = allKeycodes.length; Kernel.sol::361 => uint256 policiesLen = activePolicies.length; Kernel.sol::380 => uint256 depLength = dependents.length; Kernel.sol::396 => uint256 reqLength = requests_.length; Kernel.sol::411 => uint256 depcLength = dependencies.length; Kernel.sol::418 => Policy lastPolicy = dependents[dependents.length - 1]; modules/INSTR.sol::43 => uint256 length = instructions_.length; modules/INSTR.sol::48 => if (length == 0) revert INSTR_InstructionsCannotBeEmpty(); modules/INSTR.sol::50 => for (uint256 i; i < length; ) { modules/INSTR.sol::61 => } else if (instruction.action == Actions.ChangeExecutor && i != length - 1) { modules/PRICE.sol::201 => /// @param startObservations_ - Array of observations to initialize the moving average with. Must be of length numObservations. modules/PRICE.sol::212 => uint256 numObs = observations.length; modules/PRICE.sol::215 => if (startObservations_.length != numObs || lastObservationTime_ > uint48(block.timestamp)) policies/BondCallback.sol::155 => uint256 len = tokens_.length; policies/Governance.sol::188 => if (instructions.length == 0) { policies/Governance.sol::278 => for (uint256 step; step < instructions.length; ) { policies/PriceConfig.sol::41 => /// @param startObservations_ Array of observations to initialize the moving average with. Must be of length numObservations. policies/TreasuryCustodian.sol::58 => uint256 len = tokens_.length;

• != is more efficient than > 0 for uint comparisons: policies/Governance.sol::247 => if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) { test/modules/PRICE.t.sol::101 => change = int256(uint256(keccak256(abi.encodePacked(nonce, i)))) % int256(1000); test/modules/PRICE.t.sol::128 => change = int256(uint256(keccak256(abi.encodePacked(nonce, i)))) % int256(1000); test/policies/PriceConfig.t.sol::124 => change = int256(uint256(keccak256(abi.encodePacked(nonce, i)))) % int256(1000);

• Switching from division/multiplication to right-shift/left-shift can save gas: policies/Operator.sol::372 => int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2); policies/Operator.sol::419 => uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price; policies/Operator.sol::420 => uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price; policies/Operator.sol::427 => int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals / 2); policies/Operator.sol::786 => ) * (FACTOR_SCALE + RANGE.spread(true) * 2)) /