PriceFeed ignores ChainLink roundId and will treat stale price as fresh

[code423n4]

Handle

hyh

Vulnerability details

Impact

Stale 'carried over' price can be used for liquidations. This can cause various types of malfunctions and manipulated liquidations.

For example, if a portfolio consists of two inversely correlated assets, which move in opposite directions most of the times, and portfolio owner uses that knowledge to borrow against such a composition. Now first asset price is stale, second one is updated and it is in decline, so the link between assets looks to be broken and the system treats the portfolio as under collateralized, while in reality the inverse correlation holds, first asset market price rose to fully compensate the decline of the second asset price, and portfolio value isn't declined and no liquidation should take place.

Proof of Concept

Prices used in liquidations are obtained from PriceFeed contract, call sequence is as follows: Trove and Pool functions -> Whitelist.getValueVC -> Whitelist.getPrice -> PriceFeed.fetchPrice_v -> PriceFeed._getCurrentChainlinkResponse

PriceFeed determines that the price is usable via _badChainlinkResponse function. _badChainlinkResponse checks that timestamp is valid (not 0, not in the future), but do not check that it is recent: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L578

Moreover, _getCurrentChainlinkResponse function which provides core price structure used, ignores answeredInRound returned by ChainLink in the first place: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L756

Chainlink feed has two properties that can help determine if the answer is actual (price is fresh): updatedAt time https://docs.chain.link/docs/faq/#what-is-the-difference-between-the-data-feed-properties-updatedat-and-answeredinround and answeredInRound: https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

This way ChainLink roundId isn't checked in PriceFeed contract used for price discovery and carried over price is treated as fresh in all VC related computations.

Recommended Mitigation Steps

Liquidation mechanics is a core system functionality which is based on current market price observations. It is crucial to ensure that these observations are sound.

Best practice is to check that answeredInRound is equal to roundId and this way the price obtained is fresh and can be used for decision making. It's advised to save answeredInRound and check it against roundId returned, and use the price in PriceFeed response only if rounds match, treating it as stale otherwise.

It is advised to allow state changing operations only when the price is recent, i.e. it is better to reject an operation than to process it using stale parameter.

[kingyetifinance]

@LilYeti : Duplicate #91 . Thorough suggestion though, severity recommended medium due to rarity of this case.

[alcueca]

Medium Severity is a good fit, because the assets are not a direct risk, something else needs to happen first.