Missed heart beat negates TWAP effectiveness and drains reward token balance

[code423n4]

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
+        }
+

[Oighty]

Agree with calling resetBeat() on a toggleBeat() call. Other issue has been commented on in #405 and #79, with a solution proposed in the latter.

[0xean]

closing as duplicate of #79