PriceFeed Manual Review Findings

PFD-01M: Significant Centralization of Sensitive Functionality

Type	Severity	Location
Centralization Concern		PriceFeed.sol:L71-L85, L87-L89, L91-L93

Description:

The PriceFeed oracle system can be adjusted by the owner and / or adminContract of the Gravita Protocol system at will.

Example:

Copy
71function addOracle(
72    address _token,
73    address _chainlinkOracle,
74    bool _isEthIndexed
75) external override isController {
76    AggregatorV3Interface newOracle = AggregatorV3Interface(_chainlinkOracle);
77    _validateFeedResponse(newOracle);
78    if (registeredOracles[_token].exists) {
79        uint256 timelockRelease = block.timestamp.add(_getOracleUpdateTimelock());
80        queuedOracles[_token] = OracleRecord(newOracle, timelockRelease, true, true, _isEthIndexed);
81    } else {
82        registeredOracles[_token] = OracleRecord(newOracle, block.timestamp, true, true, _isEthIndexed);
83        emit NewOracleRegistered(_token, _chainlinkOracle, _isEthIndexed);
84    }
85}
86
87function deleteOracle(address _token) external override isController {
88    delete registeredOracles[_token];
89}
90
91function deleteQueuedOracle(address _token) external override isController {
92    delete queuedOracles[_token];
93}

Recommendation:

We advise these functions to be invoke-able solely by governance mechanisms as they present a significant centralization threat to the protocol. To note, the oracle update timelock can be bypassed entirely by invoking PriceFeed::deleteOracle followed by PriceFeed::addOracle, a trait that should also be addressed in the system.

Alleviation:

The code was revised to instead ensure that the PriceFeed::addOracle (now labelled PriceFeed::setOracle) function can be solely invoked by a timelock instead of a centralized entity. As such, we consider this exhibit alleviated provided that the timelock is in use by a multi-signature wallet, DAO, or similar multi-party collective.

PFD-02M: Incorrect Error Handling

Type	Severity	Location
Logical Fault		PriceFeed.sol:L281

Description:

The catch clause of the first try-catch construct in PriceFeed::_fetchCurrentFeedResponse is incorrect as it will continue execution of the function. As such, if the _priceAggregator does not implement the decimals function but implements the latestRoundData function it will be accepted by the contract as correct with a decimal accuracy of 0 incorrectly.

Impact:

The potential of an aggregator supporting the latestRoundData function but not the decimals one is inexistent, however, custom oracle implementations may fall into this category and would cause the system to misbehave greatly.

Example:

Copy
274function _fetchCurrentFeedResponse(AggregatorV3Interface _priceAggregator)
275    internal
276    view
277    returns (FeedResponse memory response)
278{
279    try _priceAggregator.decimals() returns (uint8 decimals) {
280        response.decimals = decimals;
281    } catch {}
282    try _priceAggregator.latestRoundData() returns (
283        uint80 roundId,
284        int256 answer,
285        uint256, /* startedAt */
286        uint256 timestamp,
287        uint80 /* answeredInRound */
288    ) {
289        response.roundId = roundId;
290        response.answer = answer;
291        response.timestamp = timestamp;
292        response.success = true;
293    } catch {}
294}

Recommendation:

We advise the code to instead yield response directly in the first catch clause, ensuring that the Chainlink response is treated as invalid if the feed does not support the decimals function similarly to the Liquity implementation.

Alleviation:

The code was updated to yield the empty response akin to the catch block of the latestRoundData invocation, ensuring that the code properly fails if the decimals function is not supported by the Chainlink oracle being added.

PFD-03M: Inexistent Initialization of Price

Type	Severity	Location
Logical Fault		PriceFeed.sol:L71-L85

Description:

The registration of an oracle to the system via PriceFeed::addOracle does not set an initial price for the asset in contrast to the Liquity implementation. As such, if PriceFeed::fetchPrice is invoked when the Chainlink oracle stops behaving properly the yielded lastTokenGoodPrice will be 0 incorrectly.

Impact:

If an oracle is added to the system and immediately stops behaving properly, the PriceFeed::fetchPrice function will yield an incorrect price of 0 that will be consumed by its callers.

Example:

Copy
71function addOracle(
72    address _token,
73    address _chainlinkOracle,
74    bool _isEthIndexed
75) external override isController {
76    AggregatorV3Interface newOracle = AggregatorV3Interface(_chainlinkOracle);
77    _validateFeedResponse(newOracle);
78    if (registeredOracles[_token].exists) {
79        uint256 timelockRelease = block.timestamp.add(_getOracleUpdateTimelock());
80        queuedOracles[_token] = OracleRecord(newOracle, timelockRelease, true, true, _isEthIndexed);
81    } else {
82        registeredOracles[_token] = OracleRecord(newOracle, block.timestamp, true, true, _isEthIndexed);
83        emit NewOracleRegistered(_token, _chainlinkOracle, _isEthIndexed);
84    }
85}

Recommendation:

We advise the PriceFeed::addOracle function to set the latest good price as well, ensuring that the system will behave properly under all circumstances.

Alleviation:

The PriceFeed::addOracle (now labelled PriceFeed::setOracle) function now properly extracts and consumes the most recent responses of the Chainlink oracle being added, rendering the behaviour outlined in the exhibit impossible in the latest iteration of the codebase.

PFD-04M: Incorrect Lido Staked ETH Value Assumption

Type	Severity	Location
Logical Fault		PriceFeed.sol:L171

Description:

The referenced statement will attempt to query the stETH price using a USD oracle and if it does not exist, it will treat the stETH equivalent of the wstETH as one-to-one interchangeable with ETH thus using the price of ETH to calculate the price of the wstETH value.

Impact:

The arbitrage opportunities introduced can lead to the creation of bad debt in the system and can be exaggerated via flash-loans.

Example:

Copy
168function _fetchNativeWstETHPrice() internal returns (uint256 price) {
169    uint256 wstEthToStEthValue = _getWstETH_StETHValue();
170    OracleRecord storage stEth_UsdOracle = registeredOracles[stethToken];
171    price = stEth_UsdOracle.exists ? this.fetchPrice(stethToken) : _calcEthPrice(wstEthToStEthValue);
172    _storePrice(wstethToken, price);
173}

Recommendation:

We advise this fallback mechanism to be omitted as staked counterparts of ETH always trade at either a premium or a loss in comparison to the actual ETH asset, causing PriceFeed::_fetchNativeWstETHPrice to introduce arbitrage opportunities.

Alleviation:

Assets that relate to ETH2.0 wrapped ETH are no longer treated as a special case by the oracle, instead utilizing the traditional Chainlink-related methodology to assess their price. As such, we consider this exhibit fully alleviated.

PFD-05M: Incorrect Lido Staked ETH Price Usage

Type	Severity	Location
Logical Fault		PriceFeed.sol:L171

Description:

The referenced statement will fetch the price of the stETH token and store it as the price of the wstETH token which is incorrect.

Impact:

The price reported per unit of wstETH will always be incorrect if a USD oracle has been defined for stETH as it will yield the price of stETH and not wstETH.

Example:

Copy
168function _fetchNativeWstETHPrice() internal returns (uint256 price) {
169    uint256 wstEthToStEthValue = _getWstETH_StETHValue();
170    OracleRecord storage stEth_UsdOracle = registeredOracles[stethToken];
171    price = stEth_UsdOracle.exists ? this.fetchPrice(stethToken) : _calcEthPrice(wstEthToStEthValue);
172    _storePrice(wstethToken, price);
173}

Recommendation:

We advise the price of the stETH token fetched to be multiplied by the wstEthToStEthValue as it represents the exchange rate between wstETH and stETH, the former's price being what we are interested in.

Alleviation:

Assets that relate to ETH2.0 wrapped ETH are no longer treated as a special case by the oracle, instead utilizing the traditional Chainlink-related methodology to assess their price. As such, we consider this exhibit fully alleviated.