zeta-chain / protocol-contracts

Protocol contracts implementing the core logic of the protocol, deployed on ZetaChain and on connected chains
MIT License
67 stars 50 forks source link

Integrate ZRC-223 standard for ZRC20 #107

Open Dexaran opened 7 months ago

Dexaran commented 7 months ago

ERC-20 standard security flaw caused millions of dollars losses

ERC-20 token standard has a major security flaw that I disclosed to Ethereum Foundation in 2017 but they ignored it.

ERC-20 does not implement "transaction handling" for transfer function and it makes it impossible to handle erroneous transfers OR deposit tokens to contracts using the transfer function i.e. if a user sends Ether, an ERC-721 NFT token, or an ERC-223 token to a contract that does not explicitly declare that it is supposed to receive them, then the tokens or Ether are rejected and the transaction automatically fails. This doesn't work with ERC-20 - if a user sends ERC-20 to some random contract then the transaction will succeed and the tokens become "frozen" at the contract's address with no way to recover them. As of today, the described problem caused $90,000,000 to $200,000,000 financial damage to users of Ethereum tokens.

Why ERC-20 is designed the way it is

There was a bug in EthereumVM at the time when ERC-20 was developed. Pull transacting method was introduced to make tokens not affected by this bug. It was a quirk that addressed the earlydays bug in the VM, not a smart design.

Here is a @vbuterin 's comment regarding the situation with token standards and call stack depth problem:

67496b44-afc0-4acc-bc1b-bafb9be43e5c

approve & transferFrom are unnecessary now and they introduce security risks as well as they authorize third parties to operate with users funds. This is not as critical as the lack of error handling with transfer func however.

Source: https://github.com/ethereum/ethereum-org-website/issues/10854#issuecomment-1679275590

ZRC20 is affected

ZRC20 implementation is prone to the same exact issue and will inevitably cause financial damage to your customers if it is not fixed:

https://github.com/zeta-chain/protocol-contracts/blob/main/contracts/zevm/ZRC20.sol#L178-L189

Alternative token standard support proposal

I've designed an alternative ERC-223 token standard to solve this exact problem of ERC-20 security bug and it was finalized in 2023.

Obviously ERC-20 will not go away overnight, but I'm building the ecosystem for ERC-223 tokens currently. I believe that in the long run the standard that does not burn users money will thrive. The main idea is to make ERC-20 and ERC-223 interoperable for now and let a dual standard ecosystem settle.

I'm ready to contribute to the development of the safer token standards. We can support Zeta testnet with our token converter. I can write example ERC-223 (or ZRC-223) contracts to demonstrate an alternative way of implementing tokens on Zeta Chain.

We can deploy Dex223 on Zeta chain.

lumtis commented 7 months ago

Hi @Dexaran thank you for the report. However, the notion of "critical security flaw" seems over-exaggerated. The link you posted mention about logical flaw. Can you explain how you would consider ERC20 implementation as a security flaw?

Dexaran commented 7 months ago

Can you explain how you would consider ERC20 implementation as a security flaw?

It falls in "critical severity security vulnerability" criteria in OpenZeppelin bug bounty.

255351496-0246fb67-bf3d-494e-97f4-1efcff64a06e

https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4451

Dexaran commented 7 months ago

Again, ERC-20 implementation violates some well-known principles of secure software development.

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

Violating some widely adopted and well-documented principles of secure software development will result in your software not being secure (quite logical). Hence millions of dollars are lost due to the ERC-20 flaws that I described.

I'm not saying that your standard has a security vulnerability and someone will hack it. I'm saying that your token standard (as well as the base ERC-20 standard) is not secure by default and it will result in your users losing money without a hack.

Dexaran commented 7 months ago

You can probably apply a "bandage" to ZRC-20 by prohibiting direct transfers to smart-contract addresses via transfer function. This will require you to redefine some part of the Uniswap logic however as it does unchecked transfers via transfer func.

See contracts below

(You can optimize the gas usage by issuing an unlimited approval at the moment of the pair contract creation to avoid multiple approve calls)