oracleCircuitBreaker: Not checking if price information of asset is stale

[c4-bot-9]

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

Vulnerability details

Impact

It does not check if the Chainlink oracle data for each asset is outdated. Users may trade at the wrong price.

Proof of Concept

At getOraclePrice , It does not check if the timeStamp of asset token price oracle is in a stale. If it uses outdated price information, users will trade at the wrong price.

function getOraclePrice(address asset) internal view returns (uint256) {
    AppStorage storage s = appStorage();
    AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
    uint256 protocolPrice = getPrice(asset);

    AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
    if (address(oracle) == address(0)) revert Errors.InvalidAsset();

    try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
        if (oracle == baseOracle) {
            // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
            uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
            basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
            return basePriceInEth;
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
@>              uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
            uint256 priceInEth = uint256(price).div(uint256(basePrice));
@>          oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
            return priceInEth;
        }
    } catch {
        if (oracle == baseOracle) {
            return twapCircuitBreaker();
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
@>              uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
@>          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;
        }
    }

The oracleCircuitBreaker function checks if the Chainlink oracle data for each asset is valid. It does not check if the timeStamp is in a stale too.

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();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Set the chainlinkStaleLimit for each asset and check if the price information is not outdated.

function getOraclePrice(address asset) internal view returns (uint256) {
    AppStorage storage s = appStorage();
    AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
    uint256 protocolPrice = getPrice(asset);

    AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
    if (address(oracle) == address(0)) revert Errors.InvalidAsset();

    try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
        ...
    } catch {
        if (oracle == baseOracle) {
            return twapCircuitBreaker();
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
                uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
-           if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
+           if (roundID == 0 || price == 0 || timeStamp > block.timestamp || block.timestamp > chainlinkStaleLimit[asset] + timeStamp) revert Errors.InvalidPrice();

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

function oracleCircuitBreaker(
    uint80 roundId,
    uint80 baseRoundId,
    int256 chainlinkPrice,
    int256 baseChainlinkPrice,
    uint256 timeStamp,
-   uint256 baseTimeStamp
+   uint256 baseTimeStamp,
+   address asset
) 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 > chainlinkStaleLimit[asset] + timeStamp;
    if (invalidFetchData) revert Errors.InvalidPrice();
}

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 #4.

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Out of scope

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese marked the issue as satisfactory

[hansfriese]

I will maintain this as a primary issue since it encompasses both concerns.
Additionally, I will allocate partial credit to other findings explaining only one case.

[c4-judge]

hansfriese marked the issue as selected for report

[c4-judge]

hansfriese marked the issue as primary issue

[hansfriese]

FYI, it should check if price <= 0 instead of price == 0 in the catch block.