Inconsistent checks on Oracle price lead to inaccurate pricing

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L117-L129
https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L19-L67

Vulnerability details

Summary

Inconsistent price checks in getOraclePrice() will lead to wrong prices being used with detrimental effects to stability of protocol .

Vulnerability Details

In getOraclePrice(), the returned price value for "other" oracles (e.g. JPY, XAU) is checked inconsistently.

Firstly, it is mentioned in the Known Issues regarding heartbeats:

" 2 hours staleness means it can be somewhat out of date "

However, both the try block and the catch block are missing even any heartbeat check as it is stated in the docs that there should be here.
The try block does price checks in OracleCircuitBreaker() as below:

    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;

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

Secondly, there is a missing check in the catch block that the timestamp is not equal to 0, which is present in oracleCircuitBreaker() i.e. timeStamp == 0.

Thirdly, there is an inconsistent use of relational operators, where in the try block there is a check for negative prices but in the catch block the is just a check that the value is not equal to 0.

In oracleCircuitBreaker() we see the check chainlinkPrice <= 0 reverts if there is a negative price returned which, given that Chainlink's prices are int values, is relevant.
However in the catch block, as below, we see the check price == 0.

if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

Impact

Regarding the heartbeat and timestamp issues, these are esstential missing checks which protect the protocol from using stale prices.

Regarding the inconsistent application of relational operators, this means that if there is a failure in the try block because a negative price reverted there, that same incorrect price will be accepted and used in the catch block.

Both of these issues will cause huge issues to the correct functioning of the protocol via incorrect prices being fetched.

Tools Used

Manual Review
Foundry Testing

Recommendations

Given that different feeds have different heartbeats, create an Oracle data struct whereby their specific infomation can be stored and fetched to be used in the oracleCircuitBreaker() and the catch block.

Update the price check and add the timestamp check to the catch block in getOraclePrice() as:

    } catch {

        // SOME CODE

-       if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
+       if (roundID == 0 || price <= 0 || timeStamp == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

        // SOME CODE
    }

Assessed type

Oracle

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4

[raymondfam]

See #8.

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Out of scope

[Qormatic]

Hi @hansfriese ,

Appeal

I believe that a mistake has been made in ruling this issue Out of Scope. May you please review below and advise if I am missing something?

Per lookout's comment in 8 which references the Readme :

Common known issue per readme: Oracle is very dependent on Chainlink, stale/invalid prices fallback to a Uniswap TWAP. 2 hours staleness means it can be somewhat out of date.

However none of the three separate vulnerabilities mentioned in this report could be considered to be covered by that statement and hence Out of Scope.
Note: There is also a single Oracle issue in the 4naly3er report which also doesn't relate to the three issues in the report

1. The First vulnerability describes the total lack of a heartbeat / staleness check for 'other' pricefeeds. Where the Readme states "2 hours staleness means it can be somewhat out of date" it is referring to the ETH / USD pricefeed because the ETH / USD pricefeed does in fact utilise a 2 hour staleness check.
   However the 'other' feeds have neither a 2 hour staleness check nor any other staleness check whatsoever.

If that part of the statement is the basis for it's exclusion I argue that it is an incorrect exclusion.

2. The Second vulnerability describes the inconsistent checking of timestamp values in the try and catch blocks for 'other' pricefeeds. In the try block there is a check timestamp == 0, see here, to ensure tmestamp is not 0.
   In the catch block this check is missing, see here, raising the possibility that a 0 timestamp value rejected in the try block would be accepted in the catch block.

Known Issues does not appear to make this finding Out of Scope.

3. The Third vulnerability describes the inconsistent application of relational operators in the try and catch blocks for 'other' pricefeeds. In the try block there is a check chainlinkPrice <= 0 to ensure returned price is not negative or 0, see here.
   In the catch block this check is different price == 0, see here, meaning that a negative value rejected in the try block could be accepted in the catch block.

Known Issues does not appear to make this finding Out of Scope.

4. The final thing to note is that this issue is not a duplicate of 4 which refers to different issues

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c

[Qormatic]

@hansfriese what about the lack of a staleness check for other pricefeeds?
252 which only mentions staleness has just been Sponsor Confirmed?

The First vulnerability describes the total lack of a heartbeat / staleness check for 'other' pricefeeds. Where the Readme states "2 hours staleness means it can be somewhat out of date" it is referring to the ETH / USD pricefeed because the ETH / USD pricefeed does in fact utilise a 2 hour staleness check.
However the 'other' feeds have neither a 2 hour staleness check nor any other staleness check whatsoever.

[hansfriese]

Check #164 for details.

[c4-judge]

hansfriese removed the grade

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #164

[c4-judge]

hansfriese marked the issue as partial-50