Olympus DAO contest - devtooligan's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Heart.sol#L92-L109

Vulnerability details

While there is a check for too quick of a heart beat, there is no check for a heart beat where an extended amount of time has passed since the last beat. This condition could result from technical difficulties preventing the keeper from calling beat(), an admin toggling the Heart beat back on without resetting the heart beat, or other unforeseen conditions.

Furthermore, when beat() is called, the lastBeat is not updated to the current block timestamp.

Impact:

As a result of this, beat() may be called multiple times until the lastBeat catches up to the current block timestamp. This has two significant impacts:

1. TWAP observations will be duplicated. If the amount of time since lastBeat exceeds the movingAverageDuration then all of the observations will be set to the current price. This negates the time-weighted effect of the TWAP. In a volatile market, this could mean the sale of reserve wall at significantly below market, or the purchase of the OHM wall significantly above market.

2. Rewards tokens can be drained. beat() can be called repeatedly thereby draining the reward token balance of Heart. The total loss is capped at reward * timeElapsedSinceLastBeat / frequency() which could be significant, especially in the case of toggling the beat to active after an extended inactive period.

Proof of concept:

    function testCallBeatRepeatedly() public {
        uint256 startBalance = rewardToken.balanceOf(address(this));
        console2.log("starting balance attacker", startBalance / 1e18);
        uint256 startBalanceHeart = rewardToken.balanceOf(address(heart));
        console2.log("starting balance heart", startBalanceHeart / 1e18);
        while (rewardToken.balanceOf(address(heart)) >= 1e18) {
            heart.beat();
        }

        uint256 endBalance = rewardToken.balanceOf(address(this));
        console2.log("ending balance attacker", endBalance / 1e18);
        uint256 endBalanceHeart = rewardToken.balanceOf(address(heart));
        console2.log("ending balance heart", endBalanceHeart / 1e18);
    }

[PASS] testCallBeatRepeatedly() (gas: 14614379)
Logs:
  starting balance attacker 0
  starting balance heart 1000
  ending balance attacker 1000
  ending balance heart 0

Test result: ok. 1 passed; 0 failed; finished in 26.34ms

Recommendation:

1. Reset beat after toggling the Heart from inactive to active.

@@ -133,7 +137,11 @@ contract OlympusHeart is IHeart, Policy, ReentrancyGuard {

     /// @inheritdoc IHeart
     function toggleBeat() external onlyRole("heart_admin") {
-        active = !active;
+        bool priorState = active;
+        if (!priorState) {
+            resetBeat();
+        }
+        active = !priorState;
     }
  

2. Handle missed beats within beat(). This may include shutting down the walls -- updateCapacity(0), filling in missed observations,updating lastBeat, or other mitigative actions.

@@ -93,6 +93,11 @@ contract OlympusHeart is IHeart, Policy, ReentrancyGuard {
         if (!active) revert Heart_BeatStopped();
         if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();

+        uint MAXIMUM_BEAT_LAG = frequency() * 3; // this can be set elsewhere
+        if (block.timestamp - lastBeat > MAXIMUM_BEAT_LAG) {
+            // handle missed beats
+        }
+

QA Report

Use a struct for config params in Operator.sol

Severity: Low

Using a struct would greatly increase the readability and reduce the chance of a mistake during deployment.

diff --git a/src/policies/Operator.sol b/src/policies/Operator.sol
index 7573526..1b70536 100644
--- a/src/policies/Operator.sol
+++ b/src/policies/Operator.sol
@@ -88,14 +88,26 @@ contract Operator is IOperator, Policy, ReentrancyGuard {
     /// Constants
     uint32 public constant FACTOR_SCALE = 1e4;

+    struct ConfigParams {
+        uint32 cushionFactor;
+        uint32 cushionDuration;
+        uint32 cushionDebtBuffer;
+        uint32 cushionDepositInterval;
+        uint32 reserveFactor;
+        uint32 regenWait;
+        uint32 regenThreshold;
+        uint32 regenObserve;
+    }
+
     /* ========== CONSTRUCTOR ========== */
     constructor(
         Kernel kernel_,
         IBondAuctioneer auctioneer_,
         IBondCallback callback_,
         ERC20[2] memory tokens_, // [ohm, reserve]
-        uint32[8] memory configParams // [cushionFactor, cushionDuration, cushionDebtBuffer, cushionDepositInterval, reserveFactor, regenWait, regenThreshold, regenObserve]
+        ConfigParams memory configParams
     ) Policy(kernel_) {

Use constants for magic numbers

Severity: Low

Through the use of constants, certain limits and other values important to the protocol can be better managed and reduce chance of setting something incorrectly.

Here is an example of constants being used along with using a struct for ConfigParams

-        if (configParams[1] > uint256(7 days) || configParams[1] < uint256(1 days))
+        if (configParams.cushionDuration > MAX_CUSHION_DURATION || configParams.cushionDuration < MIN_CUSHION_DURATION )
             revert Operator_InvalidParams();

-        if (configParams[2] < uint32(10_000)) revert Operator_InvalidParams();
+        if (configParams.cushionDebtBuffer < MIN_DEBT_BUFFER) revert Operator_InvalidParams();

-        if (configParams[3] < uint32(1 hours) || configParams[3] > configParams[1])
+        if (configParams.cushionDepositInterval < MIN_CUSHION_DEPOSIT_INTERVAL || configParams.cushionDepositInterval > MAX_CUSHION_DEPOSIT_INTERVAL)
             revert Operator_InvalidParams();

Using true/false to mean "High" / "Low" is an antipattern

Severity: Low

It is confusing to read something like: _regenerate(false)

For better readability and to prevent possible mistakes in the future, use constants instead.

bool constant HIGH = true;
bool constant LOW = false;

_regenerate(LOW)

Use a local constant instead of calling an external contract.

Severity: Gas optimization

Instead of calling PRICE.decimals(), use a local constant for decimals. Both the decimals in PRICE.sol and the decimals when calling PRICE.sol as an external contract can be imported from a constants.sol file thereby keeping the gas savings from the constant and also retaining only a single source of truth for the constant values.