SJIP: Clarify Oracle Staleness

[WangSecurity]

Description

Clarify that the staleness rule is about any oracle, not only Chainlink, and add an example of the exception.

Judging Guidelines PR

https://github.com/sherlock-protocol/sherlock-v2-docs/pull/50/files

Rationale

Watsons flagged that the rule reads like it is only about Chainlink, while it's supposed to be about any oracle.

Additionally, added an exception case where such issues can be judged beyond the rules:
The protocol may be using Pyth pull-based oracle, which requires requesting the price before using it. Hence, if we don't request the price firstly, or check it for staleness, then we can end up using very old price (e.g. from 1 hour/day ago). Here's the issue that was validated in such a scenario, despite this rule.

Relevant Issue Discussions

sherlock-audit/2024-12-mach-finance-judging#41

[spacegliderrrr]

Instead of making the rule for any oracle and instantly introducing exceptions, consider making the rule for just Chainlink and Chainlink-like oracles (perhaps ones that fit the Chainlink interface?). This would help avoid further confusion and leave less room for judge interpretation on whether they should make an exception or not.

[WangSecurity]

Interesting idea, do you think something like the following would be better:

Chainlink Stale prices and round completeness checks Recommendations to implement round completeness or stale price checks for Chainlink or other oracles similar to Chainlink (push-based) are invalid

Do you think it requires an additional explanation of what is meant by "oracles similar to Chainlink (push-based)"?

[IllIllI000]

Chainlink is coming out with a pull oracle as well. I'd suggest limiting it to oracles where the api being called, or the network it relies on, already enforces a max age in some way. I'd also move the round completeness to a new rule since it's not related.

[WangSecurity]

About limiting to oracles where the API is being called, can you take a look at the above comment and share your opinion? About moving around completeness, do you think it can be in the same rule as these are all price freshness checks?

[IllIllI000]

Round completeness no longer is a thing with OCR so it's a check that does nothing, and the finding is always invalid. Combining round completeness with the other rule makes it seem like they're in the same category of checks that do nothing, when in fact staleness checks are still useful.

[IllIllI000]

Re: the first part of your question, what you have proposed doesn't make any of the findings I flagged below any clearer, so I think it still needs work.

[WangSecurity]

Indeed, the change doesn't cover the issues you've mentioned below, so I've answered about them separately.

One argument that I have for keeping this as one guideline is that separating them makes the guidelines unnecessarily longer. Do you think this is a crucial change, or more of something that would make the rules easier but can be left as is?

Also, this is more of a general question: do you think the suggestion from Deadroses above makes sense and, indeed, would make the guideline easier to understand than the change proposed in the SJIP? As a security researcher, which version of the guideline is better for you?

[IllIllI000]

not a crucial change so can be left as is. It also ends up requiring you to mention Chainlink in the rule, rather than generalizing to push-based or API-with-age-checks-based

If Deadroses' suggestion is to only exclude APIs that follow the Chainlink interface, then it gets messy when there's a pull-based oracle which uses the Chainlink interface.

Your answers to my examples below indicate that you're looking for the existence of heartbeat checks in any form, so I'm not sure why you're against my suggestion:

Stale oracle prices findings where where the oracle- or oracle-calling API, or the infrastructure the oracle it relies on, already enforces a max age in some way, that don't prove that the existing duration is insufficient, are invalid

[IllIllI000]

Can you also take a look at these and comment on whether you think each would be valid or invalid after the change? They're all slightly different variations on the same theme, and would likely lead to an escalation without clarification:

• ak1 - DepositReceipt_Base.sol#L21 : HEARTBEAT_TIME gap is too huge sherlock-audit/2022-11-isomorph-judging#256
• 0x52 - chainlinkAdaptor uses the same heartbeat for both feeds which is highly dangerous sherlock-audit/2023-04-jojo-judging#449
• 0x52 - stETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can cause loss of funds sherlock-audit/2023-03-olympus-judging#2
• eeyore - Incorrect calculation of TWAP in OptionTokenV4.getTimeWeightedAveragePrice() function. sherlock-audit/2024-06-velocimeter-judging#298
• 0x52 - Adversary can sandwich oracle updates to exploit vault sherlock-audit/2023-03-olympus-judging#1

In some of the more recent contests, a lot of watsons are being rewarded for just saying 'stale price' or 'min/max not checked', without actually showing a valid attack, but they should be invalid according to duplication rules. Perhaps the rule should change to clarify that saying that is not enough, and specific examples from on-chain settings must be used, and an actual attack must be outlined.

[WangSecurity]

These are actually very good examples, and my thoughts on heartbeat issues may sound a bit strange, so don't stress to much over it, ask for clarifications and share your opinion:
a. If the issue suggests implementing heartbeat checks (i.e. they're not in the code), then it's more of a recommendation rather than a security issue, and should be low/info.
b. If the check is already in the codebase, but it's too large (e.g. 24-hour heartbeat), then it can be medium. The reason is that price changes can be fairly large and frequent in those 24 hours, and not updating it could be extremely bad for the protocol.

About the issues you've mentioned:

1. Considering the above points, from my perspective, this should be Med.
2. This one is more severe as it can lead to constant reverts of the oracle. Hence, it has even more grounds to be valid.
3. Similar to 1, and I think it should be valid.
4. I think this should be valid even with the guidelines change because it's more a code error to not include the current price, rather than a recommendation to implement price staleness check.
5. In a similar sense to the above, this one is more of a code error that allows to exploit and steal funds from the vault rather than a recommendation to implement staleness price checks.

[WangSecurity]

@IllIllI000 @spacegliderrrr I see that we haven't quite reached a concrete conclusion here, but this seems to be the only such discussion (or one of the few) among other SJIPs, so I'm moving it to the Last Call phase. please share your opinion on the above suggestion.

Re-iterating, there are three paths I can see here:

1. We leave everything as initially suggested.
2. We make the guideline only for Chainlink-like oracles, like here.
3. We make the guideline only for Chainlink-like oracles + separate it into 2 different guidelines, one about round completeness and one about stale price checks.

What is better for you and you think would be better for other SRs?

[IllIllI000]

4. We change to this suggestion

I vote 4