Severity standardization - Contract unupgradeability

[trust1995]

It would be beneficial for all of us to standardize severities for common issues, as we have seen variance among judges and contests.

Example of two different severities, for the same issue of lack of __gap[50] array for upgradeable contracts:
Rubicon contest - awarded M
Nouns Builder contest - downgraded to L.

Most of the time I've seen this issue judged as L/QA. IMO since it is an easy find and will find its way to the QA reports, we can safely dismiss it as a L as no funds are at risk and there is an option to reinitialize the contract's existing variables, so current state is not at risk of being unupgradeable.

Opinions are welcome.
@GalloDaSballo

[HardlyDifficult]

Agree with low.

Storage gaps are never strictly required, they are a best practice to ease adding new storage in the future. You can always make changes by declaring virtual functions which implement new storage in an append-only way.

[GalloDaSballo]

See my thoughts on the "usefulness of the submission":

---

Quote from: code-423n4/2022-09-nouns-builder-findings#358

After further thinking:

The presence or absence of a Gap is not an additional safety guarantee
It does however mean that no additional storage slots can be added to parent contracts (can still add to leaf contract)
In order for an upgrade to go wrong:

The Sponsor would need to write the new logic, introduce a storage bug, not test for it, and mark it as valid (vote via their own DAO per the README)
The End Users would need to believe the logic to be safe (not test it), vote via governance, approve the contract
The upgrade would need to impact a slot so important that it's value would need to cause loss (that's not a guarantee either)
Ultimately approving a report such as that means saying that all upgrades are unsafe, which is simply not true.

End users should be informed that upgradeability does introduce higher risk, it can in some circumstances also give more power than expected to the developers, however forgetting to add a gap is not a guarantee that the contract will be bricked if upgrade (meaning if you upgrade it get's bricked), it only guarantees that parent library contracts will not be able to add more storage.

As a judge is really frustrating to see tens of submissions which are basically the copy of a submission that got a Med Severity once. The specific reason is because these submission don't look tailored to the contest, they offer basically no new information to people that are read every report, and they lack the nuance you'd expect from wardens that claim to have experience in the field.

Because I don't have a tool to make these invalid (due to poor quality), I'll have to split the diff, I'll downgrade to Low Severity.

I'll concede that in some cases, if the Sponsor wanted to extend the Parent Contracts (for some reason which is beyond me, when it's so much easier, less risky, to just deploy a new contract), they chose to upgrade and introduce breaking changes, and both the developer DAO and the end DAO vote to accept these changes (meaning these people are not testing the code they run, definition of beyond reckless), then yes a storage gap issue could brick the contract, or it could cause some undefined behaviour.

I will be closing reports like these once the new rule drops as I don't believe anyone would genuinely rate these type of blanket statement reports above 50/100

---

Personally I think a Severity between L and NC to be acceptable, based on context and judge feedback.

Specifically:

• Some codebases may have Gaps, and some library may have forgotten -> Low Severity as inconsistent / Valuable Gotcha
• Some codebases do without Gaps (e.g. Storage Abstract Contracts like Compound) - These may simply disagree with the need for Gap

Some developers will dispute the need for gaps, and rightfully so. This is because:

• They inherited the base contracts from a Library (e.g. OpenZeppelin)
• The only code for which they want the upgradeability is the Child Contract (the one they wrote), the upgradeability of the dependencies is a side-effect that is accepted but not useful to most devs
• Because they plan on ever only upgrading the child contract, no Gap is necessary (try yourself and see that newer slots are fine with proxy pattern).

Meaning that a "normal" dev would probably not care about Gaps in their dependencies, they know they will not upgrade the dependency but only the child contract.

However certain projects may write their own library / abstract contracts, those projects will find value in having a lack of Gap flagged.

I believe @Picodes works in prod with Upgradeable contracts, and because we're in a similar niche they can share their opinion on the above based on real usage (and frustration / gratefulness for the Gap reports as a Sponsor)

[Picodes]

To me gaps are really just a best practice that enables the devs to do minimal changes to the code when adding a variable to a child contract. Without gap you could end up refactoring all your code - basically flatten all dependancies - but it should always work in the end. So it should really be NC.

In the end it does not add any safety: if the dev team is not cautious with the storage layout, thing will go wrong anyway. An example of this would be adding a variable to a child contract without modifying the gap size. Side note regarding this, OpenZeppelin now checks this kind of thing in their plugin.

This being said, I'd still appreciate NC gap reports as a sponsor as we use them so any missing gap would be a mistake of the dev team!

[GalloDaSballo]

Just for clarity @Picodes do you recall ever upgrading the parent dependencies during an upgrade, or do you consider dependencies as locked and will only upgrade the child contract?

[Picodes]

I don't recall having done it, the only case would be a vulnerability at the parent level I think

[zzykxx]

Just my 2 cents as a new warden.

If the contract is upgradable every issue in the lines of if in the future the contract gets upgraded, XYZ might happen, should be considered low issues at best. Reason being that stating something about a future upgrade is close to doing an audit on a code that does not exists yet.

In the same line of thinking, findings should not be invalidated by using upgradability as an excuse.

We are auditing the source code we have in front of us, not a possible future source code.