wise-foundation / lending-audit

5 stars 4 forks source link

[OHR-05M] Potentially Abnormally Low Heartbeat #172

Open vm06007 opened 11 months ago

vm06007 commented 11 months ago

OHR-05M: Potentially Abnormally Low Heartbeat

Type Severity Location
Logical Fault OracleHelper.sol:L94, L116-L137, L139

Description:

The OracleHelper::_recalibratePreview function will attempt to calculate the second-biggest time delta in which a Chainlink price update was performed, however, there is no restriction in the Chainlink protocol's round deltas.

As such, the OracleHelper::_recalibratePreview function may yield incorrect results if a small iterationCount sample is selected and the second-biggest delay is significantly smaller than the first one.

Impact:

A misconfigured heartbeat that is abnormally small would cause the Chainlink oracle to be reported as "dead" even when it functions as expected.

Example:

uint256 currentDiff;
uint256 currentBiggest;
uint256 currentSecondBiggest;

for (uint80 i = 1; i < iterationCount; ++i) {

    uint256 currentTimestamp = _getRoundTimestamp(
        _tokenAddress,
        phaseId,
        latestAggregatorRoundId - i
    );

    currentDiff = latestTimestamp
        - currentTimestamp;

    latestTimestamp = currentTimestamp;

    if (currentDiff >= currentBiggest) {

        currentSecondBiggest = currentBiggest;
        currentBiggest = currentDiff;

    } else if (currentDiff > currentSecondBiggest && currentDiff < currentBiggest) {
        currentSecondBiggest = currentDiff;
    }
}

return currentSecondBiggest;

Recommendation:

We advise a "minimum" heartbeat that is enforced in case updates happened very frequently in the last iterationCount instances. For this to function properly, the OracleHelper::_recalibratePreview function would need to evaluate whether currentSecondBiggest is greater than the minimum. If it is not, it should then evaluate the currentBiggest value and if that too does not exceed the contract-imposed minimum the minimum itself should be yielded by the function.

vm06007 commented 11 months ago

Chainlinks deviation percentage necessary for an update is highest on ETH mainnet since gas is most costly there. (e.g 0.5% for eth/usd). Backtesting gave no indication of 50 rounds having not shown second highest to be actual heartbeat. For other chains with much smaller deviation percentage and less gascost 50 loops can be increased to 500 loops or even more. Worst case recalibration necessary.. thats it.