Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: if a bug exists in a base contract of an in-scope contract or a library used by an in-scope contract and will manifest itself while interacting with an in-scope contract, treat the bug as in-scope #136

Open
bart1e opened this issue Dec 22, 2023 · 11 comments

Comments

@bart1e
Copy link

bart1e commented Dec 22, 2023

Note: this case was already mentioned here and the following verdict by the Supreme Court was made:

We would like to clarify the situation where an in-scope contract composes/inherits with an OOS contract, and the root cause exists in the OOS contract. In such cases, the finding is to be treated as OOS, while exceptional scenarios are at the discretion of the judge.

However, I believe that there wasn't enough discussion on this topic and I would like to present arguments for treating such findings as in-scope.

Arguments

I will now shortly enumerate all of my arguments and I will elaborate on each of them in the next section.
The arguments are as follows:

  1. Base contracts (or used libraries) are actually part of code of a contract inheriting (or using) them.
  2. Current situation encourages wardens to not submit valid, sometimes critical findings that they find in base contracts / used libraries.
  3. In order to fully understand the code, Wardens have to go through all base contracts / libraries used anyway.
  4. I argue that this is probably what Sponsors would expect / prefer.
  5. I argue that is probably what most Wardens prefer.

Justification

  1. When contracts are compiled, they will not only contain the code from the last contract from the inheritance graph, but will contain the code of all base contracts (and functions used from the libraries). We cannot say that a contract was audited, when only part of its code was. The reason smart contracts are audited is to ensure they are safe and they are only safe if their code that is deployed on blockchain is, not only some part of it as users may interact with the entire contract, including functions defined in base contracts. Blackhats will not care whether a bug exists in an inheriting contract or base contract.
  2. While according to the Supreme Court's decision, it is still possible to reward Wardens for findings with a root cause in OOS contract, it is only "exceptional", as stated in the decision I quoted in the beginning. It means that Wardens aren't incentivised to submit such findings during contests as they will likely be invalidated. It leads to scenarios when Wardens will not share some of their findings with Sponsors and will wait until they open a Bug Bounty program.
  3. Often it's not even possible to fully understand the code without analysing base contracts (or libraries used), so there is no point in pretending that they aren't a part of the code being audited - they are, and the real question is whether to hide the issues found in them from the Sponsors (which is now incentivised) or risk reporting them during the contest.
  4. Let's assume that there is a bug in an OOS contract X and X' is in-scope and inherits from X. The entire situation described in the previous points is just unfair for the Sponsors as they already booked an audit for X' and it is natural to expect that after all reported issues in X' are fixed, then X' is safe. But it won't, unless they explicitly put all base contracts in-scope. First of all, they may even be unaware that they have to report every single base contract and libraries that X' uses in order for X' to be entirely audited. Secondly, even if they are aware, they may still miss some base contracts or libraries. I think that if we asked Sponsors whether, by including X' in scope, they would like Wardens to search for bugs in X as well, the vast majority would say "Yes" - they are already paying for the audit, so having more valid issues submitted will not make them pay any more money (and it will make the code more secure), but if Wardens purposely don't report valid findings, Sponsors will pay for it, possibly even a lot of money in Bug Bounty program (or if the funds will be stolen using unreported exploit).
  5. Finding vulnerabilities is hard and ingenious findings should be rewarded. Currently they may not be rewarded at all as the decision about it is entirely up to Judge, but depending on who is judging a contest, it may be different. Moreover, Sponsors may behave unfairly confirming that it's OOS, but silently fixing it in their codebase. I think that all Wardens would prefer to know if their valid finding will be rewarded or not, instead of potentially losing their time for reporting it.

Suggestions

As I have stated in the previous section, I believe that treating findings from base contracts (and used libraries) of in-scope contracts that are OOS, as in-scope would benefit both sides (Wardens and Sponsors), so my natural suggestion is to start doing it.

The change that I would suggest, is to treat all issues from base contracts (and libraries used in in-scope contracts), that can manifest while interacting with in-scope contracts, as in-scope by default.

Alternatively, the following question should be added to a questionnaire that normally is in the "Scoping Details" section of Readme of every audit:

Do you want all the vulnerabilities that can manifest when interacting with in-scope contracts to be reported and treated as in-scope (even if their root cause is in an OOS base contract of some in-scope contract or in a library used in an in-scope contract)?

It will make the case clear for everyone:

  • if Sponsors answer "No", they will consciously accept the risk of still having vulnerabilities in in-scope contracts (with a root cause outside)
  • no matter what answer Sponsors give, Wardens will know for sure whether their finding will be treated as in-scope
  • Judges will not have to analyse each case individually since it will always be clear what is and what is not in scope, from the beginning

Adding this additional question to the questionnaire is a very small change and will help to find out what is Sponsors' view on this matter.

@bart1e bart1e changed the title Proposal: if a bug exists in a contract inherited from an in-scope contract or a library used by an in-scope contract and will manifest itself while interacting with an inscope contract, treat the bug as in-scope Proposal: if a bug exists in a base contract of an in-scope contract or a library used by an in-scope contract and will manifest itself while interacting with an inscope contract, treat the bug as in-scope Dec 22, 2023
@bart1e bart1e changed the title Proposal: if a bug exists in a base contract of an in-scope contract or a library used by an in-scope contract and will manifest itself while interacting with an inscope contract, treat the bug as in-scope Proposal: if a bug exists in a base contract of an in-scope contract or a library used by an in-scope contract and will manifest itself while interacting with an in-scope contract, treat the bug as in-scope Dec 22, 2023
@0xA5DF
Copy link

0xA5DF commented Dec 23, 2023

I agree this should be explained to the sponsor during booking/scouting and we should recommend including any library or base contract (unless it has been audited sufficiently).

However, the contest's pool size is based on the total SLOC, so we have to know in advance what contracts are in scope. The sponsor can't just implicitly add more contracts to scope by agreeing to the question you presented.

In order to fully understand the code, Wardens have to go through all base contracts / libraries used anyway.

This might be true for some contracts/libraries, but for many others there can be a big difference between having to just understand what the code is expected to do and actually deep diving and verifying that there are no bugs in the code.

@bart1e
Copy link
Author

bart1e commented Dec 23, 2023

@0xA5DF , first of all, thank you for sharing your point of view! I really appreciate it. If you don't mind, I would like to respond to several points you stated.

However, the contest's pool size is based on the total SLOC, so we have to know in advance what contracts are in scope. The sponsor can't just implicitly add more contracts to scope by agreeing to the question you presented.

The thing is that some Sponsors already do that - sometimes they state in contests' Readme that they will also accept findings from OOS files if they affect in scope contracts (it's even a broader category than only base contracts). Please correct me if I'm wrong, but I think that they don't have to increase contest's pool by providing such statement.

My point here is, that if Sponsors can bring some OOS contracts in scope anyway, just by stating this in contest's Readme and not additionally paying for it, it's just unfair for other Sponsors who aren't aware that it's possible - if they were aware, I think that the vast majority would do it as additional findings will make them more secure for no additional cost.

This might be true for some contracts/libraries, but for many others there can be a big difference between having to just understand what the code is expected to do and actually deep diving and verifying that there are no bugs in the code.

I agree that the case can indeed be different for more complex contracts / libraries, but if an auditor already undrestands what the code does, it's easier to find bugs than if they had to start from scratch. Moreover, it's often the case that base contracts / libraries are forks of existing ones (from OpenZeppelin or Solmate, for example). It means that many auditors already understand them and even if they contain 500 or 1000 SLOC, then, in reality, amount of code to be audited in them is far less, as if a bug is present, it will be present in the code that was written on top of that fork.

@0xA5DF
Copy link

0xA5DF commented Dec 24, 2023

The thing is that some Sponsors already do that - sometimes they state in contests' Readme that they will also accept findings from OOS files if they affect in scope contracts

Can you please point out contests where sponsors did that?
I don't think that they can do that, and I doubt such findings would be considered valid

Moreover, it's often the case that base contracts / libraries are forks of existing ones (from OpenZeppelin or Solmate, for example)

You might be right in some cases, but we can't have a general rule to add additional files to scope without counting those lines in the total SLOC.
The ideal solution imo is for the sponsor to add those files to the scope explicitly.

@bart1e
Copy link
Author

bart1e commented Dec 28, 2023

Can you please point out contests where sponsors did that?

I will give two examples, but I'm doing this only to prove my previous statement, not to complain about Sponsors behaviour (as I wrote earlier, I would actually prefer to have all files that in-scope contracts interact with to be in-scope).

One example is foundation. Quote from Readme:

Out of scope
[OOS contracts enumerated]
Any issues or improvements on how we integrate with the contracts above is in scope.

Another one is Maia:

Our protocol has permissionless factories where anyone can create with for example deposit poison erc20 tokens in ports or create malicious routers. While contracts generated by these are not in scope, if it does affect other contracts or other balances, it is in scope.

The ideal solution imo is for the sponsor to add those files to the scope explicitly.

While it may seem the ideal solution, I don't think it really is. Consider the following example:

  • Protocol P has a bunch of smart contracts, including Y. Y is in scope, but its base contract X is OOS, since it has been audited before and Sponsors assume that it is safe and they don't want to increase total SLOC. However, there is actually a bug in a function f in X that can be exploited by calling Y.f and it has severe consequences for the protocol. It will be considered OOS, because Sponsors incorrectly (but logically, since the X's code was audited before) assumed that X works correctly. Hence Wardens who report the bug, which is certainly valuable for the Sponsors, may not receive any money - some Judges can bring this into scope, but others may keep it OOS.

One way of solving this, that comes to my mind, that will benefit both Sponsors and Wardens, would be to dedicate some % of H/M pot (say, 5% or 10% or any other number) for issues from OOS base contracts of in-scope contracts (and libraries used in in-scope contracts). If no such issue is found, it will be included in the "normal" H/M pot. I think that both Sponsors and Wardens would be satisfied with this solution because:

  • Sponsors will make sure that audited contracts are indeed fully safe, not only part of their bytecode (excluding base contracts)
  • Wardens will be rewarded for their valuable findings affecting in-scope files

But that is just an idea that came to my mind.

@0xA5DF
Copy link

0xA5DF commented Dec 29, 2023

Any issues or improvements on how we integrate with the contracts above is in scope.

Integration is something that happens in the contracts in scope, therefore it makes sense to consider those issues in scope (the root cause is in the contract in scope).
The statement from Maia seems invalid, and I guess that in that case the rules of C4 would override that statement.

Anyways, I think that the rules of C4 should be simple, if the sponsor omits a contract from scope then it should stay OOS. If there's a bug that was discovered in the OOS contract (whether it's related to the contracts in scope or not) then the sponsor should consider awarding the warden besides the contest.

@GalloDaSballo
Copy link

GalloDaSballo commented Jan 5, 2024

Me, @trust1995 and @dmvt have spent quite some time on this

I believe this boils down to how some people think

As SWE, obviously the child contract is part of the scope, that is the definition of what the contract IS, if I'm A, then obviously A is in scope

As Salespeople and as Security Researchers, that is not obvious, when quoting the contest, those lines are not part of the quote, and are not considered.

I have had a chat with staff about ensuring that this is handled when scoping.

If scoping was done in a way that:

  • Guarantees inclusions from libraries as in scope

Then we wouldn't have this issue

Hence, why the SC has made the decision, changing the process avoids having a rule that can be bent.

I believe all 3 from the SC would agree that:

  • We want the rule
  • We want scoping to change as to avoid these bad scenarios

Sent this to staff and would like to hear their thoughts on this

@CloudEllie
Copy link

Sent this to staff and would like to hear their thoughts on this

I am discussing this with our Booking team, but I think @0xA5DF and @GalloDaSballo have summed up the salient counter-points quite well.

In practice, since audits are priced in relation to LOC count, and sponsoring projects are well-positioned to understand the context of their own codebase and how libraries / base contracts interact with it, it's reasonable for sponsors to wish to have discretion over the scope definition rather than for C4 to have a hard and fast rule about including all base contracts and libraries in scope.

I definitely understand @bart1e's concerns and arguments here, and am sympathetic to them, however practically speaking I don't know that we can reasonably insist on including all libraries in the scope for every audit.

As I said, I'm consulting with other staff to check my assumptions here, but my inclination is to continue ahead with the approach outlined in the SC recommendation.

@GalloDaSballo
Copy link

Sent this to staff and would like to hear their thoughts on this

I am discussing this with our Booking team, but I think @0xA5DF and @GalloDaSballo have summed up the salient counter-points quite well.

In practice, since audits are priced in relation to LOC count, and sponsoring projects are well-positioned to understand the context of their own codebase and how libraries / base contracts interact with it, it's reasonable for sponsors to wish to have discretion over the scope definition rather than for C4 to have a hard and fast rule about including all base contracts and libraries in scope.

I definitely understand @bart1e's concerns and arguments here, and am sympathetic to them, however practically speaking I don't know that we can reasonably insist on including all libraries in the scope for every audit.

As I said, I'm consulting with other staff to check my assumptions here, but my inclination is to continue ahead with the approach outlined in the SC recommendation.

I don't believe that's the case today, because this dynamic is not made explicit / clear

The sponsor can without a doubt make these comments, but it seems like as of today this is not explicitly asked

In lack of explicitly asking, you'd expect SWE to tell you to skip the libraries (because they think they are in scope) and business people explicitly add the libraries

We have had an instance of one file being a governor (literally just a call to a constructor) that imports an implementation of the governor, this is an example of the disconnect because at that point, why even have the one line of the constructor in scope?

@0xA5DF
Copy link

0xA5DF commented Jan 8, 2024

I think we should make the general rule that Alex suggested - to not allow imports to be excluded from scope, with exceptions being made at the discretion of the scouts.
An example where this might be justified - sponsor makes an upgrade to the protocol and is using code that has already been audited in the past.
We might also consider OOS libraries as additional SLOCs due to the time it takes wardens to understand the functions being used.

The sponsor would be asked during booking whether they have such contracts that they'd like to exclude and the reason for that, and the scout will review that during the scouting phase.

@GalloDaSballo
Copy link

I think we should make the general rule that Alex suggested - to not allow imports to be excluded from scope, with exceptions being made at the discretion of the scouts. An example where this might be justified - sponsor makes an upgrade to the protocol and is using code that has already been audited in the past. We might also consider OOS libraries as additional SLOCs due to the time it takes wardens to understand the functions being used.

The sponsor would be asked during booking whether they have such contracts that they'd like to exclude and the reason for that, and the scout will review that during the scouting phase.

This is not what I said

I believe the sponsor can mark OSS whatever they want, but the mechanics of how SLOC are calculated has to be explained to them

Any SWE can understand that this process is ignoring inheritance, and can adapt, provided that this is made clear during scoping

@0xA5DF
Copy link

0xA5DF commented Jan 8, 2024

I was referring to an earlier comment you made:

If scoping was done in a way that:
Guarantees inclusions from libraries as in scope
Then we wouldn't have this issue

I agree it should be better communicated to the sponsor, but I think the best way to communicate that to the sponsor is to raise a red flag once the booking team sees such a case, and let the scout review the case and ensure the sponsor is making the right decision and understands the consequences (+ count the OOS lib as additional LOCs when needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants