code-423n4 / org

Code4rena Governance and Discussion
71 stars 17 forks source link

RFC - A case for or against Admin Privilege Reports #59

Closed GalloDaSballo closed 9 months ago

GalloDaSballo commented 1 year ago

This discussion is aimed at exploring which Admin Privilege findings we should continue to mark as valid, vs consider "out of scope", "known risks", etc...

The discussion requires taking both sides as there seems to be no ideal, simple answer, Malicious Sponsor must be called out for their malpractice, however, Honest Sponsor don't like being told what they already know.

Context:

With end users here I mean "retail", "non technical people", people that rely on an expert for their security.

While I would recommend any person even remotely involved with Ethereum's tech to learn the basics, we cannot assume that and we know that the majority of users are "blind signers"

That said, we have historical track-record of our Reports being used to "prove" the safety of a Sponsor system.

Ultimately this is natural as C4 being 50% Crowdsourced Security and 50% being proper Formal Audit (thanks to some legends in the community).

This means that our Reports are used as marketing tool, in the right hands they are just proof of thoroughness (Project X did Audit1, Audit2 and even did C4 to iron itself out)

In the wrong hands, they are a tool to get a quick list of issues, dispute them all and put the logo of C4 on their website.

Historical Precedents

Since day 1, we've had some findings over time we ended up calling "Admin Privilege".

Admin Privilege findings are specific attacks that a Privileged Role can apply to the system in scope.

Because of their nature, they are controversial (if not outright insulting to the Sponsor), however most sponsors (including myself) recognize them as valid as in they are technically risks that the end user has to accept if using the system.

Also, while extremely easy to downplay, our space has seen these "basic mistakes" happen countless times:

And ultimately because of the anon-friendly nature of the space, it's impossible to discern between an Incompetent Project (Leaks the key because of bad Opsec), or a Malicious Project (steals the funds and claims the keys were compromised).

There is ultimately no way of knowing.

The solution to the above is not a "more trust", or "more due diligence" type deal, as again we are not in a position to know if the Admin did it on purpose.

The only solution to the above is to always flag those reports as valid risks that end-users face if interacting with the contract.

This has been consistently practiced for a long time at CodeArena, and perhaps due to market demands is not being put to question.

The discussions goal is to determine whether it's correct to flag Admin Privilege type findings, and what is the limit of absurdity as well as how these types of reports can be gamed.

While there have been oscillation in judging, I think a very strong argument can be made for Admin Privilege type findings to always fall in a Medium Severity.

See gist for 2021: https://gist.github.com/GalloDaSballo/881e7a45ac14481519fb88f34fdb8837

First Principles Questions, First Principle Conclusions

If we agree on the above, then the next logical question is what C4 is, or what we want it to be.

Do we want C4 to be a technical only type firm? Or do we want to keep the user facing nature of the Project?

If we keep it public, knowing the above, we have to agree that Admin Privilege has to be flagged.

Not flagging Admin Privilege allows malicious projects to create fake projects and just rug via Admin Privilege, while using C4 as a quick "We were audited bro" scapegoat tool.

Risk of Abuse, where do we draw the line?

A few specific instances are linked below, with the goal of sparking a discussion that can help us figure out how to best deal with these type of reports in the future:

[REDACTED CURRENT CONTEST FINDING]

Zora Nouns Builder, Vetoer, lack of Vetoer and Bribing

These three reports show the circular logic that we may have whenever we deal with Governance System as well as Admin Privilege.

I have judged these, and you should read the comments in depth to understand my point of view.

Anyhow, I don't mind disputing these as lower severity (e.g. Low), which I think can be argued for or against.

What I'd like to show is that if you pick any of the three as Med or Low, then you are logically bound to rate the others with the same severity.

That's because these three findings (to me), are different perspectives on the same issue:

Do we need to call this another trilemma? I think we can do without.

But I think you'll be hard pressed to argue that a Vetoer is a totally good thing, it's not, it's a compromise, same for not having a vetoer as well as any other governance attack which has been exploited in the past

Request for Feedback

Do you agree with me that those 3 findings are logically equivalent? (Vetoer is trusted party, opens up risk, lack of vetoer is risk, bribing is a specific attack when lacking a vetoer)

Do you agree with me that because of the historical context, and the rules we have, given the logical equivalency, those 3 findings are all Med Severity?

Given the two above, do you agree with me that this situation shows "a glitch in the matrix" as due to logical equivalence, circular logic is made valid as it would otherwise create favourites?

What do you think we should do?

1) Acknowledge and continue consistently with the convention

I'm thinking that explicitly acknowledging the contraddiction, out of an interest to transparency to end-users is probably the best first step, meaning we will keep flagging these reports, fully knowing that sponsor will disagree in the majority of the cases.

2) Acknowledge and create a new category, Admin Privilege

This is equivalent to the above and creates more work for the Judges, but should reduce attrition with the sponsor, Sponsor feedback should be sought to better understand this option

3) Find the line of absurdity

Let's further discuss about the line of Admin Privilege vs logical absurdity through nuanced, evidence based discussion.

trust1995 commented 1 year ago

A very interesting discussion indeed. The way C4 wants to be looked at may shape the way we handle different types of findings, and especially centralization vectors.

I have a problem with awarding M severity for superficial, trivial threats as it degrades other, original attack vectors of M severity. We already have an issue with the fact not all mediums/highs are equal, yet they are paid as such. All reports of type "Governor can set approve / can update contract" and so on provide little value to all parties involved. The current status quo IMO is pretty terrible in that regard, as wardens are incentivized to submit nonsense findings and get awarded for them despite the issues considered known.

Centralization reports do have the potential to be M worthy reports IMO, in the sense that owner has permission to do X, but by some previously unknown method gets to do Y, where Y is much more potent than X, i.e. privilege escalation. A good example is in a recent contest, where admin can revoke user's staked tokens, after the staking period. Since this is not something users would see in advance, and it impacts their assets by right, it does warrant a major severity level.

I can understand that it is important for the org, as a reputable audit publisher, to state centralization issues as you have explained well several scenarios where they are relevant. I think the best option is to add "Centralization report" similar to QA / gas reports. They shall be awarded on a scale, for 3% of the pot, to be taken from the QA report ( feel free to crucify me for this).

Another option to consider is merging centralization issues into the QA/Low report, which is already capped at 10% and provides a lot of value for sponsors.

IllIllI000 commented 1 year ago

I think if you were to do a search for recent submissions of centralization, my handle would be on most of them. That being said, I think it's important to distinguish ease of finding vs the severity of the finding. There are already many 'weird' ERC20 mediums where 30 wardens submit the same finding, but there's no push for those to be marked as low. With the previous (now removed) OWASP methodology severe consequences+unlikely = medium, and one out of thirty being a rug seems to fit that bill. I've not heard arguments for why it should be low, in terms of risk, so if anyone has some good explanations in that area, that would be helpful to hear.

trust1995 commented 1 year ago

I would like to argue that filing centralization findings on trivial issue like "owner can rug the funds" is exactly the same as reporting other known issues. If known-issues are out of scope in contests, why should centralization known issues not be? There are known issues that could easily qualify as M risk, and by the logic of "C4 has a reputation to defend" we should include those as well. I argue for injustice in the way different known issues are awarded.

GalloDaSballo commented 1 year ago

I argue for injustice in the way different known issues are awarded.

That's basically the quote, you feel it's a cheap way to rack points, and due to payout, these reports end up mattering.

Question is, how do you make it work long-term?

trust1995 commented 1 year ago

I think it's not difficult to differentiate. Findings that show owner is able to do some dangerous action that by surface-level analysis it seems they cannot do, are eligible as privilege escalation bugs. Submissions that state trivial things like owner can use backdoor transfer function, etc. should be part of a centralization report.

For example, I reported that an operator in Tokemak can use the addLiquidity() function to exfiltrate the entire funds. Since operator only needs the ADD_LIQUIDITY role, it is a valid privilege escalation. Another example is what I stated in previous comment, where staked tokens can be revoked retroactively.

GalloDaSballo commented 1 year ago

It's not difficult as long as you don't explain where the line is, if you want to put it into rules, you have to be extremely specific about what fits and what doesn't.

Would recommend you list a few findings and argue about them being "trivial" vs "non-surface level."

Also, keep in mind that trivial is subjective so you'd need to define that as well (or find a definition we can use).

I will check the report you linked, but as you saw, it was dismissed as Admin Privilege per Immunefi's new rules, meaning that arguably their team dismissed it as trivial as well (and the devil is in the details)

trust1995 commented 1 year ago

I can define trivial - privileges that are documented in the published repository or developer / general documentation.

Immunefi do not accept any submissions that are related to permissioned addresses, even if it's a privilege escalation. It is a harsher stance than I have, and a much harsher stance than yours (can be understandable with the risk management their projects employ).

I'll have time later to dive into several examples, including my opinion of ArtGobblers.

Chomtana commented 1 year ago

If a Sponsor can change a setting maliciously, how do you rate it?

If timelock or governance system is implemented then QA. If timelock and governance system is not implemented then Medium. If it didn't specified then go for the worst case.

You should ask the sponsor first whether they use Timelock or any Governance module or just an EOA wallet.

If a Sponsor can change the setting, but only a set of specific ranges is dangerous, is it still admin privilege?

If that specific range can drain all funds immediately then Medium. But if it is too slow it can be QA.

What other instances should be escalated? What's the differentiating factory between cheap and nuanced?

Has Timelock? Use a Governance module? Use multisig? (Still ruggable if all key holders are friends) Time used to rug? immediately or long or short?

GalloDaSballo commented 1 year ago

If timelock or governance system is implemented then QA. If timelock and governance system is not implemented then Medium.

Please note that saying you'll use a timelock is not proof there will be one. Opposite goes as well.

Recommend checking this and countering / arguing there: https://github.com/code-423n4/org/issues/7

sockdrawermoney commented 1 year ago

I think specifically creating a separate report category for this alongside QA and Gas reports would be a good idea.

This becomes a useful element for projects and users to refer to directly.

I think at the same time we could also create a fourth report category for 'codebase analysis and recommendations' because I think those have tremendous value and are kind of lumped into QA.

mlafon commented 1 year ago

Let the sponsor write a clear and detailed paragraph on known admin privileges. Everything that is not clearly described and may put user's assets at risk is a High. In the report, copy the sponsor's description as a limit of the C4 audit.

trust1995 commented 1 year ago

Agree with these sentiments, but we need to be wary of C4 not becoming too much of a centralization report machine, which may happen from incentive mechanics.

carlitox477 commented 1 year ago

Creating a separate category for centralization risks seems a good approach, it will limit the rewards a warden can receive for bad faith of people who are supposed to act in good faith, but it will also maintain C4 reputation if any of this scenarios end up happening.

I think at the same time we could also create a fourth report category for 'codebase analysis and recommendations' because I think those have tremendous value and are kind of lumped into QA.

I also like this approach, I think it might ease judges job too.

sockdrawermoney commented 1 year ago

@trust1995 can you say more about that? Not 100% sure I follow

gititGoro commented 1 year ago

Let the sponsor write a clear and detailed paragraph on known admin privileges. Everything that is not clearly described and may put user's assets at risk is a High. In the report, copy the sponsor's description as a limit of the C4 audit.

I want to second this. In my experience there are two types of contests: one where the sponsor explains governance in docs so that ownable contracts have a certain expectation. Then there are those where the sponsor says nothing. In the latter, strict flagging of high risk is important but perhaps in the latter, the C4 report should simply contain a disclaimer like

"The risk of the findings below are subject to the sponsor's claim that the contracts would all be owned by a standard DAO (fork of CurveDAO). If the final deployed contracts are not transferred to a DAO as detailed in the documentation then the results of this audit should be considered void and C4 withdraws its approval" or some such.

Basically require sponsors explicitly detail governance as a covenant with the end users.

trust1995 commented 1 year ago

@trust1995 can you say more about that? Not 100% sure I follow

I mean that if centralization reports are rewarded as HIGH, we will see a ton of reports, mostly of low technical value, get submitted and probably accepted. It will make the "juicy" reports ( we all know what I mean) have much less significance in terms of % of pot. This is why I advocated for a separate centralization report pool, of capped value (this works well enough with QA/Low).

sockdrawermoney commented 1 year ago

@trust1995 Ok, got it. We are on the same page then. I originally thought your response was to what I had posted.

GalloDaSballo commented 1 year ago

Let's add more work to the sponsor, then punish them with higher severity if they fail to disclose. Disappointed by the one-sidedness of the discussion

IllIllI000 commented 1 year ago

@trust1995 It sounds like you care about capping the payouts for these findings, rather than with the severity designation - is that right? Why not just submit them so that there are duplicates and the payouts become smaller? It seems like if an exception is made here, exceptions will be made elsewhere, leading to more work for judges.

KenzoAgada commented 1 year ago

Why not just submit them so that there are duplicates and the payouts become smaller?

I think because it can become a game of "gotcha" and then when one trivial issue "slips through" with few wardens submitting it, it will get a disproportionate amount of awards. Like the recent Zora "vetoer can veto" issue. This is obviously by design and obvious to everybody, yet only 3 wardens submitted it. Why do we need to take the risk of having such trivial issues submitted and getting rewarded unproportionally? Why do we need to wait until describing every corner of the system design becomes another low hanging fruit and we get 100 submissions stating the obvious, meanwhile taking rewards from the real issues, which are the bread and butter of C4?


I very much agree with trust's sentiments and suggestions in this thread.


As users might read the reports, let there be a "centralization risks" report (like QA) which states all the trivial issues regarding trusted actors. I agree with trust that it might be taken from the QA report pot. Trivial = just describing the system properties and design. For example: vetoer can veto, no timelock for changing fees, fees are not capped, admin can upgrade upgradeable contracts, admin functionality is bricked if admin transfers adminship to wrong address. Non trivial issues are still worthy of their own separate issue perhaps. For example trust's TOKEMAK example which has privilege escalation.


I see no downsides and many advantages to having a separate centralization report. This way:

GalloDaSballo commented 1 year ago

This is obviously by design and obvious to everybody, yet only 3 wardens submitted it

What would you have rated https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/533 https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/479 then?

KenzoAgada commented 1 year ago

What would you have rated code-423n4/2022-09-nouns-builder-findings#533 code-423n4/2022-09-nouns-builder-findings#479 then?

Are you asking this considering the C4 issues historical context, which does rate some of these as mediums? And in which we don't have a dedicated "centralization report" so the "only way" to show the users these issues is to rate them as medium? Or are you asking considering what I think is more accurate/reflecting/fair/should be done?

I pretty much agree with all that you said in the "Request for Feedback" section above (my previous response was already quite long so I didn't mention it 🙂). But going forward or as a judge I wouldn't wanna treat these as mediums.

IMO most of these trusted-actor issues basically just describe general properties of the crypto/governance ecosystems, and do not reflect a novel problem in the design/implementation (which wardens are paid to discover). Because of this, and because of the circular logic, I believe we should change the rules and add a dedicated centralization report.

GalloDaSballo commented 1 year ago

I pretty much agree with all that you said in the "Request for Feedback"

I was asking to understand your perspective "if you were a Judge".

I would like to hear from you and other wardens some more adversarial thinking around the report.

This is me spewing ideas in a few minutes, would like to see more of this as if you want to create a new category of reports, given the environment we're in,it would be naive not to expect the new category to create new opportunities for "easy Meds".

We should also keep in mind the Sponsor's experience, what if some of those ARE gotchas to them?

Should the sponsor feedback influence judging in any way? Or should we have a specific "Sponsor Persona" that we assume to work with? Should we keep it vague to avoid further gotchas?

HickupHH3 commented 1 year ago

+1 on having the sponsor providing a section on trust assumptions / admin privileges, and having a separate report category for it. It will provide much clarity on the kind of issues that should be reported / accepted. While it could be part of QA, creating a separate category would be good for report generation purposes. Using the same grading system as QA / gas should make grading straightforward.

The more explicit the expected actions for trusted roles are, the better.

Eg.

Or it can be as simple as "assume that the owner will act honestly, in the interest of users" or "operators can be trusted to do A, B, C, but should not be able to do X, Y, Z."

Including the sponsor's section in the report will be important, because it specifies the boundaries that wardens worked with.

What happens if the admin can directly change a variable? What happens if it's indirect? What if they can change a variable but you need an external condition (e.g. can rug but you must give allowance first)

These findings should be part of the report.

What happens if the system is a Multisig Like (e.g. Comp governor), is a contest around a timelock by definition just the centralization report?

Precedent here: the badger citadel contest's medium findings were solely privilege findings. For awarding, as Kenzo and Trust suggested, shared with QA.

What happens if a vulnerability happens if the Admin DOES NOT do something? (forgot to water the plants analogy)

Probably classified as part of the admin privilege report, but we'd have to go into specifics.

kirk-baird commented 1 year ago

tl;dr it's worth having centralisation risks, it's not worth having numerous medium issues for this, solution is to downgrade them to QA.

My thoughts on the issue are that we should rate all centralisation risk as QA. These are worth adding to the report for any external user reading the report since there are a lot of rug pulls in this industry. However, the majority of rug pull vectors are inherent in the protocol.

a) Upgrade the contract with a malicious implementation and transfer funds b) Whitelist a malicious contract c) Recover tokens functions d) Modify rates / parameters without a timelock to allow draining tokens (many different ways to achieve this) e) Mint protocol tokens then redeem them (or trade them in DEXs) f) Pausing a contract to lock funds in the protocol

In the past we've accepted methods for stealing tokens from the protocol by the owner as Medium since they result in the loss of funds. Not considered a high as admin priviledges are required.

I'd like to make the case that there are many more methods for centralisation risks. We are starting to see more and more unique centralisation issues / rug pull vectors for each contest. While it's beneficial to note these exist, having many different issues is not as helpful. That is because the client response is almost always these are behind a multisig during the initial stages of the contract to allow rapid upgrading and patching and will be migrated to a time locked DAO in the future.

Furthermore, wardens spending their time finding more and more niche centralisation risks to receive extra payouts for less duplicated awards is not beneficial to the client and so is not aligning the interest of the client with the work of the wardens.

An alternate option is to only include one centralisation risks issue per contest. So duplicate all centralisation risks even if they are not from the same underlying problem.

trust1995 commented 1 year ago

I like your take on it, although we should be wary of including only one cent. issue per contest as that could lead to a lot of unfair dup scenarios. I still thinking having a separate Centralization report alongside QA would be the best way to handle it for wardens, sponsor and judges.

kirk-baird commented 1 year ago

Thanks for the feedback @trust1995 . I wouldn't be against a Centralisation Report. My only consideration on the point is the amount of overhead. User's have to find these bugs, write the reports then they need to be judged and finally reviewed for the report. I'm not sure if think there's enough benefit to the client or external readers to justify the output.

GalloDaSballo commented 1 year ago

Because the idea of a centralization report is more work and prob extra hoops, what if we agree to downgrade the Admin Privilege Reports to QA, and perhaps rate them as either a Low or 1.5* a Low finding?

I think this ensures we are being fair to end-users while avoiding slapping the face of Sponsors and Wardens with Gotcha Reports that ultimately create a sense of unfairness.

This also avoids creating a new category, and obviously, we can always change it in the future, but at least we can try something and see how it goes

Shungy commented 1 year ago

Case for admin privilege reports: https://twitter.com/DefiIgnas/status/1598512462487498752/photo/2 Imagine a protocol audited by c4 gets exploited due to privilege issue, and imagine if the winning QA report did not mention that. It would be bad for our reputation.

I also align with this: https://github.com/code-423n4/org/issues/59#issuecomment-1290854970 And that disclaimer must be included in the final audit report as well. And in that case any privilege issues not disclosed by the sponsor should warrant MED risk.

0xA5DF commented 1 year ago

what if we agree to downgrade the Admin Privilege Reports to QA

I agree that it should be considered QA if it's obvious and by design, I think an exception should be made for cases where it's not that obvious and the sponsor wasn't aware of it (e.g. https://github.com/code-423n4/2022-08-rigor-findings/issues/264 ).

Also, we can separate between the awarding and the final contest report, we can award it as a QA but give it more emphasis in the final report (e.g. - create a separate category for it in the report) - this way we don't waste the HM pool on trivial bugs but on the other hand give the project's users the necessary warning.

dmvt commented 1 year ago

I think admin privilege issues should be highlighted, so QA would probably not work in this case. As we've seen with many recent hacks, admin accounts being compromised is still a big problem. Medium is potentially offensive, but QA is effectively buried. Going through and extracting the unique admin privilege issues from each satisfactory QA report strikes me as a ton of work.

I agree with @trust1995 and @sockdrawermoney here that a separate pool would strike a good middle ground. We'll indeed have more issues to dedup, but they have to be parsed out somewhere, and spreading that work between the wardens and judges makes sense to me.

Picodes commented 1 year ago

On my end, as our goal is also to set the standards for the industry, I believe like @HickupHH3 that we should ask the sponsor to provide a section on trust assumptions / admin privileges. If they don't want to spend time to do it we can always use our #rsvp-certified channel to pay someone to do it for them.

As a DeFi user, who you are trusting is one of the first things you want to know. And as a sponsor, it does not take much time to write down your trust model, and state every role in the system and their privileges.

Then providing there is such report, we can use @trust1995's point of view and only accept "privilege escalation" findings

Picodes commented 1 year ago
  • Perhaps less experienced wardens can practice writing good reports and get a dedicated reward for these kind of issues. In fact it might democratize the QA pool as some might be better at finding general QA issues and some might be good at describing the centralization risks.

Side note, I disagree with this take. At this stage, C4 should really be a professional workspace, and although it's a great learning tool, I don't think we should push "less experienced" wardens to submit findings "to practice." We frequently have 500+ reports per contest, so we should look for quality, not quantity.

kirk-baird commented 1 year ago

On my end, as our goal is also to set the standards for the industry, I believe like @HickupHH3 that we should ask the sponsor to provide a section on trust assumptions / admin privileges. If they don't want to spend time to do it we can always use our #rsvp-certified channel to pay someone to do it for them.

As a DeFi user, who you are trusting is one of the first things you want to know. And as a sponsor, it does not take much time to write down your trust model, and state every role in the system and their privileges.

Then providing there is such report, we can use @trust1995's point of view and only accept "privilege escalation" findings

I think this is a neat solution. It's quite close to how a traditional security review would work. There's an actor/threat model and anyway around this model is a valid attack.

JeffCX commented 1 year ago

Seems like there is still no conclusion about this thread of discussion,

I agree that the sponsor should label if the admin is considered trusted or untrusted in the contest readme

even the sponsor does not do so, the finding such as if admin can toggle function X, user asset is lost / user cannot withdraw is at most QA

or such centralization risk is just more suitable for analysis report

sockdrawermoney commented 1 year ago

The intent is for centralization risks to be covered in analysis reports.

kartoonjoy commented 9 months ago

Per the Autumn 2023 C4 Supreme Court verdicts, the Supreme Court's verdict on this issue is:

This has been addressed through other verdicts. See: Severity Standardization - Centralization risks

Link to verdict: https://docs.google.com/document/d/1Y2wJVt0d2URv8Pptmo7JqNd0DuPk_qF9EPJAj3iSQiE/edit#heading=h.9qbdvml1nftm