PriceOracle has invalid checks on price staleness.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/PriceOracle.sol#L124-L125

Vulnerability details

Impact

PriceOracle has invalid checks on price staleness.

Proof of Concept

There are two checks on price staleness in PriceOracle::getAssetPriceFromChainlink. But both checks are invalid. (1) updatedAt != 0 In chainlink aggregator, the price is updated at a set heartbeat and a threshold of deviation. updatedAt should be used to check if the answer is within the hearbeat or acceptable time limits. See doc.

(2) answeredInRound >= roundId answeredInRound is deprecated and shouldn't be used. see doc.

//src/PriceOracle.sol
  /// @notice Query the price of asset from chainlink oracle
  function getAssetPriceFromChainlink(address asset) public view returns (uint256) {
    AggregatorV2V3Interface sourceAgg = assetChainlinkAggregators[asset];
    require(address(sourceAgg) != address(0), Errors.ASSET_AGGREGATOR_NOT_EXIST);

    (uint80 roundId, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = sourceAgg.latestRoundData();
    require(answer > 0, Errors.ASSET_PRICE_IS_ZERO);
|>  require(updatedAt != 0, Errors.ORACLE_PRICE_IS_STALE);
|>  require(answeredInRound >= roundId, Errors.ORACLE_PRICE_IS_STALE);

    return uint256(answer);
  }

(https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/PriceOracle.sol#L124-L125)

Tools Used

Manual

Recommended Mitigation Steps

Consider using asset-specific hearbeat(e.g. ETH/USD has 1 hour hearbeat) and check against (block.timestamp - updatedAt).

Assessed type

Oracle

[c4-judge]

MarioPoneder marked the issue as primary issue

[c4-judge]

MarioPoneder marked the issue as selected for report

[c4-judge]

MarioPoneder marked the issue as satisfactory

[thorseldon]

Checking the stale interval or grace period of oracle price, it's maybe better do this as suggested, but it's hard to set or predict the appropriate interval time.

We suggest adjust the severity level to informative.

This finding looks like same with 24 (https://github.com/code-423n4/2024-07-benddao-findings/issues/24).

[MarioPoneder]

I like to keep this group of issues separate from the #24 group due to distinct core issues of stale oracle prices itself vs. sequencer down which both should be checked individually.
Historically, findings like this which can have a price impact were awarded with Medium severity on C4.