Non-flexible update window forces an ingestion of de/in-flated prices across protocol

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/oracle/BalancerOracle.sol#L114

Vulnerability details

Proof of Concept

The BalancerOracle#update() function implements a strict update window that prevents price updates within a specified time frame:

function update() external virtual onlyRole(KEEPER_ROLE) returns (uint256 safePrice_) {
  if (block.timestamp - lastUpdate < updateWaitWindow)
    revert BalancerOracle__update_InUpdateWaitWindow();
  // ... rest of the function
}

This implementation is flawed however, which is because since the function enforces a strict waiting period between updates, regardless of market conditions, during periods of high volatility, the oracle cannot update prices, and the keeper can also not do anything about it, leading to the use of outdated information.

And internal systems/logic relying on this oracle are forced to use highly deflated/inflated prices for an extended period.

Impact

Based on discussions with the sponsor examination of test files the updateWaitWindow is to be set to an hour. This means that regardless of market conditions, prices cannot be updated more frequently than once per hour. However in cryptocurrency markets, significant price movements can occur within minutes or even seconds. A one-hour update restriction means that during volatile periods, the oracle will be reporting severely outdated prices.

Recommended Mitigation Steps

Allow updates within the wait window if the price has moved beyond a certain threshold:

function update() external virtual onlyRole(KEEPER_ROLE) returns (uint256 safePrice_) {
  uint256 newPrice = calculateNewPrice();
  if (block.timestamp - lastUpdate < updateWaitWindow) {
    require(isPriceDeviationSignificant(newPrice, currentPrice), 'Price deviation not significant');
  }
  // ... rest of the update logic
}

Assessed type

Oracle

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[evokid]

Hi @koolexcrypto, I suggest there is a clear impact of this issue can put it in a validity field.

The sponsor is right and confirms that the oldest safe price is stored, but this means there's always a lag in price updates as if the users trade with old prices then we have sort of disadvantage for the protocol.

There is a catch, the protocol relies on a keeper to call update() regularly, so If the keeper fails to call update() for an extended period, the price could become severely outdated while still being considered safe within the stalePeriod.

The current implementation for price updates enforces a strict waiting period between updates, regardless of market conditions, which means that during periods of high volatility, the oracle cannot update prices, and the keeper can also not do anything about it, leading to the use of outdated information.

In which case all internal systems/logic (be it pricing/liquidations) relying on the returned price from the oracle are forced to use highly deflated/inflated prices for an extended period (a duration of stale window) allowing the gamification of this window.

This logic is flawed as shown in this report and it's duplicates, see my issue #263 and the current primary #124.

To support the claim in these reports, consider the classic Chainlink implementations of oracles & pricing:
One of two factors must be met before a price update is considered, i.e:

• Either the heartbeat of the feed say 1 hour has passed, or
• The real price of the asset has gone past the deviation threshold, for e.g say 2%.
  Quote on quote the docs:

Chainlink Data Feeds do not provide streaming data. Rather, the aggregator updates its latestAnswer when the value deviates beyond a specified threshold or when the heartbeat idle time has passed.

The above ensures the price is never stale both time/value wise.

Loopfi's current implementation leaves both them & users at the risk of exploitation. Since tech savvy users know that regardless of the current price, the keeper cannot make an update, they can use this knowledge to potentially steal value from the system.

[Bauchibred]

Agree with @evokid's comment above. Also, afaik this logic is quite common with defi protocols where price updates have a time window to be updated but are also allowed when the price difference is significant.

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[koolexcrypto]

Answered in the primary issue for transparency.

[c4-judge]

koolexcrypto marked the issue as satisfactory