Oracle vulnerability when the price of an underlying asset drops significantly

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xe0e09f38d16ea0e9202aa62cbca677b33fc61b61006f79b3ea7c1f674c4a312c Severity: medium

Description:

Impact

Oracle feed / hub will report inaccurate price during a crash. Borrowing against an inflated price asset (at chainlink's minPrice) can lead to significant accumulation of bad debt.

Description

Chainlink feeds use underlying aggregators. Chainlink aggregators have a built in mechanism that prevents the price going outside of a predefined price range. During significant price drops, the oracle will continue to report the minimum price instead of the actual asset price. This would allow users to borrow against the asset at an inflated price.

If the asset's price falls below the minPrice threshold, the protocol will continue to value the token at minPrice instead of the market value. This would enable users to accumulate substantial amounts of bad debt, potentially leading to the protocol's bankruptcy.

CRV / USD Chainlink Feed - Etherscan

  function latestRoundData()
    public
    view
    virtual
    override
    returns (
      uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound
    )
  {
    Phase memory current = currentPhase;

    (
      uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 ansIn
    ) = current.aggregator.latestRoundData(); // @audit uses underlying aggregator

    return addPhaseIds(roundId, answer, startedAt, updatedAt, ansIn, current.id);
  }

TWAP's inablity to defend

The price difference check will pass when the TWAP is near the minPrice during a crash. A TWAP oracle can't really defend against this scenario, nor potentially comparing with a third oracle. Once the Uniswap oracle reports the underlying asset's value at zero, latestResolver() will skip the price difference comparison, simply returning the minPrice from Chainlink.

latestResolver()

The first check chainLinkIsDead() will pass since everything works as intended (heartbeat and update time up to date). If there is a uniswap TWAP defined, it will compare the price difference with Chainlink, however during the price drop there's a short to medium timeframe when the TWAP price will be close to minPrice. If an asset's value drops to zero according to uniswap oracle, the difference check is skipped, as is when a TWAP oracle is not defined for tokenAddress.

contracts/WiseOracleHub/WiseOracleHub.sol - latestResolver()

    function latestResolver(
        address _tokenAddress
    )
        public
        view
        returns (uint256)
    {
        if (chainLinkIsDead(_tokenAddress) == true) {
            revert OracleIsDead();
        }

        UniTwapPoolInfo memory uniTwapPoolInfoStruct = uniTwapPoolInfo[
            _tokenAddress
        ];

        uint256 fetchTwapValue;

        if (uniTwapPoolInfoStruct.oracle > ZERO_ADDRESS) {
            fetchTwapValue = latestResolverTwap(
                _tokenAddress
            );
        }

        uint256 answer = _getChainlinkAnswer(
            _tokenAddress
        );

        if (fetchTwapValue > 0) {

            uint256 relativeDifference = _getRelativeDifference(
                answer,
                fetchTwapValue
            );

            _compareDifference(
                relativeDifference
            );
        }

        return answer;
    }

Recommendation

Consider to compare minPrice and optionally maxPrice of the underlying aggregators against Chainlink's returned answer from latestResolver() and revert the call if it's outside or equals the bounds.

        uint256 answer = _getChainlinkAnswer(
            _tokenAddress
        );
+       if (answer >= maxPrice || answer <= minPrice) revert PriceOutOfBounds()

You can get the minPrice and maxPrice of the aggregator by calling .aggregator().minPrice() on top of the price feed. Aggregator

Real world exploits

Rekt - Venus Finance

[0xfuje]

Also worth to mention that TWAP oracles are more susceptible to manipulation, so the attacker could intentionally keep the TWAP price within a range that does not revert during _compareDifference() call as long as it remains profitable.

[vonMangoldt]

Can you include a POC where the Twap would also report 0 eventhough there is liquidity? @0xfuje

[vonMangoldt]

Because there are instances where we cant have a twap for the underlying this definetly improves security and is therfore considered valid I am just curious with the question above in case we have twap if it would be necessary ( I dont think so)

[0xfuje]

Thank you @vonMangoldt!

Can you include a POC where the Twap would also report 0 eventhough there is liquidity?

Did more research about it and I don't think that's possible (after deployment). Small note that the first price observation of a Uni TWAP oracle is 0, upon initialization, and this will be overwritten by the first "real" observation (e.g. after a swap), but it's highly unlikely you are going to use a uni v3 pool that was just deployed, without any newer observations.

I will comment here if I could find a realistic edge case where a TWAP could return zero with liquidity still in the pool.

[00xSEV]

I think it would also be helpful to provide proof that the feeds used by the protocol have this vulnerability

https://docs.chain.link/data-feeds#check-the-latest-answer-against-reasonable-limits

The data feed aggregator includes both minAnswer and maxAnswer values. On most data feeds, these values are no longe used and they do not stop your application from reading the most recent answer.

See also https://docs.chain.link/data-feeds/api-reference#variables-and-functions-in-accesscontrolledoffchainaggregator

This value is no longer used on most Data Feeds. Evaluate if your use case for Data Feeds requires a custom circuit breaker and implement it to meet the needs of your application.

[0xfuje]

On most data feeds, these values are no longer used and they do not stop your application from reading the most recent answer.

This value is no longer used on most Data Feeds. Evaluate if your use case for Data Feeds requires a custom circuit breaker and implement it to meet the needs of your application.

Yes that's the point, data feeds do not make use of their aggregator's min / max values. This means the price feed won't revert if the answer hits minAnswer or maxAnswer. That's the responsibility of the project to bound the values (if they choose to do), either based on the aggregator's min max values or custom defined ones.

The most recent answer would be minAnswer in case an asset's price drops below minAnswer, as underlying aggregators can't return a value below minAnswer and above maxAnswer. Using customly configured min / max values to each asset to revert is the matter of is it worth to revert the value earlier? (since chainlink would still provide accurate market values if between the bound), but reverting based on chainlink's min max answer values would already prevent the vulnerability where an attacker can borrow an asset from the protocol with an inflated price (compared to the real market price) which would lead to catastrophic outcomes (bad debt / potential insolvency of that market).