wise-foundation / lending-audit

5 stars 4 forks source link

[WOH-01M] Improper Integration of Chainlink Oracles #171

Open vm06007 opened 11 months ago

vm06007 commented 11 months ago

WOH-01M: Improper Integration of Chainlink Oracles

Type Severity Location
Logical Fault WiseOracleHub.sol:L63-L69

Description:

The WiseOracleHub::latestResolver function will simply invoke the IChainlinkOracle::latestRoundData function and yield its answer without any form of sanitization in relation to the oracle's status of operation.

Impact:

Improper integration of Chainlink oracles will render the Wise protocol helpless in case of sharp market events or outdated price feeds.

Example:

/**
 * @dev Returns priceFeed latest USD value
 * by passing the underlying token address.
 */
function latestResolver(
    address _tokenAddress
)
    public
    view
    returns (uint256)
{
    (
            ,
            int256 answer,
            ,
            ,

        ) = priceFeed[_tokenAddress].latestRoundData();

    return uint256(answer);
}

Recommendation:

We advise multiple safeguards to be put in place with the primary safeguard being the validation of data staleness. As the Chainlink aggregator is an off-chain oracle, the data it reports entirely relies on timely operation of the off-chain Chainlink system.

There is no guarantee that the IChainlinkOracle::latestRoundData result is actually the latest of the market, it simply implies that it is the latest one reported on-chain. To validate that the data retrieved via the function is fresh, we advise the OracleHelper::_chainLinkIsDead functions to be utilized.

A secondary protection measure that should be put in place is extreme asset volatility protection. This measure can be put in place in two ways. A universal one that should potentially be applied across the protocol is the measurement of volatility in terms of percentage changes over a time period. This would permit sharp increases or decreases in price to allow the Wise protocol to pause borrowing or supplying of the asset in question within the system, proactively reacting to a significant market event.

An alternative, easier to implement and Chainlink-specific way to detect extreme market events is to compare the price reported by the aggregator against the minimum and maximum acceptable values the oracle is capable of reporting. If the price reported is approaching these values, it means that the oracle will soon cease reporting correct values for the asset and the asset should be measured using alternative price points (i.e. Uniswap V3 TWAPs) or be temporarily paused altogether. The minimum and maximum permitted values can be identified in the AccessControlledOffchainAggregator and require an additional data point to be stored whenever a new Chainlink feed is accepted into the protocol pointing to the access-controlled implementation.

vm06007 commented 11 months ago

Team utlized _chainLinkIsDead inside latestResolver(), however throughout the code of wiseLending where oracleHub is used Team already makes these checks to make sure it is safe to use value from latestResolver()

AccessControlledOffchainAggregator values of min and max also serve no point because they are irrelevant if we look here: https://etherscan.io/address/0xE62B71cf983019BFf55bC83B48601ce8419650CC#readContract Screenshot 2023-10-17 at 9 27 29 PM