ABA - Inadequate price oracle checks

[sherlock-admin]

ABA

medium

Inadequate price oracle checks

Summary

Price of premium vault token, when triggering a depeg, is taken via Chainlink's latestRoundData function.
Not all checks are not on the latestRoundData output, thus leaving a possibility for the price to be outdated or have suffered a price manipulation that in turn would go undetected.

Concrete the issues are:

1. Missing outdated data validation on latestRoundData

There is not checked if the answer was received from latestRoundData was given an accepted time window.

Note: This is different from the sequencer's uptime, where there is a check in place.

2. No resistance for oracle price manipulation

This missing check consists of saving previously received price and compare it with the new price. If the difference is above a certain threshold then stop the execution.

Vulnerability Detail

For the second issue, see Summary, for the first issue, in ControllerPeggedAssetV2 the price for premium vault tokens when triggering a depeg is retrieved via the getLatestPrice function.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Controllers/ControllerPeggedAssetV2.sol#L273

    function getLatestPrice(address _token) public view returns (int256) {

getLatestPrice retrieves the Chainlink feed price via latestRoundData and does several checks. What it does not check is if the retrieved price is a stale one.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Controllers/ControllerPeggedAssetV2.sol#L299-L318

        (uint80 roundID, int256 price, , , uint80 answeredInRound) = priceFeed
            .latestRoundData();
        uint256 decimals = priceFeed.decimals();


        if (decimals < 18) {
            decimals = 10**(18 - (decimals));
            price = price * int256(decimals);
        } else if (decimals == 18) {
            price = price;
        } else {
            decimals = 10**((decimals - 18));
            price = price / int256(decimals);
        }


        if (price <= 0) revert OraclePriceZero();


        if (answeredInRound < roundID) revert RoundIDOutdated();


        return price;
    }

latestRoundData's 4th return value is updatedAt: Timestamp of when the round was updated.
https://docs.chain.link/data-feeds/api-reference/#latestrounddata

This value is not stored or checked for an outdated price.

Another, not so common check relating to time is to see if the round was incomplete, by checking if updateTime is 0.

Impact

The price impacts where or not a trigger depeg call reaches the strike price or not, this ultimately means the correct execution of the protocol functionality.

Code Snippet

Tool used

Manual Review

Recommendation

For issue 1:

• when launching the ControllerPeggedAssetV2 contract, also include a priceUpdateThreshold variable that stores what is the tolerated age (in seconds) of the retrieved price.
• save the updatedAt return data from latestRoundData
• check it to be != 0
• also check that it was determined less then priceUpdateThreshold seconds ago

For issue 2:

• for each token oracle save a previousValidPrice while also providing a deviation threshold for which to accept a new price.
• the threshold can be set as to not impact a potential black swan event that would cause a sudden dip in prices.

Duplicate of #70

[3xHarry]

We will consider this, however, given that the controller should be able to read multiple chainlink oracles, one oracle could have a different heartbeat which makes defining a priceUpdateThreshold hard.

[dmitriia]

To clarify, this is about price stability in general and cannot be a dup of mere updatedAt > 0 issues, which are best practice suggestions.

[0xRobocop]

Escalate for 10 USDC

Sherlock docs clearly states that external oracle manipulations are not considered valid med/high.

"External Oracle Price Manipulation: Issues related to price manipulation in an external oracle used by the contracts are not considered valid high/medium."

[sherlock-admin]

Escalate for 10 USDC

Sherlock docs clearly states that external oracle manipulations are not considered valid med/high.

"External Oracle Price Manipulation: Issues related to price manipulation in an external oracle used by the contracts are not considered valid high/medium."

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[dmitriia]

I consider this as an important surface for project to think about due to substantial reliance on Oracle feed.

But yes, this is about external Oracle manipulation/malfunction, have to agree.

[hrishibhat]

Escalation accepted

Valid duplicate of #70
Sherlock does not accept issues related to external Oracle price manipulation in general.
Considering this a duplicate of #70 for appropriate checks on the latestRoundData

[sherlock-admin]

Escalation accepted

Valid duplicate of #70
Sherlock does not accept issues related to external Oracle price manipulation in general.
Considering this a duplicate of #70 for appropriate checks on the latestRoundData

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.