SJIP 9: Change Trusted/Restricted Logic

[Evert0x]

Description

Remove trusted/restricted logic on role level and use specific values for each function variable.

Judging Guidelines PR

sherlock-protocol/sherlock-v2-docs#27

Rationale

It's unclear what TRUSTED and RESTRICTED mean precisely and how they will be considered during the judging.

At Sherlock, we think it's best to take a different approach to clarify and simplify what values can be considered when identifying an attack path.

The SJIP introduces impactful changes to the QA questions and the judging guidelines, shifting the focus to specific parameter values in access-restricted functions rather than broad protocol roles.

This change will provide watsons with the utmost clarity regarding what values they can use to identify a valid attack path.

Sherlock will be more responsible for guiding the protocol well in answering these QA questions.

Relevant Issue Discussions

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

[IllIllI000]

It's possible to answer ""No."" and not put any limitations on possible values. It's not clear what this implies without separately reading the Sherlock rules. Perhaps say It's possible to answer ""No."" and not put any limitations on possible values, meaning watsons will not report findings related to specific values of the parameter

[IllIllI000]

The values in that script will be taken into account as specific values. -> The values in that script for the in-scope mainnet deployments will be taken into account as specific values., but yes

[Evert0x]

It's not only about contracts that are in-scope. If it's an update audit for example the deployment variables for contract out of scope (but in the codebase) could be relevant.

I think the instructions (language) for the protocol team is clear. It's on Sherlock to make sure it doesn't include a testnet deployment script for example.

[IllIllI000]

By in-scope, I meant in-scope chains, not in-scope contracts. Anyway, up to you

[WangSecurity]

Wouldn't it be better like this:

The values in that script, for the chains you deploy to, will be taken into account as specific values

[IllIllI000]

yes, but I was trying to head off the case where the readme lists fewer in-scope deployment chains than the script lists as deployment chains

[IllIllI000]

Exception: if the attack path is based on a value that's currently live on-chain, it will be a valid issue. mainnet only, or testnets too?

[WangSecurity]

I believe only mainnet.

[IllIllI000]

and the same question for pointing to deployment scripts

[Evert0x]

Right. Should be on-chain where the protocol is planning to deploy to. Which is known based on the first QA question.

[Evert0x]

Pushed a change

[WangSecurity]

We've had a discussion about the following phrasing:

When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

From my perspective it would sound more simple if it sounds the following:

For admin's set values, only values for specific variables mentioned in the README can be taken into account when identifying an attack path.

To me it looks simpler, but on the other hand, Evert's phrasing has the following advantage: "access-restricted captures all possible functions with roles. Where as "admin set values" can be interpreted to apply to a single role (the admin)."

So the question is what watsons think about it?

[IllIllI000]

I prefer Everts since I believe it will result in fewer escalations, and less reliance on case law

[IllIllI000]

Are there any limitations on values set by admins (or other roles) in the codebase? -> Are there any limitations on values set by admins (or other roles) in the codebase, including limitations on array lengths?

[WangSecurity]

To clarify, an example of the array you mean is an array of rewards token? So, the Watsons can check if they'll cause any DOS to the users during the reward claiming, for example?

[IllIllI000]

yes, I meant it to mean array lengths of input argument arrays, as well as total expected number of insertions into storage arrays

[Evert0x]

Yeah i'm positive we should include this.

[IllIllI000]

For the 'incorrect call order', the examples are of obvious order violations, but what about ones where it's not so obvious. For example, to prevent front-running of a blocklist change, the admin should pause the features that interact with the blocklist, before applying the specific block of a user, but this may not be immediately obvious to the sponsor. Perhaps you could make it explicit what should be the interpretation for this sort of call order issue, like you do for limitations on values of specific input arguments, so that it's clear to both the sponsor and watsons who will waste time with unnecessary escalations.

[Evert0x]

I believe this discussion is outside the scope of this SJIP. This part of the rules has only been updated to remove this case

Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue.

Happy to discuss this based in another SJIP based on case where this language was problematic