Skip to content

Instantly share code, notes, and snippets.

@Dexaran
Last active December 16, 2023 22:40
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save Dexaran/9bd90c1885b4818573368ad02b784125 to your computer and use it in GitHub Desktop.
Save Dexaran/9bd90c1885b4818573368ad02b784125 to your computer and use it in GitHub Desktop.

Poloniex exchange was just hacked.

A hacker made this transaction https://etherscan.io/tx/0xc9700e4f072878c4e4066d1c9cd160692468f2d1c4c47795d28635772abc18db

And the tokens got permanently frozen in the contract of GLM! This shouldn't have happened if ERC-20 GLM token would be developed with security practices in mind. But ERC-20 still contains a security flaw that I discloser multiple times (here is a history of the ERC-20 disaster).

You can also find a full history of my fight with Ethereum Foundation over token standards since 2017 here https://dexaran.github.io/erc223/

The problem is described here.

Here is a security statement regarding the ERC-20 standard flaw: https://callisto.network/erc-20-standard-security-department-statement/

As of today, about $90,000,000 to $200,000,000 are lost to this ERC-20 flaw. Today we can increase this amount by $2,500,000.

The problem with ERC-20 token is that it doesn't allow for error handling which makes it impossible to prevent user errors. It was known for sure that the GLM contract is not supposed to accept GLM tokens. It was intended TO BE THE TOKEN, not to own the tokens. For example if you would send ether, NFT or ERC-223 token to the address of the said GLM contract - you wouldn't lose it.

Error handling is critical for financial software. Users do make mistakes. It's a simple fact. Whether it is misunderstanding of the internal logic of the contract, unfamiliar wallet UI, being drunk when sending a tx or panicking after hacking an exchange - doesn't matter. Anyone could be in a position of a person who just lost $2,5M worth of tokens to a simple bug in the software that could have been easily fixed.

I would use an opportunity to mention that ERC-223 was developed with the main goal of preventing such accidents of "funds loss by mistake: https://eips.ethereum.org/EIPS/eip-223

What is even worse - EIP process doesn't allow for security disclosures now. There is simply no way to report a security flaw in any EIP after its assigned "Final" status.

I'm proposing a modification to EIP process to allow for security disclosures here: https://ethereum-magicians.org/t/modification-of-eip-process-to-account-for-security-treatments/16265/12

There are ongoing debates on submission of an informational EIP regarding the ERC-20 security flaw: ethcatherders/EIPIP#293

And the Informational EIP pull request: ethereum/EIPs#7915

We've built ERC-20 <=> ERC-223 token converter that would allow both standards to co-exist and eventually prevent the issue of lost funds https://dexaran.github.io/token-converter/

Also my team is building a ERC-223 & ERC-20 compatible decentralized exchange that will also remove such a weird opportunity to lose all their life savings to a software bug from users: https://dex223.io/

If you are rich and worried about ERC-20 security bugs dealing damage to Ethereum ecosystem and ruining users days - welcome to our ERC-223 family. We stand for security. We don't let our users funds to be lost by mistake.

@mal-plankton
Copy link

I don't know why you keep calling this a security flaw when it's not.

  1. The hacker accidentally pasted the wrong address. Is it up to code to fix user errors and determine when you accidentally paste the wrong address?
  2. You're not supposed to send tokens to the contract. A security feature warn of potentially lost tokens would be a nice-to-have, not a need-to-have. This also adds additional complexity. And how would someone know whether you're accidentally sending to the wrong address, or purposely trying to burn tokens or send tokens to an upgradeable contract that may have use for those tokens in the future?

This is not a security bug. This is working as designed. I wouldn't want a security check on-chain. I would rather have it off-chain in the wallet client.

@Dexaran
Copy link
Author

Dexaran commented Nov 11, 2023

Is it up to code to fix user errors and determine when you accidentally paste the wrong address?

Yes. It is called "error handling" and it's a musthave feature.

For example if you send ETH, NFT or ERC-223 token to the token contract - it will be rejected and you will not lose your funds. ERC-20 doesn't implement error handling. It's a security flaw.

Lack of any critical feature in a software is a security flaw. If you don't put onlyOwner access restriction on a governance function it would be a security flaw.

You're not supposed to send tokens to the contract.

Token contract is not supposed to receive them in first place. In case of sending ETH / NFT / ERC-223 token - it rejects what it is not intended to receive. In case of ERC-20 its impossible due to a design flaw of ERC-20.

It is worth mentioning that if you send money to the wrong recipient via bank transfer you will not lose it. The bank is supposed to return it.

A security feature warn of potentially lost tokens would be a nice-to-have, not a need-to-have.

We don't need a warning. A contract that is not supposed to receive tokens must not be able to receive tokens just like it's unable to receive ETH if it doesn't explicitly declare "I am supposed to receive payments".

how would someone know whether you're accidentally sending to the wrong address, or purposely trying to burn tokens

The recipient contract explicitly declares whether it's supposed to receive funds or not. In case of ether there is a special function receive() payable.

In case of ERC-223 there is a special function tokenReceived(...).

If there is no such function that accepts the payment - consider the contract is not supposed to receive the payment and revert it. In software security it's called failsafe defaults principle: https://www.cs.purdue.edu/homes/clifton/cs526/hw6-sol.pdf

This is not a security bug. This is working as designed.

It is a security bug. We have a security bug in the standard.

I wouldn't want a security check on-chain. I would rather have it off-chain in the wallet client.

That's why we have $90,000,000 lost: https://dexaran.github.io/erc20-losses

What you would want - results in huge financial damage. If we want to save our users from losing all their life savings to software bugs that could be easily fixed then we must admit the things must work differently from what many of us would want.

@chimmykk
Copy link

chimmykk commented Dec 2, 2023

agree with the point from @Dexaran *Token contract is not supposed to receive them in first place. In case of sending ETH / NFT / ERC-223 token - it rejects what it is not intended to receive. In case of ERC-20 its impossible due to a design flaw of ERC-20.

It is worth mentioning that if you send money to the wrong recipient via bank transfer you will not lose it. The bank is supposed to return it.* It is a must needed upgrade, it's a complete flaw of the system if something that is not designated to there start accepting.

@marsrobertson
Copy link

Took me a while to figure to figure out you mean sending ERC20 to the token contract address.

Error handling can be done on wallet level. Metamask warning / preventing sending to token contract address.

@mal-plankton
Copy link

mal-plankton commented Dec 3, 2023

agree with the point from @Dexaran *Token contract is not supposed to receive them in first place. In case of sending ETH / NFT / ERC-223 token - it rejects what it is not intended to receive. In case of ERC-20 its impossible due to a design flaw of ERC-20.

It is worth mentioning that if you send money to the wrong recipient via bank transfer you will not lose it. The bank is supposed to return it.* It is a must needed upgrade, it's a complete flaw of the system if something that is not designated to there start accepting.

I still don't agree. What he said was incorrect. Contracts don't automatically send back ETH and NFTs. There are plenty of contracts that have ETH and NFTs in them. (Edit: What I wrote here is inaccurate. You need a payables function to receive ETH, so it's not available by default.) Some bridged tokens need this feature. You can know implement a return function, but it SHOULD NOT be the default option.

Also banks and services like Venmo don't send back money if you send them to the wrong person. Google it if you don't believe me. You have to request the other person send them back. Wired funds are gone forever. I learned this the hard way. If your bank is nice, they might cover you through their insurance, but it's at their discretion and loss.

@Dexaran
Copy link
Author

Dexaran commented Dec 3, 2023

@marsrobertson

Error handling can be done on wallet level. Metamask warning / preventing sending to token contract address.

It can be done since 2015 but it is not done and millions of dollars are already lost. Pretending that it will be done and all the UI/wallet devs will magically start doing it at some point makes no sense. They didn't in 9 years.

You can't just write insecure contract and pretend that someone else should develop a safe workaround for it. It violates "Secure by default" principle of software design.

There is a lot of research done in regards of "what should we do to make software secure?" and there are few common principles that you can easily google. ERC-20 violates this principles. $200M lost confirm it.
https://www.cisa.gov/sites/default/files/2023-04/principles_approaches_for_security-by-design-default_508_0.pdf

@Dexaran
Copy link
Author

Dexaran commented Dec 3, 2023

@mal-plankton

Contracts don't automatically send back ETH and NFTs.

Contracts that are not supposed to receive ETH or NFTs or ERC-223 tokens will reject them on transfer. A contract must explicitly declare that it is supposed to receive ETH / NFT / ERC-223 tokens by implementing a special function in the code. By default contracts reject incoming payments.

In some cases you can deposit funds to some malicious contract that declares "I want to receive payments" but doesn't need to be able to receive payments. <== this is not the issue we are trying to solve.

  • Sending to wrong address is an issue that can't be prevented on a token standard level.

  • Contract not being able to reject a known-to-be-incorrect transfer is an issue that needs to be prevented on the token standard level.

You can know implement a return function, but it SHOULD NOT be the default option.

If we would be talking about some browser application where a mistake would result in some font not being properly - OK.

If we are talking about financial software, a bug in which can cause someone to lose all their life savings - it should be the default.

Again, software security has some rules such as:

  • Fail Safe defaults
  • Error handling is a must
  • Principle of list privilege
  • Weakest Link
    etc.

ERC-20 clearly violates the principle of failsafe defaults and error handling is a must and $200M are lost as the result. This is what happens when you violate a widely known security principle.

https://owasp.org/www-project-developer-guide/draft/04-foundations/03-security-principles

https://kirkpatrickprice.com/blog/secure-coding-best-practices/

https://www.cisa.gov/sites/default/files/2023-04/principles_approaches_for_security-by-design-default_508_0.pdf

@mal-plankton
Copy link

mal-plankton commented Dec 3, 2023

@mal-plankton

Contracts don't automatically send back ETH and NFTs.

Contracts that are not supposed to receive ETH or NFTs or ERC-223 tokens will reject them on transfer. A contract must explicitly declare that it is supposed to receive ETH / NFT / ERC-223 tokens by implementing a special function in the code. By default contracts reject incoming payments.

Ok. You're right about that. I'm having a brain fart.
You need a payables or receive function.


For the bank thing. Wired funds are lost forever. You can never get them back unless the government stops the transfer due to sanctions violation.

For non-wired transfers, they need the bank or the recipient to return the funds. This is not automatic, and it often requires going through the legal system. They have to use another transaction back to send it back.


Taking a step back and thinking about this philosophically, I wish Ethereum had something more similar to standardized native tokens like TRC-10 tokens, BRC-20 tokens, or Cardano's native tokens. These are tokens where all their functions are standardized and the only differences are the input parameters like symbol, name, total supply, mintable, burnable, etc.

ERC-20 is the opposite of them because it's purposely so flexible. You can create scam tokens, spoofed tokens, and functions that return arbitrary values. And that's just how ERC-20 is. I'm not sure I'd want to lock it down. If I wanted something else, I'd use ERC-223 or create another standard that's more similar to native tokens.

I'm not going to oppose you in pushing for this change. It would've been very nice to have if this safety feature had been implemented from the start. But I'm concerned that changing the default action now would possibly break some dApps that are expecting tokens to be sent to the contract.

@Dexaran
Copy link
Author

Dexaran commented Dec 3, 2023

I'm concerned that changing the default action now would possibly break some dApps that are expecting tokens to be sent to the contract.

Yes, this is the main challenge with the adoption of a new standard. However, this is the only secure option.

In order to address this problem I've created ERC-7417: Token Converter. A service that will allow ERC-20 and ERC-223 tokens to co-exist. It will create a ERC-223 "fork" for every existing ERC-20 token and allow users to transform tokens from one standard to another.

@marsrobertson
Copy link

FYI... The other day I also suggested to include it by default: WETH10/WETH10#94

Ability to rescue tokens and ETH sent to contract address

Also here, popped in my timeline: https://x.com/PaulRBerg/status/1731360509533020199?s=20

image

@marsrobertson
Copy link

FYI... https://dexaran.github.io/erc20-losses/

image

Some token contract addresses are more advanced, they include staking / burning / streaming / locking / vesting / migrating to V2 / any other primitive.

Just saying that the list is not 100% accurate. But still get the point, it can be life changing situation to some victims, pretty sure some people selfdestruct as a consequence.

@Dexaran
Copy link
Author

Dexaran commented Dec 4, 2023

Some token contract addresses are more advanced, they include staking / burning / streaming / locking / vesting / migrating to V2 / any other primitive.

Yeah, we already did some research and removed tokens that were obviously deposited to contracts on purpose. This is a very time-consuming task however as we need to go through all the media associated with hundreds of tokens to figure out what happened.

Here is a list of exclusions (without exclusions there were $2.4B lost):

https://github.com/Dexaran/dexaran.github.io/blob/master/erc20-losses-sources/src/constants/excludes.json

@Dexaran
Copy link
Author

Dexaran commented Dec 4, 2023

@marsrobertson

FYI... The other day I also suggested to include it by default: WETH10/WETH10#94

Thats a good effort but I don't believe that the problem can be solved on the wallet level. This is an inherent problem with the token standard.

  1. You need to deposit tokens to contracts. Tokens that can't be deposited are useless.
  2. ERC-20 doesn't have a "communication model" i.e. when it's sent to the contract it's impossible to recognize the transaction for the recipient.
  3. In order to address pt.2 Vitalik and @frozeman developed a weird quirk which resembles a pull transacting method: approve & transferFrom pattern. It was not a smart design, it was a crutch to address a bug in early days EVM that doesn't exist since it was fixed in 2016.
  4. However, leaving transfer() function without handling makes it impossible to handle transactions and therefore it's impossible to handle errors.
  5. At the same time approve&transferFrom is very bad. It takes 2 transactions to deposit a token to contract this way, it requires more GAS than a direct deposit with a callback invocation, it introduces security risk as users are authorizing some third party to have access to their funds hence all the permit scams and approval-related accidents. Approvals are (1) unnecessary and (2) insecure.

Even if we solve the problem with unhandled transfer function in ERC-20 at the wallet level the problem of inherently wrong design and insecure method of depositing via approve&transferFrom will persist.

And the root of the problem is pt.1 - we need a proper method of depositing tokens to contracts. Pull tx method is good for credit cards, not for trustless systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment