Positions may be liquidated due to incorrect implementation of Oracle logic

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L210

Summary

Steadefi checks for historical data to make sure that last price update are within maximum delya allowed and in the range of maximum % deviation allowed.

But checking the historical data is incorrect according to the chainlink docs which can damage some serious logic with in the protcol

Vulnerability Details

Vault calls ChainlinkARBOracle.consult(token) to get the fair price from chainlink oracle

File:

  function consult(address token) public view whenNotPaused returns (int256, uint8) {
    address _feed = feeds[token];

    if (_feed == address(0)) revert Errors.NoTokenPriceFeedAvailable();

    ChainlinkResponse memory chainlinkResponse = _getChainlinkResponse(_feed);
    ChainlinkResponse memory prevChainlinkResponse = _getPrevChainlinkResponse(_feed, chainlinkResponse.roundId);//@audit incorrect way to get historical data
    if (_chainlinkIsFrozen(chainlinkResponse, token)) revert Errors.FrozenTokenPriceFeed();
    if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse, token)) revert Errors.BrokenTokenPriceFeed();

 return (chainlinkResponse.answer, chainlinkResponse.decimals);
  }

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L62

which calls an interval function _getPrevChainlinkResponse() and try to fetch previous roundId price and other details

  function _getPrevChainlinkResponse(address _feed, uint80 _currentRoundId) internal view returns (ChainlinkResponse memory) {
    ChainlinkResponse memory _prevChainlinkResponse;

    (
      uint80 _roundId,
      int256 _answer,
      /* uint256 _startedAt */,
      uint256 _timestamp,
      /* uint80 _answeredInRound */
    ) = AggregatorV3Interface(_feed).getRoundData(_currentRoundId - 1);

    _prevChainlinkResponse.roundId = _roundId;
    _prevChainlinkResponse.answer = _answer;
    _prevChainlinkResponse.timestamp = _timestamp;
    _prevChainlinkResponse.success = true;

    return _prevChainlinkResponse;
  }

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L210

But this is incorrect way of fetching historical data. chainlink docs say: `Oracles provide periodic data updates to the aggregators. Data feeds are updated in rounds. Rounds are identified by their roundId, which increases with each new round. This increase may not be monotonic. Knowing the roundId of a previous round allows contracts to consume historical data.

The examples in this document name the aggregator roundId as aggregatorRoundId to differentiate it from the proxy roundId.` check here

so it is not mendatory that there will be valid data for currentRoundID-1. if there is not data for currentRooundId-1 then _badPriceDeviation(currChainlinkResponse,PrevResponse) check here will return true. Hence vault won't able to get the price of token at some specific times

Impact

1. In worse case keeper won't able to get the price of token so rebalancing , debt repay won't be possible leading to liquidation breaking the main important factor of protocol
2. Almost 70% of vault action is dependent on price of a token and not getting price will make them inactive affecting net APR

Tools Used

Manual Review

Recommendations

As chainlink docs says. Increase in roundId may not be monotonic so loop through the previous roundID and fetch the previoous roundId data

pseudo code

 iterate (from roundId-1 to untill we get previous first data corressponding to roundID){
    if(data present for roundID){
        fetch the data and return
    }else{
        again iterate to get the data
    }
 }