SJIP 12: Duplication Considerations

[WangSecurity]

Description

Clarify what the considerations for duplication are based on

Judging Guidelines PR

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

Rationale

The rules for duplication confuse what consideration for duplicates (reentrancy, slippage protection, front-running) is based on.
Hence, the goal of this SJIP is to clarify these considerations and prevent situations of incorrect duplications.

For reentrancy, I believe it's best to group duplication based on the following categories:

1. Reentrancy in the same function.
2. Cross-function reentrancy (re-entering another function within the same contract).
3. Cross-contract reentrancy (re-entering another contract within the same system).
4. Read-only reentrancy.

Front-running is different, cause there are no types like for reentrancy. The grouping is based on the fix for the issue. The first group can be fixed by slippage protection and it's quite straightforward. The second group can be fixed by a commit-reveal mechanism. This means the attacker (front-runner) won't see the transaction until it executes. For example, in an issue when the admin wants to send rewards to protocol's stakers, the attacker front-runs, stakes and receives the reward, although they're staking only for one block. With the commit-reveal mechanism, the admin's transaction won't be seen until it goes through leaving no opportunity for front-running (note: it doesn't mean it has to be necessarily mitigated with commit-reveal mechanism, and each protocol may have a better fix depending on its implementation).

Another change is about the following line:

The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

It brings confusion as to whether all of these factors have to be different or only one of them. Therefore, I propose the following change:

The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they can be treated separately.

I believe it gives us flexibility for edge cases, e.g. when the root cause is the same, but attack paths lead to different impacts and should be fixed differently.

Relevant Issue Discussions

#7 (comment)
#7 (comment)
#7 (comment)

[midori-fuse]

The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they can be treated separately.

This sounds like oversimplifying it. For example:

• Missing onlyOwner in function A allows anyone to drain accrued protocol fees/other funds (but the contract is technically still usable).
• Missing onlyOwner in function B allows anyone to brick the contract (but no funds are drainable).

Are they different or the same? This might not be limited to just access control or any specific categories.

[IllIllI000]

likewise, for each of the reentrancy types, wouldn't a different type imply a different code implementation?

[WangSecurity]

This sounds like oversimplifying it

Yeah, agree it simplifies the rule, but it will give us flexibility and in some complex case will allow us to make more fair judgement. But in the example you expressed, I agree these should be duplicates on the first glance, but it depends on case-by-case basis.

likewise, for each of the reentrancy types, wouldn't a different type imply a different code implementation?

Yes, the different types would have different code implementations and would be in different families. Maybe it's confused a bit, but the goal is that if there are two types of reentrancies in the protocol, e.g. cross-function and cross-chain, these would be two different families.

[IllIllI000]

I'd add cross-chain reentrancy

[WangSecurity]

Thank you, sounds good! Added.

[IllIllI000]

solodit requires an account, and watsons shouldn't have to have an account at a separate site to understand the rules

[WangSecurity]

Thank you for noticing, changed to link to github.

[IllIllI000]

I believe the second pair can be duplicated as the same logic mistake or conceptual mistake. - they have different attack paths, so does that mean they're separate now? (118 114) (as a clarification, it looks like I mistakenly didn't not remove the second paragraph of the impact of #118 when polishing the submissions, so for the purposes of this discussion, assume that it isn't there)

[WangSecurity]

they have different attack paths, so does that mean they're separate now?

Not necessarily, it's often the case that the same root cause leads to different attack paths. The line "The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they can be treated separately" says it "can be treated separately" so it depends on case-by-case basis and the contest of the issue and the protocol.

I believe first and foremost, we should use the groups that have the same root cause, and only in the edge cases, we can use this line ("The exception to this would be...") if it's appropriate (does this sentence makes sense?).

[IllIllI000]

I'd suggest sticking to rfc terminology everywhere and use may rather than can. I'd also change to:

For the root cause categories above, multiple similar findings that are reported separately must be considered to be distinct findings if they belong to separate groups below.

...

In addition to the rules for the consideration groupings above, if the underlying code implementations OR impact OR the fixes are different, then the Sherlock head of judging may consider similar findings to be distinct.

[WangSecurity]

I've updated the commit, but not how you advised. The first I believe sounds a bit more complicated and I tried to make it simpler. The second, I just changed can to may. Does it sound better?

[IllIllI000]

thanks, that looks fine. Can you change the If several reports...can be considered to be If several reports...should be considered, and possibly change the rest of the relevant can bes in the rest of the file?

[WangSecurity]

Agree on the should be part and changed. I don't see other can bes in the part of the rules we add, so I think changing it in other parts of the rules should be in the other SJIP.

Do you think changing to can be to may be everywhere in the code may have a large impact on the rules and how people understand them?

[IllIllI000]

For a long time I was unsure on the full meaning of the rules since can be means "one is able to", rather than giving a directive, i.e. the judge is not required or suggested to decide one way or another. I'm fine with you fixing them as the various rules are updated over time.