SJIP 7: Duplication

[Evert0x]

Description

Change duplication rules

Judging Guidelines PR

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

Rationale

The duplication rules are: a) unclear, b) complex, and c) allow for low-quality reports to get rewarded.

The current rules create unnecessary friction in the judging process and significantly limit AI duplication from performing at its best.

For this reason, I propose new duplication rules.

The primary thought process is: will this issue get rewarded if the main issue isn't submitted?

Relevant Issue Discussions

sherlock-audit/2024-02-perpetual-judging#127

[IllIllI000]

This finding appears to have found the root cause and Medium impact, but not the right attack path. By the new rules, would it now count as valid? Clarification of the rule either way is needed. Also, if only one of the answers to the two questions is Yes, then does that automatically mean Low/Invalid?

edit by admin: This discussion been addressed in the following commit: sherlock-protocol/sherlock-v2-docs@8213a3d

[WangSecurity]

@Evert0x and change Identify a valid attack path to Identify a valid attack path or vulnerability path?

[IllIllI000]

We can keep 3 requirements instead of 4. (remove medium, add valid as standalone issue). If it's valid as a standalone issue, it means that the issue is at least Medium.

1. Considered valid as a standalone issue
2. Identify the root cause
3. Identify a valid attack path

Wouldn't a valid standalone issue identify the root cause an a valid attack path? The advantage of using Medium impact, is that Impact is a section of the submission template, whereas standalone issue is a vague term. What about:

1. Identify the root cause
2. Identify at least a Medium impact
3. Identify a valid attack path or vulnerability path
4. Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one)

[WangSecurity]

We want to clarify what the "attack path", "vulnerability path" and "root cause" are. I propose these to be added into Glossary:

- Attack Path - sequence of steps or actions taken by a malicious actor to cause losses to the protocol/users and/or gain value/profit.
- Vulnerability Path - sequence of steps showcasing how a vulnerability within the codebase leads to the loss for the users and/or the protocol.
- Root Cause - the specific primary factor or a fundamental reason that leads to a vulnerability and/or can be exploited by an attacker

Please tell me what you find vague about them, and as for vulnerability path, maybe it's better to change to "...how a root cause within the codebase..."

[IllIllI000]

vuln and attack paths look the same in what you have, and you don't mention griefs, which is another way. How about

- Root Cause - the specific primary factor or a fundamental reason that leads to a vulnerability and/or can be exploited by an attacker
- Attack Path - a sequence of steps or actions taken by a malicious actor to cause losses/a grief to the protocol/users and/or gain value/profit.
- Vulnerability Path - the specific input arguments provided to a function which result in behavior contrary to the chosen source of truth; the path must show how it is possible for those arguments to be provided by a user (e.g. what inputs to a public function result in these values triggering the unexpected behavior in an internal function many layers down)

[Evert0x]

The attack path and vulnerability path are similar. The only difference is that the vulnerability path can be taken by well-intended (non-malicious, non-technical) users.

Also suggesting to simply the root cause definition and exclude possible outcomes. As broken protocol functionality is excluded from the previous definition for example.

Vulnerability Path: a sequence of steps showcasing how an issue causes losses or grief to the protocol or users through well-intended usage of the protocol. 

Root Cause: the primary factor or a fundamental reason that imposes an unwanted outcome 

[IllIllI000]

Please clarify the definition of front-running

edit by admin: This discussion has been addressed in the following commit: sherlock-protocol/sherlock-v2-docs@05b2e29

[IllIllI000]

first example is commit (reward) reveal (block range of who is included). Second example I'd argue is not a vulnerability

[IllIllI000]

Still my main concern is that there can be two completely different issues that can be exploited with front-run or sandwich and they would be marked as duplicates or one of them is not reported.

That is my concern here too, so I'm trying to see where Evert is going with the distinctions

[WangSecurity]

first example is commit (reward) reveal (block range of who is included). Second example I'd argue is not a vulnerability

Why do you think the second is not a vulnerability. It's similar to the first one, you steal the rewards (in terms of the fee) from LPs who's been providing liquidity for longer time (the same as first, you steal rewards from people who's been in the vault for longer time).

And can you please explain what you mean by "commit-reveal mechanism" so I'm sure I understand you.

first example is commit (reward) reveal

Still it doesn't change the fact that it's a sandwich.

[IllIllI000]

Still it doesn't change the fact that it's a sandwich.

Do you agree with my assertion that sandwich/front-run is unlike the others, and that the others are proposed fixes, not methods of attack? If so, we should be trying to get rid of front-run/sandwitch and replace them with something else. I don't have a clean 'something else' so so far I've been using a combination of a limited definition of front-run, along with a proposed fixed. Maybe you can come up with a better replacement.

And can you please explain what you mean by "commit-reveal mechanism" so I'm sure I understand you.

It's still fuzzy in my mind what it should cover, but essentially it's some sort of admin-initiated delay/pause, followed later by the actual change requested. It could be an admin pausing the whole protocol before blocklisting a specific user, or it could be an actual textbook commit-reveal. I think the admin pause/blocklist could be re-written as a commit-reveal, so that's why I was using that phrase

Why do you think the second is not a vulnerability. It's similar to the first one, you steal the rewards (in terms of the fee) from LPs who's been providing liquidity for longer time (the same as first, you steal rewards from people who's been in the vault for longer time).

Exchanges can have different forms of priority. Most people are familiar with price/time priority, where whoever submits first gets their order filled first. Another way is pro-rata where time is ignored and whoever is present gets filled. Uniswap etc are pro-rata. It's a design decision as to which one to use. I agree that the first is similar to the second, but I think it's less likely for someone to want one-time rewards to be pro-rata without regard to duration. It's something Sherlock will have to decide the bounds of

[Evert0x]

As much as I want this SJIP to create bulletproof duplication rules for all cases. I want to make it clear that it's an improvement (which can be followed by another improvement). I don't want to get lost in discussion about these details that disallow this change to get merged.

The language related to the cases of reentrancy, access control, front-running isn't clear in the current rules. This improvement makes it marginally better. But it definitely improves the language related to the general duplicates rules.

I believe it's best to create a follow up SJIP to change the language to "Front-runs that could be solved by a commit-reveal mechanism".

[IllIllI000]

To cut down on judging questions/escalations: Only when the "potential duplicate" meets all three requirements, the "potential duplicate" will be duplicated with the "target issue". -> Only when the "potential duplicate" meets all three requirements, the "potential duplicate" will be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

[Evert0x]

Addressed here sherlock-protocol/sherlock-v2-docs@2283fe6

[IllIllI000]

Under the new rules, are these two really going to be duplicates? They seem different enough to be separate. I'm afraid that one instance will be submitted, and the other one will not be reported (e.g. because it's harder to write a poc), and therefore something will be missed during the mitigation review. Two more examples, more close to the duplicate line, but if one is missed, will have bad effects.

[IllIllI000]

Read-only reentrancy requires a reentrancy method prior to the view function, which is often through ETH transfer (e.g. from the link: "During the asset transfer back, the WETH was unwrapped to the native token and transferred. This triggered the fallback method in the attacker’s contract."), so it's still not clear what you mean the difference is. Also, with the new rules, I believe the first pair I linked to would be counted as separate bugs, but it's not clear whether the second pair are as well. It looks like you're changing what counts as a duplicate, but without being specific you're just invalidating all of the prior case law, which is going to lead to a lot of confusion. Shouldn't you at least make sure that the rule makes it crystal clear what should be done with the two cases I pointed to, if they were to be submitted again in a new contest?

[WangSecurity]

I believe the second pair can be duplicated as the same logic mistake or conceptual mistake.

As for the first one, without the modification added above by @Evert0x , they would be duplicate as just the root cause of reentrancy.

Hence, I believe it would be good to keep that part:

If the following issues appear in multiple places, even in different contracts. In that case, they are considered to have the same root cause.
1. Issues with the same logic mistake.
   > Example: uint256 is cast to uint128 unsafely.
2. Issues with the same conceptual mistake.
   > Example: different untrusted external admins can steal funds.
3. Issues in the category
   - Slippage protection
   - Reentrancy
   - Access control
   - Front-run / sandwich ( issue A that identifies a front-run and issue B that identifies a sandwich can be duplicated )

But, with slight changes, I think it's better to change "they are considered" to "they can be considered" leaving some space for issues that were pointed out by @IllIllI000.

And add specific clarifications for reentrancy. I'm not sure we need these for slippage, access control or front-running (though front-running can be controversial as well).

[IllIllI000]

Re: the second pair, all of the examples given in the numbered and bulleted lists above are very simple mistakes, except for Reentrancy, which is being very careful to separate out issues where the attack is different. The pair that I pointed to also has very different attack paths (I was the only one that submitted the second attack, so it would have been easy for it to slip through), so it would be good to get confirmation from Evert that "conceptual mistakes" aren't analyzed as closely/rewarded like reentrancy, and I would like to hear more about the distinction between reentrancies he's trying to make.

[Evert0x]

After some discussion internally we agree that this proposed clarification is insufficient to show what the exact consideration is. #7 (reply in thread)

We have some ideas on how to make it as clear as possible, but it would be best discussed in a follow up SJIP (for batch #3).

Planning to change the language to "they may be considered to have the same root cause" to keep the clarity regarding the cases the same as the current guidelines, and continue discussion on another SJIP.

[IllIllI000]

I hope the change includes the old The exception to this would be ..., but will wait to see what the actual language is.

[0xumarkhatab]

HI @Evert0x , i want to add a side note , it would be really helpful to simplify the language to very basic one for international people
for whom even the basic sentences would be hard to grasp and leading to not understanding the rules.

For example , the mention of words like latter , former etc. and mention of answer to both questions is yes or no I mean we can simply say if let's say first question was is there front-running ? second is is admin trusted so the line answer to both questions is yes or no
can be simplified to if there is front-running and admin is trusted , then .... in this way I know there is some sort of words duplication
but it clearly describes what we're trynna say instead of introducing grammatical complexities for user.

A lot of people including me has come across these issues.

A simple chat bot might even help to use a language that a 10 year old can understand while taking care of international words
and english grammar fluency.

[WangSecurity]

It's a good remark, but to understand you advise to completely change the wording to "if there is front-running and admin is trusted , then ..." or you propose that the rule has examples and would look something like this:

The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.

1. Identify the root cause
2. Identify at least a Medium impact
3. Identify a valid attack path

Only when the "potential duplicate" meets all three requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

+ For example, if there is front-running and admin is trusted , then ...

Something like this?

[0xreadyplayer1]

This one is a lot better/clear wording , good suggestion !

[WangSecurity]

You can check that the proposed rules were updated and tell if there are more places that you see confusing.

[WangSecurity]

@0xreadyplayer1 @0xumarkhatab the proposal has been updated since you shared this comment, please look at how it is now and tell me if you find anything hard to read/understand.

[0xreadyplayer1]

Yes Looks Good @WangSecurity , thank you

[IllIllI000]

Just to clarify, does this mean all three need to be different, or is it subsets on a case-by-case basis? The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately. Would change fixes are to fixes are all if you meant the former

[WangSecurity]

Regarding underlying implementation I agree, but about impact, what do you think needs to be defined about it?

[IllIllI000]

Example: uint256 is cast to uint128 unsafely. I don't see how two of these can have the same impact, except in the same function, and causing the function to revert. Same for safeTransferFrom, etc, however those are always grouped together as duplicates

[WangSecurity]

Fair enough, but I'm not sure what you mean, you propose to explain in which cases these issues would be treated separately due to impact, i.e. define how different the impact should be to separate these?

[IllIllI000]

I'm just saying that the way this rule has historically been interpreted is not the same as what is written in the text of the rule. It usually seems to be whatever the judge decides is the cutoff, so I was trying to see if maybe I was misinterpreting the text, but from what you're saying, I believe my interpretation was correct, and it's just how the rule has been applied. I'm fine with it remaining this way until it's revisited later, since it isn't really being changed from what the current rule is, and I don't have a better replacement text.

[WangSecurity]

Hm, then yeah, let's keep it this way and change it in the next SJIP together with other questions. Thank you for pointing it out.