search

code-423n4 / 2024-03-dittoeth-findings

0 stars 0 forks source link

The `LibOracle::getOraclePrice()` can return miscalculated prices due to flash crash events #256

Closed c4-bot-2 closed 6 months ago

c4-bot-2 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L33-L46 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L50-L65 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L43 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L37 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L27 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L54 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L44 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L125-L126 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L60

Vulnerability details

Impact

The LibOracle::getOraclePrice() does not detect or handle the Chainlink aggregators' minAnswer and maxAnswer. For instance, in case of a significant price drop, the asset price reported by the Chainlink price feed will continue to be at the pre-determined minAnswer instead of the actual price (below the minAnswer). For more, refer to the case of Venus Protocol and Blizz Finance in the crash of LUNA.

In other words, in the flash crash event, Chainlink's price feeds can report incorrect basePrice and/or price variables. These incorrectly reported price variables lead to miscalculating the basePriceInEth (base price in ETH) or priceInEth (non-base price in ETH).

Whereas the calculation of the basePriceInEth has a price deviation detection mechanism to partially mitigate the issue, the calculation of the priceInEth does not validate or handle the issue.

Subsequently, the getOraclePrice() will return the miscalculated basePriceInEth and/or priceInEth (especially this one) to the protocol's core functions to consume, harming the protocol's and users' funds.

Please refer to the Proof of Concept section for more details.

Proof of Concept

This PoC section can be categorized into three sub-sections.

  1. Introduction
  2. Vulnerability Details
  3. Lack of Detecting Chainlink Aggregators' minAnswer and maxAnswer

1. Introduction

The getOraclePrice() calculates the base price in ETH (ETH/USD asset oracle) or non-base price in ETH (non-ETH/USD asset oracle) for the protocol's core functions to consume. The function can be divided into four code paths.

  1. To calculate the base price in ETH (ETH/USD asset oracle) in a try case
  2. To calculate the base price in ETH (ETH/USD asset oracle) in a catch case
  3. To calculate the non-base price in ETH (non-ETH/USD asset oracle) in a try case
  4. To calculate the non-base price in ETH (non-ETH/USD asset oracle) in a catch case

In the first code path, the base price in ETH (basePriceInEth) is calculated from the basePrice fed by Chainlink's ETH/USD price feed. The function validates and handles the flash crash event using the price deviation detection mechanism. In the second code path, the function will derive the basePriceInEth from Uniswap's TWAP instead (in case Chainlink's price feed reverts a query). Both code paths can partially mitigate the flash crash event. Thus, I will leave out discussing these two code paths for brevity's sake.

This report will discuss the third and fourth code paths of the getOraclePrice(), which calculate the non-base price in ETH (non-ETH/USD asset oracle). I discovered neither code path validates or handles the flash crash event. As a result, the getOraclePrice() will return a miscalculated non-base price (i.e., the priceInEth variable).

2. Vulnerability Details

The third code path of the getOraclePrice() calculates the priceInEth using uint256(price).div(uint256(basePrice))(see @1.1 below) . Where the price (see @1.2) is fed by Chainlink's non-ETH/USD price feed (e.g., XAU/USD), and the basePrice (see @1.3) is from Chainlink's ETH/USD price feed.

Meanwhile, the fourth code path of the getOraclePrice() calculates the priceInEth using uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv) (see @2.1). Where the price (see @2.2) is reported by Chainlink's non-ETH/USD price feed, and the twapInv (see @2.3) is returned from Uniswap's TWAP (WETH/USDC pool).

    function getOraclePrice(address asset) internal view returns (uint256) {
        ...

@1.3    try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
            if (oracle == baseOracle) {
                // ... Code Path #1 ...
            } else {
                // ... Code Path #3 ...

                // prettier-ignore
                (
                    uint80 roundID,
@1.2                int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
@1.1            uint256 priceInEth = uint256(price).div(uint256(basePrice));
                oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
                return priceInEth;
            }
        } catch {
            if (oracle == baseOracle) {
                // ... Code Path #2 ...
            } else {
                // ... Code Path #4 ...

                // prettier-ignore
                (
                    uint80 roundID,
@2.2                int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
                if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

@2.3            uint256 twapInv = twapCircuitBreaker();
@2.1            uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
                return priceInEth;
            }
        }
    }

As mentioned earlier, Chainlink's price aggregators have pre-determined minAnswer and maxAnswer price ranges they will return. In a flash crash event, an asset price can fall significantly below the pre-determined minAnswer. However, the aggregator will continue to report its minAnswer. For more, refer to the case of Venus Protocol and Blizz Finance in the crash of LUNA.

For example, the following lists some assets the protocol supports (see docs) and their pre-defined minAnswer/maxAnswer (represented by 8 decimals precision).

I noticed that the getOraclePrice() does not detect such a flash crash event. If the market crashes, the price and/or basePrice reported by Chainlink can be higher than the actual asset price (below the minAnswer).

As a result, the incorrectly reported price or basePrice leads to miscalculating the priceInEth.

3. Lack of Detecting Chainlink Aggregators' minAnswer and maxAnswer

This sub-section shows that the getOraclePrice() does not detect or handle the Chainlink aggregators' minAnswer and maxAnswer.

After calculating the priceInEth, the third code path of the getOraclePrice() validates the obtained price and basePrice by invoking the oracleCircuitBreaker() (see @3.1 below).

As you can see, the oracleCircuitBreaker() does not verify both price variables against their corresponding aggregators' minAnswer or maxAnswer (see @3.2). Therefore, the function cannot be aware of the flash crash events.

    function getOraclePrice(address asset) internal view returns (uint256) {
        ...

        try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
            if (oracle == baseOracle) {
                // ... Code Path #1 ...
            } else {
                // ... Code Path #3 ...

                // prettier-ignore
                (
                    uint80 roundID,
                    int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
                uint256 priceInEth = uint256(price).div(uint256(basePrice));
@3.1            oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
                return priceInEth;
            }
        } catch {
            if (oracle == baseOracle) {
                // ... Code Path #2 ...
            } else {
                // ... Code Path #4 ...
            }
        }
    }

    ...

    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
@3.2    bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@3.2        || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

        if (invalidFetchData) revert Errors.InvalidPrice();
    }

Also, be the same in the case of the fourth code path of the getOraclePrice(). The function does not verify the obtained price against its corresponding aggregator's minAnswer or maxAnswer (see @4 below). Hence, the function cannot detect whether the flash crash event occurs.

    function getOraclePrice(address asset) internal view returns (uint256) {
        ...

        try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
            if (oracle == baseOracle) {
                // ... Code Path #1 ...
            } else {
                // ... Code Path #3 ...
            }
        } catch {
            if (oracle == baseOracle) {
                // ... Code Path #2 ...
            } else {
                // ... Code Path #4 ...

                // prettier-ignore
                (
                    uint80 roundID,
                    int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
@4              if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

                uint256 twapInv = twapCircuitBreaker();
                uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
                return priceInEth;
            }
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a mechanism to detect the Chainlink aggregators' minAnswer and maxAnswer for both baseOracle (ETH/USD) and oracle (non-ETH/USD) price feeds.

Suppose the reported basePrice or price reaches the minAnswer or maxAnswer (e.g., the flash crash event occurs). In that case, the mechanism can revert a transaction or even fall back to consume a more accurate price from Uniswap's TWAP or other oracle services instead (if necessary).

Assessed type

Oracle

c4-pre-sort commented 6 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

raymondfam marked the issue as duplicate of #98

raymondfam commented 6 months ago

See #98.

c4-judge commented 5 months ago

hansfriese marked the issue as unsatisfactory: Insufficient proof

serial-coder commented 5 months ago

Hi @hansfriese,

Appeal

Point 1

I don't understand why the judge marked this issue as insufficient proof.

Especially when considering these valid issues in the past contests:

Could you have a second look at the issue details thoroughly again?

Point 2

It may be subjective to decide whether this issue is validated or not. However, this issue affected Venus Protocol and Blizz Finance in the past.

Here are some of the valid ref issues in the past contests: ref-1 (also reported by the lookout himself), ref-2, ref-3, ref-4, and ref-5.

The following lists some assets the protocol supports (see docs) and their pre-defined minAnswer/maxAnswer (represented by 8 decimals precision).

If this issue is invalidated, but the incident happens in the future, who will be responsible for the determination?

hansfriese commented 5 months ago

It's a known issue - M-03 revert if outside of min/max answer: Shouldn't happen for a feed like ETH/USD, but will add checks for minAnswer and maxAnswer according to chainlink docs