Olympus DAO contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

Price data can get lost due to small changes in the price in PRICE.updateMovingAverage(). The average os updated by adding or subtracting the delta divided by the number of observations. However, if the delta is not divisible by numObs, the calculation here won't be accurate and some price data will be lost due to rounding errors.

if (currentPrice > earliestPrice) {
    _movingAverage += (currentPrice - earliestPrice) / numObs;
} else {
    _movingAverage -= (earliestPrice - currentPrice) / numObs;
}

Proof of Concept

Let's assume the moving average is a 120 day moving average sampled three times a day, which means numObs = 360. Also assume the current moving average is 2e18 (2 reserve tokens per ohm), and the price changes to 2.001e18 which means the delta here is 0.001e18 = 1e15. Now, the PRICE.updateMovingAverage() function will calculate the average change by: (currentPrice - earliestPrice) / numObs = (2.001e18 - 2e18) / 360 = 1e15 / 360 = 2777777777777.777.... However, due to the operations being applied to uints, the actual delta is 2777777777777 and this value will be added to the moving average instead of the actual value.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider maintain the sum of the observation prices instead of the actual average, and each time the average is accessed divide it by the number of observations. That way, The delta itself will be added/subtracted from the maintained variable instead of the delta divided by the number of observations which caused to data loss.

Another possible solution is to update the average as so:

if (currentPrice > earliestPrice) {
    _movingAverage = (_movingAverage * numObs + (currentPrice - earliestPrice)) / numObs;
} else {
    _movingAverage = (_movingAverage * numObs - (currentPrice - earliestPrice)) / numObs;
}

QA

• Forgot to remove some TODO comments:
  • TreasuryCustodian - lines 51, 52, 56
  • OlympusERC20 - line 664
  • Operator - line 657
  • Deploy - line 145
• Use constants for roles names instead of writing the raw string in every place it used. This will prevent possible typos and will make changing a role name easier (just change the constant value instead of changing every instance).
• The comment in Kernel line 180 is wrong - it says the mapping maps a policy to a keycode, but the opposite is correct - it maps a keycode to a policy

  /// @dev    Policy -> Keycode -> Function Selector -> bool for permission
  mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions;

  The correct one:

  /// @dev    Keycode -> Policy -> Function Selector -> bool for permission
  mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions;

• There isn't any functionality to remove a module in the Kernel contract, which might be a thing you'd like to do.
• A user is not able to use his new votes if he get more votes after he already voted, but these votes will be counted as part of the total supply. In the worst case, this can lead to a proposal not being able to pass because the total supply will be increased and the users won't be able to use their votes in order to pass the execution threshold
• Pragmas should be locked to a specific compiler version, to avoid contracts getting deployed using a different version, which may have a greater risk of undiscovered bugs.

Gas Optimizations

• Use the calldata modifier instead of memory for array, struct and dynamic arguments in order to save gas.
  • PRICE - line 205
  • RANGE - lines 79-80
  • BondCallback - line 152
  • Governance - line 162
  • Operator - line 96-97
  • PriceConfig - line 45
  • TreasuryCustodian - line 53
• Unroll the loop in KernelUtils.ensureValidKeycode() to save gas spent on loop index and condition operations
• Line 452 in the Kernel contract (if (!isRole[role_]) revert Kernel_RoleDoesNotExist(role_);) is redundant because !isRole[role_] => !hasRole[addr_][role_]
• In the Kernel._activatePolicy() function, in order to avoid a redundant calculation, use:

  activePolicies.push(policy_);
  getPolicyIndex[policy_] = activePolicies.length - 1;

  instead of

  getPolicyIndex[policy_] = activePolicies.length;
  activePolicies.push(policy_);

  (This optimization can be done in more places)
• Cache activeProposal.proposalId in the Governance.vote() function
• Cache updateMovingAverage in the PRICE.updateMovingAverage()
• Prefix incrementation and decrementation costs less gas than postfix incrementation and decrementation.
  • Operator - lines 488, 670, 675, 686, 691
  • KernelUtils - lines 49, 64
• Initializing variables to their default value is done automatically, and doing it manually actually costs more gas.
  • Kernel - line 397
  • KernelUtils - lines 43, 58
• Cache the array length outside of the loop to avoid accessing it in every iteration:
  • Governance - line 278
• Use != 0 instead of > 0 when comparing a uint with 0: OlympusERC20 - line 611 FullMath - lines 35, 122 Governance - line 247
• Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas than shorter strings, as documented publicly. Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, use custom errors, introduced in Solidity 0.8.4: https://blog.soliditylang.org/2021/04/21/custom-errors
• Use left/right shift instead of multiplying/dividing by powers of 2 (x * (2**i) == x << i, x / (2**i) == x >> i)
  • Operator - lines 372, 419-420, 427, 786