DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Insufficient checks to confirm the correct status of `sequencerUptimeFeed`

Summary

The KeeperProxy contract has sequencerUptimeFeed checks in place to assert if the sequencer on Arbitrum is running, but these checks are not implemented correctly. Since the protocol implements some checks for the sequencerUptimeFeed status, it should implement all of the checks.

Vulnerability Details

The following implemented check in _validatePrice is useless if its called in an invalid round

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

as startedAt will be 0, the arithmetic operation block.timestamp - startedAt will result in a value greater than GRACE_PERIOD_TIME (which is hardcoded to be 3600) i.e block.timestamp = 1719739032, so 1719739032 - 0 = 1719739032 which is bigger than 3600. The code won't revert.

The chainlink docs say that sequencerUptimeFeed can return a 0 value for startedAt if it is called during an "invalid round".

  • startedAt: This timestamp indicates when the sequencer changed status. This timestamp returns 0 if a round is invalid. When the sequencer comes back up after an outage, wait for the GRACE_PERIOD_TIME to pass before accepting answers from the data feed. Subtract startedAt from block.timestamp and revert the request if the result is less than the GRACE_PERIOD_TIME.

Please note that an "invalid round" is described to mean there was a problem updating the sequencer's status, possibly due to network issues or problems with data from oracles, and is shown by a startedAt time of 0 and answer is 0.

Imagine a case where a round starts, at the beginning startedAt is recorded to be 0, and answer, the initial status is set to be 0. Note that docs say that if answer = 0, sequencer is up, if equals to 1, sequencer is down. But in this case here, answer and startedAt can be 0 initially, till after all data is gotten from oracles and update is confirmed then the values are reset to the correct values that show the correct status of the sequencer.

From these explanations and information, it can be seen that startedAt value is a second value that should be used in the check for if a sequencer is down/up or correctly updated. The checks in _validatePrice will allow for sucessfull calls in an invalid round because reverts dont happen if answer == 0 and startedAt == 0 thus defeating the purpose of having a sequencerFeed check to assert the status of the sequencerFeed on L2 i.e if it is up/down/active or if its status is actually confirmed to be either.

Impact

Inadequate checks to confirm the correct status of the sequencerUptimeFeed in KeeperProxy contract will cause _validatePrice to not revert even when the sequencer uptime feed is not updated or is called in an invalid round.

Tools

Manual Review

Recommendations

Add the following check in the _validatePrice function.

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; //! check some issues on zaros
require(isSequencerUp, "sequencer is down");
+ if (startedAt == 0){
+ revert();
+ }
// 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);
}
Updates

Lead Judging Commences

n0kto Lead Judge 5 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.