SuperchainERC20: Redesign

gotzenx commented 4 weeks ago

https://github.com/ethereum-optimism/specs/pull/384 https://github.com/ethereum-optimism/specs/pull/413

Dexaran commented 3 weeks ago

I would like to draw your attention to a security issue with your token standard that will definitely affect your users and cause them to lose their funds permanently.

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

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

Security flaw

ERC-20 standard contains a security flaw: lack of error handling on its transfer function.

The ERC-20 standard was created in 2015, at that time there was a 1024-call-stack-depth bug. So in order to make tokens unaffected by the earlydays EVM bug there were two transacting methods implemented in ERC-20 standard: (1) transfer function and (2) approve & transferFrom pattern.

1024-call-stack-depth bug was fixed in Tangerine whistle hardfork at block 2463000 in 2016. At this point the approve & transferFrom should have been deprecated but it didn't happen.

As the result, ERC-20 standard implement still has two transferring patterns, one is designed to transfer tokens between externally owned addresses and the other is designed to deposit token to contracts. The transfer function does not implement error handling at all, which is a security flaw. If a user will use the transfer function to deposit tokens to a contract then instead of getting an error the tokens will be subtracted from the user's balance but the deposit will not be recognized by the recipient contract.

Also it is impossible to prevent incorrect ERC-20 deposits to contracts which were never designed to receive or hold tokens such as token contracts themselves (they are supposed to BE tokens, not to HOLD tokens).

Keep in mind that this is only an issue of the ERC-20 standard. If you will send an asset to a contract which is not designed to receive it then the following will happen:

you will NOT lose native currency in this scenario, the contract must explicitly declare that it is supposed to receive native currency by implementing a payable function
you will NOT lose ERC-721 NFTs in this scenario, these act similar to native currency
you will NOT lose ERC-223 tokens, these act similar to native currency
you WILL LOSE ERC-20 tokens, these fail to implement proper error handling

The history of the problem

I've discovered this problem in 2017, 8 years ago. At that time there were about $10,000 lost. This is my post
I've notified the creator of ERC-20 standard about the issue and the fact that it can cause funds losses in 2017, at that time there were $16,000 worth of lost tokens. This is my github comment on ERC-20 finalizing thread
Here is the description of the problem on Ethereum reddit.
On December 23, 2017 one user lost $130,000 worth of their tokens and it is a well-documented case. Here is one of the posts
In 2018 the amount of lost funds due to this problem has grown to $1,000,000.
The creator of ERC-20 standard refused to use it in his new project. Here is his tweet, later he confirmed that this is a security issue that needs to be fixed. Here is the tweet.
In 2023 a user lost $240,000 worth of ERC-20 tokens in a single transaction due to this well-known flaw. Here is the post
The issue was escalated to EthereumCatHerders and there is a consensus that this is a security flaw, however EIP process does not allow for security disclosures in finalized EIP texts so I'm doing a disclosure myself, here at your github issue. Here are historical references to EthCatHerders discussions (this, this and this)

The scale of the problem: $80,000,000 are lost due to this problem

I've built a script that calculates the amount of ERC-20 tokens which were lost because of this well-known and fixable problem which persists since 2017. As of 8 October 2024 there were $83,000,000 worth of ERC-20 tokens lost:

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

Security disclosure

You are about to build your ecosystem on top of the ERC-20 standard which contains a serious security flaw, which caused $80,000,000 loss to Ethereum token users due to lack of error handling in the transfer function.

Your users are guaranteed to lose tokens if you will not fix it.

Recommendations

It is possible to restrict the transfer function by disallowing it to perform deposits to contracts (which in fact it is not supposed to be used for).

    function transfer(address _to, uint256 _value) public override returns (bool success)
    {
        require(!_to.isContract(), "Transfers to contracts are not allowed.");
        balances[msg.sender] = balances[msg.sender] - _value;
        balances[_to] = balances[_to] + _value;
        emit Transfer(msg.sender, _to, _value);
        return true;
    }

Alternatively you can disable transfers to the contract of the token itself. This will not fix all the problems however as other contracts will be able to receive ERC-20 tokens without handling the deposit transaction.

As you can see there are plenty of LINK, USDC, CRO tokens in the USDT contract address on Ethereum chain, all these tokens are unrecoverably lost due to the flaw in ERC-20 standard: https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7

    function transfer(address _to, uint256 _value) public override returns (bool success)
    {
        require(!_to != address(this), "Transfers to contracts are not allowed.");
        balances[msg.sender] = balances[msg.sender] - _value;
        balances[_to] = balances[_to] + _value;
        emit Transfer(msg.sender, _to, _value);
        return true;
    }

You can use alternative standards, like I've designed ERC-223 to address this specific issue, https://github.com/ethereum/eips/issues/223, in the original thread it was written in the Motivation section that this standard is supposed to solve the problem of lost money and millions of dollars were lost at the moment of it's creation in 2018.

You can take a look at ERC-1363 as it does not override the logic of the transfer function of ERC-20 and only extends the standard with new functions. You would have to apply the restriction on transfer function however, as the pure ERC-1363 inherits the ERC-20 security flaw in it's default implementation.

skeletor-spaceman commented 3 weeks ago

Hi @Dexaran ! Thanks for the suggestion, this standard is agnostic and should work on top of erc20s and any other token standard. We'll not be implementing any modifications to the erc20 contract, nor be using another custom standard as it's out-of-scope for this specific case.a We are well aware that user or dev errors might end up in non-recoverable funds, but thanks for the heads-up :)

ethereum-optimism / optimism