Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Missing check in `KeeperProxy::_validatePrice` could lead protocol assume sequencer is up when it is not and will lead to using incorrect prices

mrmorningstar

Summary

Whenever some action is initiated by the keepers before execution protocol checks price is valid via _validatePrice :

function _validatePrice(address perpVault, MarketPrices memory prices) internal view {
// L2 Sequencer check
(
/*uint80 roundID*/,
int256 answer,
uint256 startedAt,
/*uint256 updatedAt*/,
/*uint80 answeredInRound*/
) = AggregatorV2V3Interface(sequencerUptimeFeed).latestRoundData();
bool isSequencerUp = answer == 0;
require(isSequencerUp, "sequencer is down");
// Make sure the grace period has passed after the sequencer is back up.
uint256 timeSinceUp = block.timestamp - startedAt;
require(timeSinceUp > GRACE_PERIOD_TIME, "Grace period is not over");
​
address market = IPerpetualVault(perpVault).market();
IVaultReader reader = IPerpetualVault(perpVault).vaultReader();
MarketProps memory marketData = reader.getMarket(market);
_check(marketData.indexToken, prices.indexTokenPrice.min);
_check(marketData.indexToken, prices.indexTokenPrice.max);
_check(marketData.longToken, prices.indexTokenPrice.min);
_check(marketData.longToken, prices.indexTokenPrice.max);
_check(marketData.shortToken, prices.shortTokenPrice.min);
_check(marketData.shortToken, prices.shortTokenPrice.max);
}

It first check if sequencer is up by checking answer and startedAt from latestRoundData.

If answer return 0 it shows the sequencer is up.

Vulnerability Details

But the issue that there is no addequate check for startedAt because it could return 0 if it is called during an invalid round (which could happen if there is an issue with updating sequencer status, network issues, oracle issues, etc...).
So it can happen that the startedAt have value of 0 and answer also have value of 0, which could lead protocol believe the round is valid and sequencer is up, which then could lead further to protocol using incorrect data for their prices.

Please check the screenshot from discord conversation with chainlink official engineer where he confirms that and recommend checking return value of startedAt:

Alt text

To see that actual discord message click on this link.

Also more on that can you read on Chainlink official docs here.

So if startedAt is called during invalid round and return 0 this check will pass because block.timestamp will always be greater than GRACE_PERIOD_TIME which would lead wrongly protocol to proceed further.

uint256 timeSinceUp = block.timestamp - startedAt;
require(timeSinceUp > GRACE_PERIOD_TIME, "Grace period is not over");

Impact

Due to inadequate check protocol will use incorrect prices.

Tools Used

Manual Review.

Recommendations

Make sure to check if return values are called during an invalid round by making sure startedAt is not 0 like this:

function _validatePrice(address perpVault, MarketPrices memory prices) internal view {
// L2 Sequencer check
(
/*uint80 roundID*/,
int256 answer,
uint256 startedAt,
/*uint256 updatedAt*/,
/*uint80 answeredInRound*/
) = AggregatorV2V3Interface(sequencerUptimeFeed).latestRoundData();
bool isSequencerUp = answer == 0;
require(isSequencerUp, "sequencer is down");
// Make sure the grace period has passed after the sequencer is back up.
+ require(startedAt != 0, "Invalid round");
uint256 timeSinceUp = block.timestamp - startedAt;
require(timeSinceUp > GRACE_PERIOD_TIME, "Grace period is not over");
​
address market = IPerpetualVault(perpVault).market();
IVaultReader reader = IPerpetualVault(perpVault).vaultReader();
MarketProps memory marketData = reader.getMarket(market);
_check(marketData.indexToken, prices.indexTokenPrice.min);
_check(marketData.indexToken, prices.indexTokenPrice.max);
_check(marketData.longToken, prices.indexTokenPrice.min);
_check(marketData.longToken, prices.indexTokenPrice.max);
_check(marketData.shortToken, prices.shortTokenPrice.min);
_check(marketData.shortToken, prices.shortTokenPrice.max);
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_sequencerUptimeFeed_startedAt_0_no_roundId

startedAt is only 0 when contract is not initialized on Arbitrum, but it is already initialized on Arbitrum. startedAt is sufficient for the protocol, it does not need roundID. Current documentation of Chainlink does not have this sentence: “This timestamp returns `0` if a round is invalid.“

Appeal created

mrmorningstar Submitter
9 months ago
mrmorningstar Submitter
9 months ago
mrmorningstar Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_sequencerUptimeFeed_startedAt_0_no_roundId

startedAt is only 0 when contract is not initialized on Arbitrum, but it is already initialized on Arbitrum. startedAt is sufficient for the protocol, it does not need roundID. Current documentation of Chainlink does not have this sentence: “This timestamp returns `0` if a round is invalid.“

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!