Open c4-bot-6 opened 6 months ago
GalloDaSballo marked the issue as insufficient quality report
GalloDaSballo marked the issue as remove high or low quality report
GalloDaSballo marked the issue as sufficient quality report
Worth flagging as new aggregators may start with a low count
GalloDaSballo marked the issue as primary issue
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
trust1995 marked the issue as satisfactory
trust1995 marked the issue as selected for report
Referring to https://docs.chain.link/data-feeds/historical-data#roundid-in-proxy it is clear the roundId needs to be trimmed to uint64.
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.
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).
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?
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.
For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.
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
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
roundId
s 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
The
roundId
returned is used in the_recalibratePreview()
function below to get previousroundIds
. The iterationCount as we already mentioned will always be 50.OracleHelper.sol#L620C1-L630C15
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()
thelatestRoundId
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
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
Assessed type
Other