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

OpenZeppelin avoided paying the bug bounty for disclosing a flaw in the contract that caused a freeze of $1.1B worth of assets #4474

Closed
Dexaran opened this issue Jul 22, 2023 · 14 comments

Comments

@Dexaran
Copy link

Dexaran commented Jul 22, 2023

I would like to escalate this issue: https://twitter.com/Dexaran/status/1682826099800125442

Long story short:

  • I reported an issue with ERC-20 token implementation at OpenZeppelin ERC20 standard known issues. #4451

  • I got a response "we can't fix it" and it is OK. You can't fix it. I agree. But it doesn't mean it is not a flaw. And the fact that you can't fix it does not automatically mean that it does not pose a threat to the safety of users funds.

  • I recommended to add a warning to documentation "Token developers must restrict transfer function or users will lose money" and it is still not done.

  • I submitted a security bug report and I expect to get $25,000 payment because the reported issue fits in "Critical severity bugs" as per your description.

  • Is it a bug that can cause permanent freeze of users funds? - Yes it is.

  • Is it in your code? - Yes it is.

  • Is it in scope and fits in "critical vulnerability criteria" at your bugbounty program? - Yes.

I kindly request an explanation of the following:

  1. This bug bounty page says that "https://github.com/OpenZeppelin/openzeppelin-contracts" are in scope. So is https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC20 in scope then? It looks like it is.

OpenZeppelinBugbounty6

  1. Permanent freeze of funds is considered a critical security vulnerability. I have a proof-of-concept script that demonstrates a permanent freeze of funds amounting in $1,1B worth of tokens. Isn't it true?

OpenZeppelinBugbounty7

  1. Your response states that "it is not a security bug report because it corresponds to the expected behavior to ERC-20 standard". This is not actually true because what I reported is a vulnerability in your particular implementation of the standard. The code is located here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol If we will take this code and compile it "as is" without applying any changes - it will contain a vulnerability that has critical severity according to your own description at the bug bounty page: it can result in permanent freeze of funds for end users. And it already resulted in a freeze of $1,1B worth of tokens in 50 examined contracts out of 1200 existing contracts. Your code contains this flaw. And this code is in scope of the bounty.

  2. "It was discussed MANY times". - Yes it was, but does this automatically mean that it is not a security vulnerability? Does discussing process reduce the severity of a reported problem?

  3. "Devs have the ability to override their implementation in order to add specific restrictions". Yes devs have this ability. But the fact that a critical severity security vulnerability in your code can be fixed by "devs" does not reduce the severity of the vulnerability in your code, right? Again, your code contains this vulnerability. Your code is in scope. This code: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol What is the point of having a bug bounty if you can just reply "It is not a critical severity vulnerability because third party devs can fix it"?

  4. "Since such restriction is not required by the standard, and some implementations rely on such transfer, we should not disable them by default." - Ok, but it doesn't automatically mean that it is not a security vulnerability. And it doesn't mean that this $1,1B worth of tokens that are stuck in the contract are not permanently frozen. I'm not saying that you must disable transfers. I'm saying that I reported a security vulnerability that fits in your bug bounty program and I want $25,000.

Here is a full description: https://dexaran820.medium.com/known-problems-of-erc20-token-standard-e98887b9532c

Here is a Proof-of-Concept script that calculates how much tokens are currently frozen in the examined contracts: https://dexaran.github.io/erc20_losses/

@Dexaran
Copy link
Author

Dexaran commented Jul 22, 2023

The fact that you will not fix a vulnerability in your code for some reasons does not automatically mean it is not a vulnerability if it fits in your classification criteria and POC was provided.

I'm not saying "fix it". I'm saying "pay me the bounty".

@johnpaul-clo
Copy link

@OpenZeppelin Let me know if your Treasury is Empty or you going Bankrupt ? , Follow the standards on your Website and Pay the Hunters

@sgitt-vassky
Copy link

Dex, you will give them nightmares at night with your tokens.
Although in fact what you write really fits their description of a critical vulnerability on bugbounty page

@brandnewx
Copy link

brandnewx commented Jul 23, 2023

OpenZeppelin bug bounty is about finding a security vulnerability that allows somebody else remotely exploit the contract code, not about the inherent issue with the underlying standards (i.e. ERC-20). If you found a problem with a standard like ERC-20, good for you but you're not the first to notice this. It's been documented for years already.

ERC-20 is far from ideal. In fact, it's one of the foolish designs I've ever encountered in my whole software development career.

@Dexaran
Copy link
Author

Dexaran commented Jul 23, 2023

OpenZeppelin bug bounty is about finding a security vulnerability that allows somebody else remotely exploit the contract code

They have a bug bounty page that describes what their bug bounty is about.

Right now the bug bounty page looks like this:

OpenZeppelin_bug_bounty

I see a phrase "The bug bounty is focused on preventing loss of funds by freezing or theft".

If the ERC20.sol contract is outside of the scope - mark it as "not in scope". But it is not marked.

The fact that the issue is known does not mean it is not a vulnerability. The fact that it was discussed does not mean that this stuck tokens are not frozen.

If it remains in wontfix stage - describe it. Update the bug bounty page and transparently describe "wontfix issues are not subject to bug bounty"

Right now I don't see it and what I have reported fits in critical vulnerabilities by their criteria

@Dexaran
Copy link
Author

Dexaran commented Jul 23, 2023

ERC-20 is far from ideal. In fact, it's one of the foolish designs I've ever encountered in my whole software development career.

Yes, it is one of the foolish designs that caused a lot of people to lose their money.

@Dexaran
Copy link
Author

Dexaran commented Jul 23, 2023

If the bug bounty will be paid I will spend the funds on solving this exact issue of ERC-20 tokens.

My projects are known to adhere to the policy of financial transparency:

I will:

  • Finalize ERC-223
  • Finish the implementation of Token Converter, a smart-contract that allows ERC-20 tokens to be upgraded to ERC-223 and back to ERC-20 at any moment: https://github.com/Dexaran/TokenStandardConverter
  • Submit Token Converter as a new EIP
  • Pay the UI developer a bounty to design an implement an interface for Token Converter
  • Write tutorials and templates for ERC-223 tokens
  • Pay content creators for spreading the word about secure practices and caveats of dealing with Ethereum tokens
  • Implement a fully decentralized exchange that operates with tokens without involving any approvals in the process of swapping as approves pose a threat to safety of users funds as well
  • Pay the UI developer to construct an interface for the exchange contract

@ernestognw
Copy link
Member

Hello @Dexaran, thank you for your contribution to the security of the space. We value bug reports and act upon whenever needed.

In this case, I’m afraid the Bug Bounty program prohibits the following:

Public disclosure of an unpatched vulnerability in an embargoed bounty

Mediation is a process held on Immunefi and we engage with whitehats in a constant, clear and well intentioned conversation. However, this makes it ineligible.

We can focus the conversation on what to do to fix the locked ERC20 issue, instead. We’re planning to support EIP-1363 as an alternative to this issue and we’re open for alternatives. (See #3736)

We’ll come back to this next Monday 👍🏻

@Dexaran
Copy link
Author

Dexaran commented Jul 23, 2023

In this case, I’m afraid the Bug Bounty program prohibits the following:

Public disclosure of an unpatched vulnerability in an embargoed bounty

Well, as @Amxx said "the issue was discussed MANY times" so I don't think that disclosing it 19241th time is anyhow violating the rules if it was disclosed 19240 times before and still not patched.

Mediation is a process held on Immunefi and we engage with whitehats in a constant, clear and well intentioned conversation. However, this makes it ineligible.

You have stopped our conversation on Immunefi - and I have no option to re-open it from my side other than escalate it publicly. Given the nature of the issue that was reported (the fact that it was discussed MANY times in particular) - I don't think it was a wrong move in our particular situation since no one can proactively EXPLOIT an issue and public disclosure of the issue can only PREVENT users from exploiting it and dealing damage to themselves.

However, this makes it ineligible.

Technically I can submit a new bounty via Immunefi and this time I will not be publicly disclosing this same issue 19242th time but I doubt it will make it any more eligible than it is now.

We can focus the conversation on what to do to fix the locked ERC20 issue, instead. We’re planning to support EIP-1363 as an alternative to this issue and we’re open for alternatives.

Great. I will read through it

We’ll come back to this next Monday

See you on Monday, have a nice weekend.

@ernestognw
Copy link
Member

ernestognw commented Jul 23, 2023

I’d suggest you scalating the issue with Immunefi. It’s not the first time mediation is needed after closing a report.

https://immunefisupport.zendesk.com/hc/en-us/articles/10644746170897-Report-Closed-for-Known-Issues

If Immunefi happens to favor your submission we’ll be happy to proceed.

We can focus the conversation on what to do to fix the locked ERC20 issue, instead. We’re planning to support EIP-1363 as an alternative to this issue and we’re open for alternatives.

Great. I will read through it

Amazing, let us know your comments.

@Dexaran
Copy link
Author

Dexaran commented Jul 24, 2023

@ernestognw here is a reply regarding EIP-1363 #3736 (comment)

Honestly I appreciate that there is an effort to solve the problem but I don't think ERC-1363 does it.

@frangio
Copy link
Contributor

frangio commented Jul 24, 2023

You publicly posted about this issue in this repository 2 weeks prior (#4451). The Immunefi rules very clearly list "Reporting a bug that has already been publicly disclosed" as disqualifying behavior, and the help article linked above says the same. There is no ambiguity about this.

Regardless, this is not an issue of our implementation but an issue in the ERC-20 standard that is well known, both things you said in #4451. We have considered alternatives to mitigate it and so far we haven't found an option that we feel comfortable implementing. You're free to disagree and prompt us to reconsider, but what you're doing now is not the way to do that.

@frangio frangio closed this as not planned Won't fix, can't repro, duplicate, stale Jul 24, 2023
@Dexaran
Copy link
Author

Dexaran commented Jul 24, 2023

You're free to disagree and prompt us to reconsider, but what you're doing now is not the way to do that.

I would still recommend to update the documentation of your implementation to explicitly state that the ERC20.sol may result in a (well-known) loss of funds for end users especially given the severity of the problem.

As for solution I guess we should move the discussion here: #3736 (comment)

@ernestognw
Copy link
Member

As for solution I guess we should move the discussion here: #3736 (comment)

No, that thread is EIP-1363 specific. Discussing EIP-223 should happen in a different thread, and it's not our priority atm.
We'll reconsider if there's clear demand from the community and a healthy conversation around it.

@OpenZeppelin OpenZeppelin locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants