ERC20 standard known issues.

[Dexaran]

ERC20 token standard known issues.

All this tokens are affected: https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC20

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

Long story short: there is no transaction handling implementation for transfer function in ERC20 standard (it is the flaw of the standard as it must enforce it - it is a critical feature).

On 27 Dec, 2017 there were $4 million worth of tokens lost due to this flaw of the standard.

On 9 Mar, 2023 there were about $12 million worth of tokens lost.

As of today there are about $30 millions worth of tokens lost.

Since the tokens are not recoverable the amount can only increase and in fact it WILL increase over time.

💻 Environment

Remix or whatever environment you may use to compile contracts.

📝 Details

Users of ERC-20 tokens can lose their tokens.

I suggest:

1. Implement a special security check that prevents sending of ERC-20 tokens where they must not be delivered. In particular the transfer(...) function must not be used to send tokens to ANY contracts (because contracts rely on approve+transferFrom as a depositing pattern).

    function transfer(address to, uint256 value) public virtual returns (bool) {
        require(!Address.isContract(to), "ERC20: transfer to contracts must be performed via approve+transferFrom pattern");
        address owner = _msgSender();
        _transfer(owner, to, value);
        return true;
    }

2. Add a corresponding paragraph to the documentation or code comments to warn the users and token developers about any potential issues. A warning that transferring tokens to contracts via transfer(...) func will result in a deposit that will not be recognized by the recipient i.e. the recipient CAN NOT KNOW that the deposit occured and it will not be credited as well as it can not be rejected by the recipient.

3. Suggest implementing the ERC20Rescure(...) function in every contract which is supposed to work with tokens in order to extract any unintentionally deposited ERC20 tokens that were not recorded.

    // Rescue ERC20 tokens
    function rescueERC20(address _token, uint256 _value) external {
        require(msg.sender == ISoyFinanceFactory(factory).feeToSetter(), 'SoyFinance: FORBIDDEN');
        // bytes4(keccak256(bytes('transfer(address,uint256)')));
        _token.call(abi.encodeWithSelector(0xa9059cbb, msg.sender, _value));
    }

It is relevant and it often happens. Rescue missions are necessary for ERC20 tokens. For example AAVE handled it already https://twitter.com/AaveAave/status/1633126370166575104

🔢 Code to reproduce bug

It is a flaw of the standard, not any particular implementation so I suggest to add some features that may help to mitigate the consequences to some degree or at least lower the risks for end users.

[RenanSouza2]

1 - the isContract method is deprecated

2 - uniswap expects you to transfer to it's contract throught transfer, not transferFrom so this would make any token that uses this non compatible with some defi projects

maybe this could be in an ERC20 extension? what do you think?

[Amxx]

This is an issue with ERC20, but changing that behavior breaks more things than it fixes, because the resulting token won't be ERC20 compliant.

Many alternatives have been proposed, mostly ERC223 and ERC777, they both have more issues than real benefit.

Our code is following the standard, which ensures interoperability with DeFi. If another standard ever reach consensus in the community, we will implement it ... but so far all effort in that dirrection has failled.

[Dexaran]

2. And what about adding a warning "If you transfer ERC-20 tokens to a contract it may result in unrecognized deposit that will not be handled on the recipients side." ?

3. And what about ERC20Rescue() function?

Also the most common scenario is depositing tokens to another token contract. What if we can create a library of deployed ERC20 token contracts and disallow ERC20 transfers to this contracts? (Token must not be transferred to TOKEN CONTRACT because token contract is obviously not intended to receive tokens. It is intended to BE a token)

You are saying your contracts are secure:

but your implementation of ERC-20 resulted in $30 millions worth of tokens being lost

Let's do something? We certainly can.

[Amxx]

Also the most common scenario is depositing tokens to another token contract. What if we can create a library of deployed ERC20 token contracts and disallow ERC20 transfers to this contracts? (Token must not be transferred to TOKEN CONTRACT because token contract is obviously not intended to receive tokens. It is intended to BE a token)

This part has been discussed and documented many times. Developpers that want to add that feature to their token can do it by overloading the _beforeTokenTransfer hook (in v4.x) or _update (in v5.0) to add an additional check.

[Dexaran]

This part has been discussed and documented many times. Developpers that want to add that feature to their token can do it by overloading the _beforeTokenTransfer hook (in v4.x) or _update (in v5.0) to add an additional check.

Well, if you leave it up to token developers without highlighting a necessity of doing this with a big red banner "It is necessary for security of your users" - the problem will not be solved.

New tokens still suffer from the issue that was discussed millions of times. May be it is worth to highlight it a bit more.

[izcoser]

This should be handled by wallet providers instead to avoid unnecessary bloat to the transfer function. Should be extremely simple for Metamask for example to add a warning "are you sure you want to transfer to a contract?".

[Dexaran]

This should be handled by wallet providers instead to avoid unnecessary bloat to the transfer function. Should be extremely simple for Metamask for example to add a warning "are you sure you want to transfer to a contract?".

This is a good way to never solve a problem - delegate it to wallet devs. Back in 2017-2018 we discussed this and also concluded that it can be solved on the side of software that interacts with tokens.

In theory it should work. In practice it certainly doesn't because wallet devs are even less aware of an issue than contract devs.

And it is certainly not possible to guarantee that all wallet developers in the future will always solve this problem. New wallets and new developers will emerge... so if the underlying technology has some sort of problems/vulnerabilities/security flaws - it will result in lost money.

[izcoser]

not possible to guarantee that all wallet developers in the future will always solve this problem

Not a problem, people are free to choose a wallet that solves the problem.
How much does it cost for a dev to add a check for that in his wallet? An insignificant amount compared to what users would collectively pay in fees for that require statement.

[Dexaran]

We can solve the problem on the standard level and it will ACTUALLY solve the problem.

What I'm proposing here is to mitigate the problem with a better reference implementation and highlighting danger areas in the description and documentation of OpenZeppelin contracts. It WILL help to some degree.

Saying "everything is fine, let's do nothing and hope wallet devs will solve the problem" WILL NOT solve the problem as you can see - we did it in 2017 and the amount of lost tokens grew from $4M to $30M.

[izcoser]

How much is 30M as a percentage of all transaction volume? Does it justify making things more expensive for everyone else?

[sgitt-vassky]

are we seriously discussing if 30M loss are a problem or not? if it would be stolen by hackers nobody will question if they needed to prevent it or not

[frangio]

We can improve documentation, but the two technical solutions that were proposed are not viable.

1. There is no way we can add require(!isContract(to)) in the transfer function. This would make the transfer function stop working when transfering tokens to smart contract wallets.
2. I would love to provide a "rescue" function that works out of the box, but such a function needs to ensure it doesn't allow stealing tokens that actually belong to the protocol. The example implementation provided does not guarantee that at all.

[izcoser]

Well remembered. Smart contract wallets are pretty much going to become the standard in the future, one more reason why 1. is terrible.

[theRomanMercury]

This is an issue with ERC20, but changing that behavior breaks more things than it fixes, because the resulting token won't be ERC20 compliant.

Many alternatives have been proposed, mostly ERC223 and ERC777, they both have more issues than real benefit.

Our code is following the standard, which ensures interoperability with DeFi. If another standard ever reach consensus in the community, we will implement it ... but so far all effort in that dirrection has failled.

The main problems of ERC-20 that ERC-223 solves

Lost tokens: there are two different ways to transfer ERC20 tokens depending on is the receiver address a contract or a wallet address. You should call transfer to send tokens to a wallet address or call approve on token contract then transferFrom on receiver contract to send tokens to contract. Accidentally call of transfer function to a contract address will cause a loss of tokens inside receiver contract.

Impossibility of handling incoming token transactions / lack of event handling in ERC20: ERC20 token transaction is a call of transfer function inside token contract. ERC20 token contract is not notifying receiver that transaction occurs. Also there is no way to handle incoming token transactions on contract and no way to reject any non-supported tokens.

Optimization of ERC20 address-to-contract communication: You should call approve on token contract and then call transferFrom on another contract when you want to deposit your tokens into it. In fact address-to-contract transfer is a couple of two different transactions in ERC20. It also costs twice more gas compared to ERC223 transfers. In ERC223 address-to-contract transfer is a single transaction just like address-to-address transfer.

Ether transactions and token transactions behave differently: one of the goals of developing ERC223 was to make token transactions similar to Ether transactions to avoid users mistakes when transferring tokens and make interaction with token transactions easier for contract developers.

ERC-223 advantages.

Provides a possibility to avoid accidentally lost tokens inside contracts that are not designed to work with sent tokens.

Allows users to send their tokens anywhere with one function transfer. No difference between is the receiver a contract or not. No need to learn how token contract is working for regular user to send tokens.

Allows contract developers to handle incoming token transactions.

ERC223 transfer to contract consumes 2 times less gas than ERC20 approve and transferFrom at receiver contract.

Allows to deposit tokens into contract with a single transaction. Prevents extra blockchain bloating.

Makes token transactions similar to Ether transactions.

ERC-223 tokens are backwards compatible with ERC-20 tokens. It means that ERC-223 supports every ERC-20 functional and contracts or services working with ERC-20 tokens will work with ERC-223 tokens correctly. ERC-223 tokens should be sent by calling transfer function on token contract with no difference is receiver a contract or a wallet address. If the receiver is a wallet ERC-223 token transfer will be same to ERC-20 transfer. If the receiver is a contract ERC-223 token contract will try to call tokenReceived function on receiver contract. If there is no tokenReceived function on receiver contract transaction will fail. tokenReceived function is analogue of fallback function for Ether transactions. It can be used to handle incoming transactions.

There is a way to attach bytes _data to token transaction similar to _data attached to Ether transactions. It will pass through token contract and will be handled by tokenReceived function on receiver contract. There is also a way to call transfer function on ERC-223 token contract with no data argument or using ERC-20 ABI with no data on transfer function. In this case _data will be empty bytes array.

The reason of designing ERC-223 token standard.

Here is a description of the ERC-20 token standard problem that is solved by ERC-223:

ERC-20 token standard is leading to money losses for end users. The main problem is lack of possibility to handle incoming ERC-20 transactions, that were performed via transfer function of ERC-20 token.

If you send 100 ETH to a contract that is not intended to work with Ether, then it will reject a transaction and nothing bad will happen. If you will send 100 ERC-20 tokens to a contract that is not intended to work with ERC-20 tokens, then it will not reject tokens because it cant recognize an incoming transaction. As the result, your tokens will get stuck at the contracts balance.

Source: https://github.com/Dexaran/ERC223-token-standard

[theRomanMercury]

We can improve documentation, but the two technical solutions that were proposed are not viable.

1. There is no way we can add require(!isContract(to)) in the transfer function. This would make the transfer function stop working when transfering tokens to smart contract wallets.
2. I would love to provide a "rescue" function that works out of the box, but such a function needs to ensure it doesn't allow stealing tokens that actually belong to the protocol. The example implementation provided does not guarantee that at all.

The ERC-223 token standard was specifically designed to address the issue you mentioned. It allows for the safe transfer of tokens to both contract wallets and externally owned accounts (EOAs) without the risk of lost tokens.

In the ERC-223 standard, the transfer function is modified to include an additional parameter, data, which can be used to pass additional information to the receiving contract. This allows the contract to handle the token transfer in a more controlled manner.

The transfer function in ERC-223 looks like this:

function transfer(address to, uint value, bytes calldata data) public virtual returns (bool success);

With ERC-223, when you transfer tokens to a contract wallet, the contract's code is executed. If the contract implements a token fallback function that can handle the data parameter, it will receive and process the tokens correctly. If the contract does not implement a fallback function, the transaction will revert, preventing the tokens from being lost.

ERC-223 provides a more secure and flexible alternative to the traditional ERC-20 token standard, specifically when interacting with contracts. It ensures that tokens are not accidentally lost due to improper transfers to contracts that cannot handle them.

[Dexaran]

We can improve documentation, but the two technical solutions that were proposed are not viable.

Ok, if you would improve the documentation you would probably save some people from losing money.

There is no way we can add require(!isContract(to)) in the transfer function. This would make the transfer function stop working when transfering tokens to smart contract wallets.

And transfer shouldn't be used to transfer tokens to contracts. The documentation of ERC-20 standard states that approve+transferFrom pattern is used for depositing funds. So all devs who rely on depositing ERC-20 to contracts rely on transfer function are doing it wrong and its their problem.

Relying on what you shouldn't rely on is a security issue that can result in millions of dollars loss - which in fact already happened.

If you want to deposit funds to contract - you can just send them via transferFrom yourself. The sender of the transaction can transferFrom the tokens from his own wallet similarly to how the contract can pull tokens in.

I would love to provide a "rescue" function that works out of the box, but such a function needs to ensure it doesn't allow stealing tokens that actually belong to the protocol. The example implementation provided does not guarantee that at all.

Implementation must be contract-specific.

If your contract calculates how much tokens were deposited then the difference between "how much a contract thinks is deposited" and "how much a token contract thinks is on balanceOf(...) of the contract" is in fact the value that can be ERC20Rescue'd

[Dexaran]

Did you know that in order to calculate how much ERC-20 tokens were lost in the contract we need to perform a number of calls so big that you can't fit it in a single tweet?

Here is a list of tokens https://etherscan.io/tokens

There are 1200 ERC-20 tokens.

We want to take USDT and calculate how much USDT are stuck in USDT contract, then how much USDT are stuck in USDC contract, how much USDT are stuck in BNB contract and so on...

Then we want to take USDC and calculate how much USDC are stuck in USDT contract, in USDC contract, in BNB contract and so on...

Then we take BNB and calculate stuck BNB for USDT, USDC, BNB and so on...

1200! of calculations.

That much:

402.387.260.077.093.773.543.702.433.923.003.985.719.374.864.210.714.632.543.799.910.429.938.512.398.629.020.592.044.208.486.969.404.800.479.988.610.197.196.058.631.666.872.994.808.558.901.323.829.669.944.590.997.424.504.087.073.759.918.823.627.727.188.732.519.779.505.950.995.276.120.874.975.462.497.043.601.418.278.094.646.496.291.056.393.887.437.886.487.337.119.181.045.825.783.647.849.977.012.476.632.889.835.955.735.432.513.185.323.958.463.075.557.409.114.262.417.474.349.347.553.428.646.576.611.667.797.396.668.820.291.207.379.143.853.719.588.249.808.126.867.838.374.559.731.746.136.085.379.534.524.221.586.593.201.928.090.878.297.308.431.392.844.403.281.231.558.611.036.976.801.357.304.216.168.747.609.675.871.348.312.025.478.589.320.767.169.132.448.426.236.131.412.508.780.208.000.261.683.151.027.341.827.977.704.784.635.868.170.164.365.024.153.691.398.281.264.810.213.092.761.244.896.359.928.705.114.964.975.419.909.342.221.566.832.572.080.821.333.186.116.811.553.615.836.546.984.046.708.975.602.900.950.537.616.475.847.728.421.889.679.646.244.945.160.765.353.408.198.901.385.442.487.984.959.953.319.101.723.355.556.602.139.450.399.736.280.750.137.837.615.307.127.761.926.849.034.352.625.200.015.888.535.147.331.611.702.103.968.175.921.510.907.788.019.393.178.114.194.545.257.223.865.541.461.062.892.187.960.223.838.971.476.088.506.276.862.967.146.674.697.562.911.234.082.439.208.160.153.780.889.893.964.518.263.243.671.616.762.179.168.909.779.911.903.754.031.274.622.289.988.005.195.444.414.282.012.187.361.745.992.642.956.581.746.628.302.955.570.299.024.324.153.181.617.210.465.832.036.786.906.117.260.158.783.520.751.516.284.225.540.265.170.483.304.226.143.974.286.933.061.690.897.968.482.590.125.458.327.168.226.458.066.526.769.958.652.682.272.807.075.781.391.858.178.889.652.208.164.348.344.825.993.266.043.367.660.176.999.612.831.860.788.386.150.279.465.955.131.156.552.036.093.988.180.612.138.558.600.301.435.694.527.224.206.344.631.797.460.594.682.573.103.790.084.024.432.438.465.657.245.014.402.821.885.252.470.935.190.620.929.023.136.493.273.497.565.513.958.720.559.654.228.749.774.011.413.346.962.715.422.845.862.377.387.538.230.483.865.688.976.461.927.383.814.900.140.767.310.446.640.259.899.490.222.221.765.904.339.901.886.018.566.526.485.061.799.702.356.193.897.017.860.040.811.889.729.918.311.021.171.229.845.901.641.921.068.884.387.121.855.646.124.960.798.722.908.519.296.819.372.388.642.614.839.657.382.291.123.125.024.186.649.353.143.970.137.428.531.926.649.875.337.218.940.694.281.434.118.520.158.014.123.344.828.015.051.399.694.290.153.483.077.644.569.099.073.152.433.278.288.269.864.602.789.864.321.139.083.506.217.095.002.597.389.863.554.277.196.742.822.248.757.586.765.752.344.220.207.573.630.569.498.825.087.968.928.162.753.848.863.396.909.959.826.280.956.121.450.994.871.701.244.516.461.260.379.029.309.120.889.086.942.028.510.640.182.154.399.457.156.805.941.872.748.998.094.254.742.173.582.401.063.677.404.595.741.785.160.829.230.135.358.081.840.096.996.372.524.230.560.855.903.700.624.271.243.416.909.004.153.690.105.933.983.835.777.939.410.970.027.753.472.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000

That's the number of options a user has to lose his tokens due to poorly coded tokens. We can do something about it.

@RenanSouza2 @Amxx

[Dexaran]

I have a script that calculates the number of lost tokens: https://dexaran.github.io/erc20_losses/

There are $1,004,000,000 worth of tokens lost. 1 BILLION DOLLARS worth of tokens.