Severity standardization - Lack of two-step ownership transfers / lack of zero address check

[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:
NounsDAO - accepted as M finding
VTVL - reduced to QA

I believe the right call is QA for lack of any / both of two-step / zero address checks. They require explicit user negligence. Also, they are very easy to spot so they will definitely end up being mentioned in QA reports, we don't need them spammed in M/H discussions.

@GalloDaSballo

EDIT: The NounsDAO was a poor example, please refer to this example:
Vader contest - lack of 2 step change of DAO's address, accepted as M

[GalloDaSballo]

Disagree with the take that the finding for NounsDAO is about 2 step transfer

I have been one of the harshest critics of 2 step transfer (always marked as NC and disputed as sponsor consistently)

However, the finding for NounsDAO is a 51% risk if Vetoer is set to 0.

Recommend finding another counter-example as I also have set the 51% attack as Med, because of the 51% risk and not because of the two-step transfer.

TL;DR I agree with the Judge judging and think it was appropriate.
Would ask for a different 2-step Med, those are not logically equivalent imo

[trust1995]

Revised the issue with a better example of "lack of two-step " submission.

[GalloDaSballo]

I think the overwhelming majority of Judges and Wardens would agree with me that the Vader Finding was over-estimated and that it should have been rated with QA severity, I believe Non-Critical to be the most appropriate because the logic for Low cuts both ways, precisely because you could 2 step delegate to a malicious attacker and they could still instantly rug anyway.

I think most Judges have a few reports where they over-estimate severity (I think I did this mistake multiple times myself), but I would expect even @dmvt to agree with me that a lack of 2 step is not a Med Severity finding

I think this finding has been recently always rated as NC, however other people can argue as to why it should have a different severity

[tibbarytsur]

I'm kind of new here so I certainly won't assume I know any better but I think there are multiple ways of looking at it.

For one I think we can say that 2-step transfer is an easy fix (at least in most cases) for which there are no real reasons not to do it.
OpenZeppelin now even includes a version of it in their contracts.

We're also talking about admin functionality that has been purposefully designed into the system, which means there is a clear need for it. Otherwise you can just leave it out.
Loss of that admin functionality then means it hurts the working of the contract in some way, which if I'm not mistaken already puts in Medium level territory according to the rules. You can even think of admin capability equal to access to/loss of funds, which would mean High level.

On the other hand as the admin functionality is designed into the system it is also the projects prerogative to handle it as they see fit.
However this design choice should be clearly stated/documented, otherwise it could be a simple oversight or in some cases ignorance.
It's not because of the fact this is a well known thing in the auditor community (and probably the developer community as well) that lack of this pattern should be treated lightly.

There are several other well-known (debatable) patterns that have the same problem:

• current best practice for sending ETH
• 0 address check
  ...

The risk is that all of these end up being treated as "everyone should know this" and we're not going to report it anymore because it has near zero payout.
Therefor I think the best way to handle this is that sponsors should be given a checklist of items which if not documented warrant a medium or high finding (just as wardens are now given a list of automated findings that are automatically out of scope)
And if it's easily detectable, just put it in the automated discovery list because that's what the automated tool output pretty much is.

However everyone would be better served if this was done long before the contest starts and those point are already addressed/fixed in the code.

This is also a discussion about the 2-step pattern: OpenZeppelin/openzeppelin-contracts#1488

[PierrickGT]

I think both can be part of the QA report since a sponsor may not have considered the possibility of an error when they deploy or change ownership of a contract.
I remember that during a PoolTogether audit, it was recommended by a warden to opt for the two steps ownership pattern and since then we use it across our whole code base.
So I think it can still be a valuable information but it should definitely not be rated as Medium.

[dmvt]

I think most Judges have a few reports where they over-estimate severity (I think I did this mistake multiple times myself), but I would expect even @dmvt to agree with me that a lack of 2 step is not a Med Severity finding

I think this finding has been recently always rated as NC, however other people can argue as to why it should have a different severity

I always downgrade lack of two-step to QA now. The Vader contest ruling was over a year and a half ago before the judges reached a consensus on this issue.