SJIP 5: Hierarchy of Truth

[Evert0x]

Description

Update hierarchy of truth

Judging Guidelines PR

sherlock-protocol/sherlock-v2-docs#21

Rationale

First, the current interpretation of the hierarchy of truth is ambiguous and provides unnecessary friction between Watsons and Judges.

Second, the current hierarchy of truth values code comments as much as protocol documentation. I believe that protocol documentation shouldn't be taken into account as it's contents can change during the audit.

Anything written in the README or CODE COMMENTS can not be changed during the audit and reflect the intention of the protocol team. However, it's possible that the code contains outdated comments. In that case we should apply common sense to the judgment.

Public statements (the current protocol opinion) is likely to change based on the information at hand, that's why it is only able to serve to clarify the language.

Third, it's unclear how to judge invariants defined by the protocol team. I believe we should move into a direction where issues that disrespect invariants defined by the protocol team are judged valid.

Relevant Issue Discussions

sherlock-audit/2024-02-perpetual-judging#65
sherlock-audit/2024-03-vvv-vesting-staking-judging#148

[IllIllI000]

All of the changes look good to me. Two points:

• The restrictions and/or expected functionality example seems to imply that any Invalid will become a Medium, but it would good to be explicit whether or not any restriction violation, regardless of how small the impact is, will become a Medium
• Some of these changes appear to be the method that judges have been following since the beginning of Sherlock (e.g. code comment = known issue). Are these rule changes, or clarifications? If clarifications, I'm assuming they'll apply to existing contests. If changes, then there should probably be an update to the changelog too

[Evert0x]

About the first point.

Issues that break these statements will be assigned Medium severity.

Isn't this explicit enough? How would you phrase it?

[Evert0x]

About the change log. I plan on generating the change-log based on the updates that get included in the batch

[IllIllI000]

Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity

[utkuerkin]

I believe documentation can be very helpful in understanding the intended use of the protocol, a solution to the issue of documentation being updated during an audit can be solved by taking a snapshot of the docs at the start of the audit.

Comments however aren't necessarily written in a way to explain the exact intended use of the protocol, they can be helpful on understanding the lines of code they are attached to and still that does not mean It was written for auditors to understand the protocol. While documents are written for everyone to understand the protocol and get an idea on the intended use.

I believe both documentations and comments should be taken into account, auditors cannot rely solely on comments when a lot of protocols lack proper comments in their code. With the lack of comments or the ambiguity of them, getting the proper context about the protocols intended use will be problematic just from the comments.

So in my opinion the current hierarchy of truth works fine but a good improvement could be the addition of a snapshot of the documents from the start of the auditing contest and treating it as the truth.

In that case we should apply common sense to the judgment.

Relying on common sense in the case of outdated comments will create more problems. Common sense will not be common sense for everyone.

Public statements (the current protocol opinion) should not be treated as the truth and shouldn't be used to validate or invalidate findings, it should only be there to provide context for auditors.

I believe we should move into a direction where issues that disrespect invariants defined by the protocol team are judged valid.

Fully agree with this part.

[IllIllI000]

Agree that there are often whole sets of findings that I regularly ignore, given that the expected behavior/usage is described in docs. A difficulty I see is that in some recent contests, sponsors have used notion docs that link to other notion pages, and aren't searchable. It may be difficult to get sponsors to reliably snapshot them all.

[utkuerkin]

It may be difficult to snapshot them all.

I agree, however sponsors should be pushed by Sherlock to provide proper and easy to access documentation, not only it makes auditors job easier it will also provide the sponsors a better security review. It will lead to an easier judging process, result in final report being published quicker and save the judge, auditors and sponsors considerable amount of time that can be wasted during escalations with ambigious and hard to access documentation.
In an ideal contest, Sherlock will encourage sponsors to provide solid, finalized(for the context of the contest) documentation that will be included in the README and be treated as the absolute truth when it comes to judging. However I do not know how realistic this ideal situation is to achieve in all contests.

[Evert0x]

Agree that common sense creates some friction. But the alternative is being too literal about each comment which will result in non-issues being rewarded.

About snapshotting the protocol documentation, this would be a good improvement. However, it's technically challenging is protocols use different tools for the docs (website/github/figma/..). We will look into it for a future iteration on this rule

[utkuerkin]

Agree that common sense creates some friction. But the alternative is being too literal about each comment which will result in non-issues being rewarded.

Agreed, being too literal about comments is a problem but common sense will lead to more unnecessary discussion. I don't know what would be a good solid way to put it in writing so It is clear for everyone what exactly the rules are. Because if someone follows the comments(on an issue where it is not clearified in docs) and submits a vulnerability, when it gets invalidated because of judges common sense it will feel very unfair for the watson. This could be solved with escalations of course and other watsons giving their opinions too but again this creates more discussion that will increase the time that is being wasted arguing.

[Evert0x]

A user is always able to ask for a public statement from the protocol team as supporting clarification. In case the protocol team confirms the comment is valid, it will hold in the judging.

[santipu03]

All the changes look good to me but I'd like to ask for clarification regarding this part of the updated guidelines:

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

My question is, what is the limit that can define a known issue? The acknowledgment of the root cause or the acknowledgment of the impact it may provoke?

I'll go through two examples to explain this:

Example 1:

Let's imagine a protocol that has a mechanism that, when used in a specific scenario, it accidentally sends all user's funds to the address zero. The code comments explicitly state that the use of this mechanism may have some risk, but they don't acknowledge that the risk may be losing all the user's funds. Would that issue be considered a KNOWN ISSUE or not?

In other words, an issue can be considered a KNOWN ISSUE just because the comments confirm that the mechanism is acknowledged? Or do the comments need to acknowledge the full risk that the mechanism may cause if used?

Example 2:

We have a protocol that in the README it's stated the following in the known issues section:

"When [X] is empty, users may experience temporary illiquidity when withdrawing funds"

Now, if an issue is discovered that when [X] is empty, all users will lose ALL their funds, would that issue be considered a KNOWN ISSUE? The README is acknowledging a possible risk (low) that the root cause ([X] is empty) may provoke, but it's not acknowledging that the same root cause can provoke all users to lose all their funds (critical risk).

Would these issues be considered valid or not?

I think it may be helpful to clarify these situations in the guidelines to avoid unnecessary discussions between judges and Watsons.

[IllIllI000]

Sure, my point was case-by-case seemed to me to be the current approach where it had to be unexpected and an escalation usually had to verify, but the way youre saying I believe is to require listing every known scenario

[santipu03]

With the current approach, what is the defining factor to consider some impact unexpected or not? Because right now it seems that we take the word of the sponsor for it AFTER the contest has ended, which I think is unfair.

I know maybe listing every known scenario could be somewhat tiring for the sponsors, but maybe a clarification in the Sherlock docs could help in order to avoid unnecessary escalations in the future. That's all I want, avoid unnecessary arguments because of some unclear or generic rules.

[Evert0x]

Good points. I believe this could be an improvement to the QA. Where we expect the protocol to write down KNOWN RISKS for each KNOWN ISSUE.

I'm not sure if this requires an update to the guidelines. But if so, what would you change? @santipu03

[santipu03]

Maybe something along these lines:

An issue will be considered invalid if its ROOT CAUSE and RISKS have been acknowledged by the protocol.
On the other hand, an issue will be considered valid if its ROOT CAUSE is acknowledged but its RISKS were unknown by the protocol at the start of the audit.

[Evert0x]

I think we should include this change in another SJIP. It doesn't apply to the hierarchy of truth but about an interpretation of this QA piece.

I think we should handle it in a similar way as we handle FUTURE ISSUES

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

By default we don't accept any BUT the protocol is able to use the README to override this. Similarly, by default we should not accept any risks coming from a known issue, but allow the protocol team to override that.

[foufrix]

However, it's possible that the code contains outdated comments. In that case, we should apply common sense to the judgment.

Why add this? If this is written in the repo, it's how it should behave. This phrase opens the gate to discuss anything. The purpose of the audit is to check if the code works as intended by the protocol team at time X. If it hasn't been updated, it should not fall on Watsons.

[Evert0x]

It's common for the protocol teams to leave outdated comments on the code. Agree that common sense creates some friction. But the alternative is being too literal about each comment which will result in non-issues being rewarded.

[IllIllI000]

The protocol team can only use public statements to clarify language in the chosen source of truth.

• Public statements will not change the interpretation of the chosen source of truth.

If the clarifications can't change the interpretation of the source of truth, what is being clarified and what are watsons supposed to use the clarification for? For the 'chosen source of truth', is the sponsor choosing, or is the judge choosing which source it applies to based on the hierarchy?
I think this section would benefit from some examples.

[IllIllI000]

Based on your new example, does that mean the sponsor is allowed to restrict the scope of any statement, as long as the statement didn't originally have any explicit restrictions that are now being contradicted? That seems like it'll lead to a lot more ambiguity, so I'm skeptical of that approach. If so, I'd make that clear in the docs. Also, does that mean public statements will be allowed at any time (e.g. after the contest judging has completed, but still during escalation resolution)?

[IllIllI000]

Specifically, I'm thinking of this case where, with your new approach, the sponsor would be free to arbitrarily reject any integration issue that they wish to, with watsons having no way to know what to submit ahead of time. I believe, though I did not follow it at all, a similar thing may have happened with the recent OP contest.

[Evert0x]

First, the language in the README needs be correct, that wasn't the case in the case you linked. To be specific, without context of the code the answer in the QA should be clear and answer the question completely.

Alternative idea is to remove the possibility for the protocol team to use clarifying statements. Only the Sherlock team is able to clarify/change the README up until 24 hours before the audit ends. This might be better as I agree that judging if a statement is clarifying or not opens another can of worms.

[IllIllI000]

Agree that the alternative idea is better

[Evert0x]

Edited. Will move to "last call" status soon