Chainlink oracles may return stale prices or may be unusable when aggregator roundId is less than 50

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseOracleHub/OracleHelper.sol#L627-L630
https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseOracleHub/OracleHelper.sol#L658-L668

Vulnerability details

Vulnerability Details

WiseLending calibrates oracles to get a heartbeat which it uses for checking the staleness of prices returned from the oracle.

To calibrate, it fetches between 3-50 inclusive historical prices and picks the second largest update time among those prices. It calls _getIterationCount() to know the number of historical prices it'll use. If the current _latestAggregatorRoundId is less than 50 (MAX_ROUND_COUNT) it uses _latestAggregatorRoundId else it uses 50.

OracleHelper.sol#L665-L667

        res = _latestAggregatorRoundId < MAX_ROUND_COUNT
            ? _latestAggregatorRoundId
            : MAX_ROUND_COUNT;

The issue with the snippet above is that _latestAggregatorRoundId will always be greater than 50 so the no. of historical prices it uses will always be 50.

It's always greater than 50 because it is fetched from the aggregator's proxy contract. The roundIds returned from the proxy are a combination of the current aggregator's roundId and phaseId. Check Chainlink docs for more info. getLatestRoundId() returns the roundId.

OracleHelper.sol#L708-L715

        (
            roundId
            ,
            ,
            ,
            ,
        ) = priceFeed[_tokenAddress].latestRoundData();

The roundId returned is used in the _recalibratePreview() function below to get previous roundIds. The iterationCount as we already mentioned will always be 50.

OracleHelper.sol#L620C1-L630C15

        uint80 i = 1;
        uint256 currentDiff;
        uint256 currentBiggest;
        uint256 currentSecondBiggest;

 ❌      while (i < iterationCount) {

            uint256 currentTimestamp = _getRoundTimestamp(
                _tokenAddress,
 ❌              latestRoundId - i
            );

The problem with the above call is that the argument, latestRoundId-1 above may not have valid data for some rounds. So calls to the Chainlink oracle for those rounds will revert.

This may occur because of the way proxy roundIds work.

E.g If the proxy returns 0x40000000000000010 as roundId.

The phaseId is 4 (roundId >> 64).
The aggregator roundId is 16 (uint64(roundId)).

After 16 iterations in _recalibratePreview() the latestRoundId will have a value of 0x40000000000000000. When the price feed is called with this roundId it will revert because it does not exist.

Check Chainlink docs for more info.

Thus if the aggregator roundId derived from the proxy roundId is less than 50, _recalibratePreview() will revert. The caller will have to wait until it is greater than 50.

Impact

1. For new price feeds, OracleHub won't be able to set a heartbeat until the aggregator roundId is greater than 50. So the new price feed would be unusable for that period.
2. For price feeds that already have a heartbeat, they won't be able to recalibrate during the period the aggregator roundId is less than 50. Which may allow the price feed return stale prices.

In both cases above the max amount of time user can wait for is 50x the official Chainlink heartbeat for the price feed i.e price feed heartbeat * 50.

For the BTC/ETH price feed this would be 50 days (24 hours * 50).

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider deriving the aggregator roundId from the proxy roundId and using that instead of the proxy roundId.

OracleHelper.sol#L665-L667

-       res = _latestAggregatorRoundId < MAX_ROUND_COUNT
-           ? _latestAggregatorRoundId
+       res = uint64(_latestAggregatorRoundId) < MAX_ROUND_COUNT
+           ? uint64(_latestAggregatorRoundId)
            : MAX_ROUND_COUNT;

Assessed type

Other

[c4-pre-sort]

GalloDaSballo marked the issue as insufficient quality report

[c4-pre-sort]

GalloDaSballo marked the issue as remove high or low quality report

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[GalloDaSballo]

Worth flagging as new aggregators may start with a low count

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[vonMangoldt]

Worth flagging as new aggregators may start with a low count

I dont understand the recommended mitigation steps. Its just typecasting uint64 to a uint80

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[trust1995]

Referring to https://docs.chain.link/data-feeds/historical-data#roundid-in-proxy it is clear the roundId needs to be trimmed to uint64.

[stalinMacias]

Hey @trust1995 . I was checking the Chainlink Documentation and the corresponding contracts, and I came up to the realization that the Chainlink Documentation only explains how the proxy queries the getRoundData on the Aggregator, but all this logic is implemented on the Proxy itself, the AggregatorProxy.getRoundData() is on charge of trimming the roundId down to uint64 before querying the aggregator, but the proxy itself receives the roundId as an uint80, thus, the WiseOracle contract is correctly integrated with the Chainlink contract. All the required logic to query the data from the aggregator is contained within the chainlink proxy, any contract interacting with the proxy doesn't need to trim the received roundId.

Based on the Chainlink contracts and the way how WiseOracle queries the getRoundData(), I think there is not any issue at all, the integration looks perfectly fine, and there is no need to implement any changes if the roundId were to be trimmed to uint64, it would disrupt the queries and it would be querying a total different values, therefore, this report looks to be invalid.

[trust1995]

I believe the root cause of the issue is correct. It is not safe to use latestRoundId - i in calculations, as the latestRoundId is crafted of a phaseID and a uint64 counter. By decreasing by a flat amount, we could find ourselves querying for an incorrect phaseID. Essentially it is a logical overflow not detected because it occurs in internal data of a larger data type (uint80).

[stalinMacias]

I see, then the root cause seems to be correct, but is the recommendation incomplete? if so, what would it be the correct way to mitigate this issue?

• At this point, I'm not arguing the validity of the report, but rather want to understand what needs to be done to fully mitigate the root cause of this issue.

[trust1995]

I see, then the root cause seems to be correct, but is the recommendation incomplete? if so, what would it be the correct way to mitigate this issue?

• At this point, I'm not arguing the validity of the report, but rather want to understand what needs to be done to fully mitigate the root cause of this issue.

Beyond the scope of PJQA, will let others discuss if interested.

[thebrittfactor]

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.