Submission Details
Severity:
Insufficient validation to confirm the correct status of the sequencer and aggregator
## Summary
Not validating correctly the status of the sequencer to prevent pulling data during an "invalid round"
## Vulnerability Details
The ChainlinkUtil library has sequencerUptimeFeed checks in place to assert if the sequencer on an L2 is running but these checks are not implemented correctly. The [chainlink docs](https://docs.chain.link/data-feeds/l2-sequencer-feeds) say that sequencerUptimeFeed can return a 0 value for startedAt if it is called during an "invalid round".
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. Further explanation can be seen as given by an official chainlink engineer as seen here in the chainlink public discord
https://discord.com/channels/592041321326182401/605768708266131456/1213847312141525002
This makes the implemented check below in the [`ChainlinkUtil.getPrice() function`](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/external/chainlink/ChainlinkUtil.sol#L26-L76) to be useless if its called in an invalid round.
```
function getPrice(
IAggregatorV3 priceFeed,
uint32 priceFeedHeartbeatSeconds,
IAggregatorV3 sequencerUptimeFeed
)
...
{
...
if (address(sequencerUptimeFeed) != address(0)) {
try sequencerUptimeFeed.latestRoundData() returns (
uint80, int256 answer, uint256 startedAt, uint256, uint80
) {
...
uint256 timeSinceUp = block.timestamp - startedAt;
if (timeSinceUp <= Constants.SEQUENCER_GRACE_PERIOD_TIME) {
revert Errors.GracePeriodNotOver();
}
} catch {
revert Errors.InvalidSequencerUptimeFeedReturn();
}
}
...
}
```
As startedAt will be 0, the arithmetic operation block.timestamp - startedAt will result in a value greater than Constants.SEQUENCER_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.
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 ChainlinkUtil.getPrice() 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.
The same problem exists with the price pulled from the aggregator, the timestamp is not verified to be != 0, this would cause the oracle to work with stale/incorrect data.
## Impact
Insufficient validation to confirm the correct status of the sequencer will cause getPrice() to not revert even when the sequencer uptime feed is not updated or is called in an invalid round.
## Tools Used
Manual Audit, Chhainlink Docs & [PR to Chainlink's Codebase](https://github.com/smartcontractkit/documentation/pull/1995)
## Recommendations
Aadd a check that reverts if startedAt is returned as 0. This same check can be added to the data pulled from the Agreggator too, to prevent the price being pulled from an incomplete round.
```
function getPrice(
IAggregatorV3 priceFeed,
uint32 priceFeedHeartbeatSeconds,
IAggregatorV3 sequencerUptimeFeed
)
...
{
...
if (address(sequencerUptimeFeed) != address(0)) {
try sequencerUptimeFeed.latestRoundData() returns (
uint80, int256 answer, uint256 startedAt, uint256, uint80
) {
...
+ if (startedAt == 0) revert Error.StartedAtIsZero();
uint256 timeSinceUp = block.timestamp - startedAt;
if (timeSinceUp <= Constants.SEQUENCER_GRACE_PERIOD_TIME) {
revert Errors.GracePeriodNotOver();
}
} catch {
revert Errors.InvalidSequencerUptimeFeedReturn();
}
try priceFeed.latestRoundData() returns (uint80, int256 answer, uint256, uint256 updatedAt, uint80) {
+ if (updatedAt == 0) revert Error.StartedAtIsZero();
if (block.timestamp - updatedAt > priceFeedHeartbeatSeconds) {
revert Errors.OraclePriceFeedHeartbeat(address(priceFeed));
}
...
} catch {
revert Errors.InvalidOracleReturn();
}
}
...
}
```
Updates
Lead Judging Commences
inallhonesty
over 1 year ago
inallhonesty over 1 year ago
Submission Judgement Published Assigned finding tags:
Inadequate implementation of sequencer check
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821
USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.