oxchryston - Chainlink price feed is `deprecated`, not sufficiently validated and can return `stale` prices.

[sherlock-admin]

oxchryston

medium

Chainlink price feed is deprecated, not sufficiently validated and can return stale prices.

Summary

The function _createActionInfo() uses Chainlink's deprecated latestAnswer function, this function also does not guarantee that the price returned by the Chainlink price feed is not stale and there is no additional checks to ensure that the return values are valid.

Vulnerability Detail

The internal function _createActionInfo() uses calls strategy.collateralPriceOracle.latestAnswer() and strategy.borrowPriceOracle.latestAnswer() that uses Chainlink's deprecated latestAnswer() to get the latest price. However, there is no check for if the return value is a stale data.

function _createActionInfo() internal view returns(ActionInfo memory) {
        ActionInfo memory rebalanceInfo;

        // Calculate prices from chainlink. Chainlink returns prices with 8 decimal places, but we need 36 - underlyingDecimals decimal places.
        // This is so that when the underlying amount is multiplied by the received price, the collateral valuation is normalized to 36 decimals.
        // To perform this adjustment, we multiply by 10^(36 - 8 - underlyingDecimals)
        int256 rawCollateralPrice = strategy.collateralPriceOracle.latestAnswer();
        rebalanceInfo.collateralPrice = rawCollateralPrice.toUint256().mul(10 ** strategy.collateralDecimalAdjustment);
        int256 rawBorrowPrice = strategy.borrowPriceOracle.latestAnswer();
        rebalanceInfo.borrowPrice = rawBorrowPrice.toUint256().mul(10 ** strategy.borrowDecimalAdjustment);
// More Code....
}

Impact

The function _createActionInfo() is used to return important values used throughout the contract, the staleness of the chainlinklink return values will lead to wrong calculation of the collateral and borrow prices and other unexpected behavior.

Code Snippet

https://github.com/IndexCoop/index-coop-smart-contracts/blob/317dfb677e9738fc990cf69d198358065e8cb595/contracts/adapters/AaveLeverageStrategyExtension.sol#L889

Tool used

Manual Review

Recommendation

The latestRoundData function should be used instead of the deprecated latestAnswer function and add sufficient checks to ensure that the pricefeed is not stale.

(uint80 roundId, int256 assetChainlinkPriceInt, , uint256 updatedAt, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();
            require(answeredInRound >= roundId, "price is stale");
            require(updatedAt > 0, "round is incomplete");

[0xffff11]

Sponsor comments:

Good point to switch away from using the deprecated method, which we will look into.
However from this issue it is not clear how / if there is any actual vulnerability resulting from the use of this method.
--
Agree with @ckoopmann , the proposed fix of using latestRoundData() looks reasonable to me
--
I switched to confirmed / disagree with severity as this issue is factually correct and will result in us changing the code, but does not seem to have any real adverse consequences.

[0xffff11]

I do believe that this should remain as a medium. Not just for the impact stated by the watson, but also because Chainlink might simply not support it anymore in the future.

[ckoopmann]

Switched to using latestRoundData and adding a configurable maxPriceAge that is compared against the updatedAt value. Fixed in: https://github.com/IndexCoop/index-coop-smart-contracts/pull/142

[IAm0x52]

Oracle was changed to AAVEOracle, which also fixed this issue