0xnirlin - Lack of staleness check in the getLatestPrice(address _token) function can lead to triggering depeg even when there is no depeg.

[sherlock-admin]

0xnirlin

high

Lack of staleness check in the getLatestPrice(address _token) function can lead to triggering depeg even when there is no depeg.

Summary

In controllertriggerDepeg()function is dependent on the getLatestPrice(_token) function to get the price from chainlink and than check whether de-peg has occured or not. But de-peg can be triggered even when there is no de-peg.

Vulnerability Detail

Lets consider a scenario:

1. USDC Depeg occurs 30 minute before the epoch start, and as people have already participated with their deposits.
2. USDC pegs back right before the epoch start time but the chainlink round that is going on extends few minutes to hour into epoch start time.
3. Now triggerDepeg() can be called successfully as it takes the stale price and reward the beneficiaries causing loss for other user even though asset was pegged back before the epoch time.

Concept ilustrated here: https://excalidraw.com/#json=zo0qQtXxpEBbrbU6ZJ70N,qKIIiwWfpJz5X8MphG4BgA

As chainlink round can be updated in few minutes to hour, chances of such scenerio to happen are very high.

Secondly the stale price check like this one is unnecessary for two reason :
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Controllers/ControllerPeggedAssetV2.sol#L315

1. roundId is deprecated as answeredInRound and roundId are always same now, roundId don't leak.
2. Secondly our issue can cause within one round.

Impact

Loss of funds for the collateral users even though depeg never occured.

Code Snippet

  function getLatestPrice(address _token) public view returns (int256) {
       (
           ,
           /*uint80 roundId*/
           int256 answer,
           uint256 startedAt, /*uint256 updatedAt*/ /*uint80 answeredInRound*/
           ,

       ) = sequencerUptimeFeed.latestRoundData();

       // Answer == 0: Sequencer is up
       // Answer == 1: Sequencer is down
       bool isSequencerUp = answer == 0;
       if (!isSequencerUp) {
           revert SequencerDown();
       }

       // Make sure the grace period has passed after the sequencer is back up.
       uint256 timeSinceUp = block.timestamp - startedAt;
       if (timeSinceUp <= GRACE_PERIOD_TIME) {
           revert GracePeriodNotOver();
       }

       // @audit-issue - medium token to oracle problem will rise here
       AggregatorV3Interface priceFeed = AggregatorV3Interface(
           vaultFactory.tokenToOracle(_token)
       );
       (uint80 roundID, int256 price, , , uint80 answeredInRound) = priceFeed
           .latestRoundData();
       uint256 decimals = priceFeed.decimals();

       // @audit-issue - high there should be a staleness check performed here

    
       

       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();

       // @audit - medium - submit with the staleness check this is deprecated
       if (answeredInRound < roundID) revert RoundIDOutdated();

       return price;
   }

Tool used

Foundry, Manual Review

Recommendation

1. Add time based staleness check as https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Controllers/ControllerPeggedAssetV2.sol#L291
   here.
2. Only let trigger happend and take price when it has been updated after epoch start time.

Duplicate of #70

[dmitriia]

Duplicate of #422

[0xnirlin]

Escalate for 10 USDC

This issue is not duplicate of #422 but rather duplicate of #70 , though most of the duplicates mentioned in #70 do not identify the problem and just talk about the staleness check. So both #70 and #462 are high imo and other similar duplicates.
If it was some other system that was just dependent on price, and a stale price could not have that big of impact than it make sense as medium.
But whole Y2k system is dependent upon the depeg price, main selling point is insurance and premium due to depeg, so if stale price slides in its a havoc for user.

Each pair on chainlink have specific hearbeat that it updates price Xtimes in a certain time , if it is in certain range and depeg can happen within that range and price may not update for hours, for example heatbeat time for frax is 1 hour.

[sherlock-admin]

Escalate for 10 USDC

This issue is not duplicate of #422 but rather duplicate of #70 , though most of the duplicates mentioned in #70 do not identify the problem and just talk about the staleness check. So both #70 and #462 are high imo and other similar duplicates.
If it was some other system that was just dependent on price, and a stale price could not have that big of impact than it make sense as medium.
But whole Y2k system is dependent upon the depeg price, main selling point is insurance and premium due to depeg, so if stale price slides in its a havoc for user.

Each pair on chainlink have specific hearbeat that it updates price Xtimes in a certain time , if it is in certain range and depeg can happen within that range and price may not update for hours, for example heatbeat time for frax is 1 hour.

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.

[hrishibhat]

Escalation accepted

Valid duplicate of #70

[sherlock-admin]

Escalation accepted

Valid duplicate of #70

This issue's escalations have been accepted!

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