search

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

0 stars 0 forks source link

Lack of a stale check for the base oracle in `LibOracle::oracleCircuitBreaker()` #252

Closed c4-bot-7 closed 4 months ago

c4-bot-7 commented 5 months ago

Lines of code

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#L77

Vulnerability details

Impact

This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue.

The LibOracle::oracleCircuitBreaker() does not check the stale price for the base oracle (ETH/USD price). Specifically, I'm talking about the condition: "block.timestamp > 2 hours + baseTimeStamp" (i.e., the 2-hour stale heartbeat as per the docs). Hence, the function cannot verify whether or not the baseChainlinkPrice is stale.

Consequently, the oracleCircuitBreaker() will not revert the transaction as expected if the baseChainlinkPrice is stale. As a result, the protocol's core functions will consume the stale price, harming the protocol's and users' funds.

Proof of Concept

The oracleCircuitBreaker() does not check the stale price for the base oracle (ETH/USD price) (i.e., the condition: "block.timestamp > 2 hours + baseTimeStamp") compared to the baseOracleCircuitBreaker().

Without the stale price check mentioned above, the oracleCircuitBreaker() cannot verify whether the baseChainlinkPrice is stale. For this reason, the oracleCircuitBreaker() will not revert the transaction as expected if the baseChainlinkPrice is stale.

    //@audit -- This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue
    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
@1      bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@1          || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0; //@audit -- The oracleCircuitBreaker() lacks checking the stale price for the base oracle (ETH/USD price) (condition: "block.timestamp > 2 hours + baseTimeStamp")

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

    ...

    function baseOracleCircuitBreaker(
        uint256 protocolPrice,
        uint80 roundId,
        int256 chainlinkPrice,
        uint256 timeStamp,
        uint256 chainlinkPriceInEth
    ) private view returns (uint256 _protocolPrice) {
        bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@2          || block.timestamp > 2 hours + timeStamp; //@audit -- Whereas the baseOracleCircuitBreaker() already checks that condition as per the docs
        uint256 chainlinkDiff =
            chainlinkPriceInEth > protocolPrice ? chainlinkPriceInEth - protocolPrice : protocolPrice - chainlinkPriceInEth;
        bool priceDeviation = protocolPrice > 0 && chainlinkDiff.div(protocolPrice) > 0.5 ether;

        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the stale price check for the base oracle (the condition: block.timestamp > 2 hours + baseTimeStamp) in the oracleCircuitBreaker().

    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
        bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
-           || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
+           || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0
+           || block.timestamp > 2 hours + baseTimeStamp; //@audit -- Add the stale price check for the base oracle (ETH/USD price)

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

Assessed type

Oracle

c4-pre-sort commented 5 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #4

raymondfam commented 5 months ago

See #4.

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Out of scope

serial-coder commented 4 months ago

Hi @hansfriese,

Appeal

This issue is not a duplicate of #4.

As #4 refers to the missing checks for roundId, price, updateTime, and answeredInRound, which was flagged as a known issue. This issue refers to an incorrect implementation of the base oracle (ETH/USD price) in the oracleCircuitBreaker().

To elaborate, the docs (bullet 5) mentioned that the check: "timeStamp + 2 hours < block.timestamp" (i.e., the 2-hour heartbeat for the base price) must be performed to verify the Chainlink data.

Apparently, the baseOracleCircuitBreaker() correctly checks the base oracle in question (see @2 in the snippet below). However, the sponsor didn't implement this check for the same base oracle in the oracleCircuitBreaker() (see @1).

Hence, this issue is not a duplicate of #4, and it raises another point that is a valid issue because the sponsor implemented the use of base oracle (ETH/USD price) incorrectly (please refer to the docs (bullet 5)).

    //@audit -- This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue
    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
@1      bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@1          || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0; //@audit -- The oracleCircuitBreaker() lacks checking the stale price for the base oracle (ETH/USD price) (condition: "block.timestamp > 2 hours + baseTimeStamp")

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

    ...

    function baseOracleCircuitBreaker(
        uint256 protocolPrice,
        uint80 roundId,
        int256 chainlinkPrice,
        uint256 timeStamp,
        uint256 chainlinkPriceInEth
    ) private view returns (uint256 _protocolPrice) {
        bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@2          || block.timestamp > 2 hours + timeStamp; //@audit -- Whereas the baseOracleCircuitBreaker() already checks that condition as per the docs
        uint256 chainlinkDiff =
            chainlinkPriceInEth > protocolPrice ? chainlinkPriceInEth - protocolPrice : protocolPrice - chainlinkPriceInEth;
        bool priceDeviation = protocolPrice > 0 && chainlinkDiff.div(protocolPrice) > 0.5 ether;

        ...
    }
ditto-eth commented 4 months ago

Agree, this seems like a valid issue. Returned base oracle price could be stale.

c4-sponsor commented 4 months ago

ditto-eth (sponsor) confirmed

c4-judge commented 4 months ago

hansfriese marked the issue as satisfactory

c4-judge commented 4 months ago

hansfriese marked the issue as not a duplicate

c4-judge commented 4 months ago

hansfriese marked the issue as selected for report

c4-judge commented 4 months ago

hansfriese marked the issue as primary issue

c4-judge commented 4 months ago

hansfriese marked the issue as not selected for report

c4-judge commented 4 months ago

hansfriese marked the issue as duplicate of #164

c4-judge commented 4 months ago

hansfriese marked the issue as partial-50