Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Try/Catch structure leads to unintuitive and less descriptive reverts

impish

Summary

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/ChainlinkUtil.sol#L41-L56

Within ChainlinkUtil.sol the formatting of the try catch will bubble up to always revert Errors.InvalidSequencerUptimeFeedReturn() as opposed to a more specific revert message.

Vulnerability Details

if (address(sequencerUptimeFeed) != address(0)) {
try sequencerUptimeFeed.latestRoundData() returns (
uint80, int256 answer, uint256 startedAt, uint256, uint80
) {
bool isSequencerUp = answer == 0;
if (!isSequencerUp) {
revert Errors.OracleSequencerUptimeFeedIsDown(address(sequencerUptimeFeed));
}
​
​
uint256 timeSinceUp = block.timestamp - startedAt;
if (timeSinceUp <= Constants.SEQUENCER_GRACE_PERIOD_TIME) {
revert Errors.GracePeriodNotOver();
}
} catch {
revert Errors.InvalidSequencerUptimeFeedReturn();
}

Clearly, reverts within the "try" block (Errors.OracleSequencerUptimeFeedIsDown and Errors.GracePeriodNotOver) will always be caught by the "catch" block before throwing revert Errors.InvalidSequencerUptimeFeedReturn();

Impact

The impact on the protocol is low - functionally it does not matter if we are reverting with a Errors.GracePeriodNotOver(); or Errors.InvalidSequencerUptimeFeedReturn(), however it could impact future debugging and error logging due to a lack of descriptionary error reverts.

Tools Used

Manual Review

Recommendations

Omit the try/catch block in its entirity - what is the point of catching a revert only to throw another revert? Let the original revert bubble up.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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