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

Deprecate ERC777 implementation #2620

Closed
k06a opened this issue Mar 29, 2021 · 71 comments · Fixed by #4066
Closed

Deprecate ERC777 implementation #2620

k06a opened this issue Mar 29, 2021 · 71 comments · Fixed by #4066
Milestone

Comments

@k06a
Copy link
Contributor

k06a commented Mar 29, 2021

🧐 Motivation

There is an opinion that ERC777 is over-engineered and is a bad practice to follow. Moreover it introduces bad abstractions to rely on and requires very important checks to be implemented by every integrator.

Let's switch to EIP2612 (Permit) to deprecate dangerous infinite approve behavior and make it mainstream ASAP: https://github.com/OpenZeppelin/openzeppelin-contracts/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

📝 Details

Do we need to collect list of issues? Starting here:

  1. Whole idea of avoiding spam tokens by having hook function is wrong. It is not possible to protect from spam tokens because developers of these tokens will always modify their tokens to allow spamming – just ignore them.

  2. Token fallback concept is wrong because the callback is being called from token smart contract and there is no way to verify who was the original msg.sender. There are so many possible ways to abuse ERC777Receiver. The only way to solve it I see – work with whitelisted tokens only – DeFi deserves better approach. Imagine you have DEX and wanna deposit token and do custom action (swap):

    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external override {
        address token = msg.sender;
        balances[token][operator] += amount;
        _performSwap(operator, operatorData); // <- `operator` is not trustworthy, due `msg.sender` can be malicious smart contract
    }
    
    function _performSwap(address user, bytes memory data) internal {
        // ...
    }
@Amxx
Copy link
Collaborator

Amxx commented Mar 29, 2021

Hello @k06a,
As a developper working with ERC777 during hackathon, I can only agree that this standard creates a lot of frustration ... and can result in very gas-extensive transactions.

However, deprecating contract breaks backward compatibility. This is not something we would consider doing during a "minor" release. The latest major release was v4.0, less then a month ago. Thus, we wouldn't move with anything like that before a while.

Still, I'd love to build a case for/against ERC777 (and other contract that might not be relevant). This will help us taking decision whenever we fell like moving to the next major version.

@rstormsf
Copy link
Contributor

rstormsf commented Mar 29, 2021

I definitely support this, so new solidity developers won't inherit this broken token design pattern. We have seen enough exploits/hacks with this approach.

@k06a
Copy link
Contributor Author

k06a commented Mar 30, 2021

@Amxx @rstormsf thanks for your support, added 2 cases to the OP.

@frangio
Copy link
Contributor

frangio commented Apr 10, 2021

Just to clarify, we can't remove the contract until the next major release, but if there is consensus we can deprecate it, in the sense of announcing its deprecation, not recommending or promoting it anymore, possibly hiding it from the documentation site, etc.

Thank you all for raising this.

@phbdias
Copy link

phbdias commented May 14, 2021

I agree that there are some issues with ERC777, however I don't see any alternative to avoid the ERC20 behavior of 'approve' then 'transferFrom' (opposing to the one-step transfer that the ERC777 proposes). There are any safer alternative to ERC777 (already implemented) that I am missing out?

Thanks in advance!

@k06a
Copy link
Contributor Author

k06a commented May 14, 2021

@phbdias sure, check out ERC20Permit.sol

@Amxx
Copy link
Collaborator

Amxx commented May 15, 2021

@phbdias ERC1363, which is not yet part of OZ contracts, but can easily be added is there is a request for it, is also good to know !

@frangio
Copy link
Contributor

frangio commented Jun 1, 2021

We don't think it should be OpenZeppelin Contracts making the decision to deprecate the ERC.

We're happy to include comments in our documentation about the downsides and alternatives.

For actual deprecation, we're happy to host the discussion here, but ultimately we feel that a deprecation requires a more ecosystem-wide consensus.

@jeffreyscholz
Copy link

@Amxx as rough as the transition is, I think some token that supports a receiver hook needs to be supported. If it isn't ERC777, then by all means ERC1363. Having to approve tokens first is counterintuitive to newcomers and it introduces attack vectors. I don't have strong opinions about the ERC777, but ERC1363 does seem more intuitive to me. Regardless, I'm 100% behind moving to a new token standard away from ERC20. It needs to happen at some point.

@k06a
Copy link
Contributor Author

k06a commented Feb 11, 2022

We can have better EIP which would allow receiver contract to do transferFrom(msg.sender, …), during transferAndCall where msg.sender will be token contract. But this flow would make it fully compatible with ERC20. Except msg.sender will not be beneficiary of the call anymore.

@k06a
Copy link
Contributor Author

k06a commented Feb 16, 2022

Here is the concept of CREATE2-based factory for arbitrary calls, where the receiver of the call still can do transferFrom(msg.sender, ..., ...) to have ultimate compatibility with ERC20: https://gist.github.com/k06a/0ea02ecd824bd3d44c760825b62bc264

Pros:

  • Smart contract ownership role is not exposed anymore. It uses the factory to have a separate "caller" smart contract for each user.
  • The receiver can be ultimately compatible with ERC20

Cons:

  • The call receiver should not consider msg.sender as beneficiary, we should use methods like depositFor and similar. But even if it was used, create2-based proxy factory would allow users to reclaim their ownership.

@frangio
Copy link
Contributor

frangio commented Feb 17, 2022

@k06a I was going to mention ERC827 but I see you were involved in that discussion already!

I think the approach with CREATE2 is interesting but isn't it an important security practice to always invoke transferFrom with msg.sender as the argument? For example, as seen here for Uniswap.

Doesn't this make that approach useless? Because msg.sender would not be the owner of the tokens.

@k06a
Copy link
Contributor Author

k06a commented Feb 19, 2022

@frangio I agree, check my example, it is fully compatible with tranferFrom(msg.sender, …)

@frangio
Copy link
Contributor

frangio commented Feb 20, 2022

Oh I hadn't noticed that. It's a pretty interesting idea, but I don't think I'd feel comfortable recommending it. transferFrom is not the only thing the receiver may use msg.sender for. For example, the auxiliary Caller contract may be granted an LP NFT in return, and some of these assets or rights may not be transferable from the auxiliary contract back to the "real" msg.sender.

@k06a
Copy link
Contributor Author

k06a commented Feb 20, 2022

@frangio exactly, that’s why I use create2 based factory, where salt is equal to user address. This makes users to own this address of msg.sender in the call 😁

@Pandapip1
Copy link
Contributor

Not sure if this issue is still active, but how about https://eips.ethereum.org/EIPS/eip-4524?

@lukehutch
Copy link

lukehutch commented Jun 15, 2022

Beyond all the over-engineering, the biggest issue with ERC777 is that it is insecure as defined, because the sender notification interface is called before state is updated: #3463

@Amxx I agree that ERC1363 is a good standard (simple and secure), as is ERC4524. An implementation of both of these should be in OpenZeppelin, and the two implementations could both share a lot of common code.

Differences: ERC1363 does not allow sending to EOAs or using an EOA as the spender/operator; ERC4524 allows both. ERC1363 allows notification of both spender/operator and recipient; ERC4524 only notifies recipients. Both declare their notification interface(s) via ERC165, which is simpler than ERC1820 used by ERC777.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

ERC1363 does not allow sending to EOAs or using an EOA as the spender/operator

Where do you get that from? AFAIK this is not true.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

ERC1363 allows notification of both spender/operator and recipient;

How is that an issue ?

  • When calling transferAndCall or transferFromAndCall, the recipient is notified. Recipient must implement the ERC1363Receiver interface.
  • When calling approveAndCall, the spender (the one that is approved) is notified. Spender must implement the ERC1363Spender interface.

In both cases, only one address is notified, and that is the address of the entity that could possibly want to react to either receiving tokens, or being approved to transfer tokens.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

the biggest issue with ERC777 is that it is insecure as defined, because the sender notification interface is called before state is updated

This is not necessarily an issue if handled properly. Our implementation of ERC4626 show how being aware of that, we can make sure it doesn't become a liability

@lukehutch
Copy link

ERC1363 does not allow sending to EOAs or using an EOA as the spender/operator

Where do you get that from? AFAIK this is not true.

From the spec (search for "MUST implement"), and from the reference implementation: 1 2.

Maybe you're thinking of using the ERC20 approve/transferFrom methods from an ERC1363-capable token, because ERC1363 is a superset of ERC20. I'm referring to the ERC1363-specific methods, approveAndCall/transferFromAndCall.

ERC1363 allows notification of both spender/operator and recipient;

How is that an issue ?

I wasn't saying it was an issue. I was only pointing out differences between the APIs, to show that there are reasons to implement more than one of these standards or proposals.

One of the primary reasons for the existence of ERC777, ERC1363, ERC4524, etc. is to prevent tokens being sent to contracts that do not know what to do with them, especially non-proxied contracts that cannot be modified (because it causes any received tokens to be lost forever). All of these standards prevent sending to contracts that do not define or declare the right interface. ERC777 and ERC4524 also transparently degrade to a standard transfer when sending to an EOA. ERC1363 doesn't allow that, so it's technically the strongest of all three standards in terms of guaranteeing that you're only sending to the desired address. But being able to send to an EOA using the ERC777 or ERC4524 API, without having to switch back to the ERC20 API, has the benefit of convenience.

Another slightly more esoteric issue with ERC777, by the way, is that if the receiver declares the correct ERC1820 interface but somehow incorrectly defines the receiver notification hook, so that the function signature is wrong, and if that contract defines a fallback function that does not revert, then the ERC777 token contract will think it successfully notified the receiver even though the receiver notification hook never got called. Therefore the fallback function feature of Solidity actually breaks the ERC777 standard, for improperly implemented receivers. ERC1363 and ERC4524 don't have this issue, because they require the hooks they call to return their own function selector as a return value.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

ERC1363 does not allow sending to EOAs or using an EOA as the spender/operator

Where do you get that from? AFAIK this is not true.

From the spec (search for "MUST implement"), and from the reference implementation: 1 2.

Maybe you're thinking of using the ERC20 approve/transferFrom methods from an ERC1363-capable token, because ERC1363 is a superset of ERC20. I'm referring to the ERC1363-specific methods, approveAndCall/transferFromAndCall.

It says that

A contract that wants to accept token payments via transferAndCall or transferFromAndCall MUST implement the following interface:

It also says that

This proposal has been inspired by the ERC-721 onERC721Received and ERC721TokenReceiver behaviours.

My understanding is that, just like in the case of 721, you should be able to transferAndCall (or similar) to an address that has no code ... the call will be successful, and we should accept the empty returndata if the target has no code.

@lukehutch
Copy link

Yes but look at the reference implementation. It cannot send to EOAs, or allow EOAs to act as spender.

Therefore, it's clear that the total omission of any mention of EOAs in ERC1363 actually originates from the author's intent that "A contract that wants to accept" means "A contract (and only a contract) that wants to accept"...

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

One of the primary reasons for the existence of ERC777, ERC1363, ERC4524, etc. is to prevent tokens being sent to contracts that do not know what to do with them

That may be the goal of 4524, but it is not the reason for 777 or 1363. I'm particularly sure of that in the context of 1363. The reason is for the contract to react to a token transfer, which simplifies the "approve + transferFrom" workflow, allow the target to "prepare", to send events, ...

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

Yes but look at the reference implementation

Do you have a link? Its not in the ERC. (Also the reference implementation could technically be wrong, it does not replace the specs)

@lukehutch
Copy link

lukehutch commented Jun 15, 2022

One of the primary reasons for the existence of ERC777, ERC1363, ERC4524, etc. is to prevent tokens being sent to contracts that do not know what to do with them

That may be the goal of 4524, but it is not the reason for 777 or 1363. I'm particularly sure of that in the context of 1363. The reason is for the contract to react to a token transfer, which simplifies the "approve + transferFrom" workflow.

I pointed this out in my comparison of these standards. ERC1363 is the only one that is capable of notifying the spender, and that's quite interesting and unique. But the whole purpose of notifying the recipient originated from the fact that hundreds of millions of dollars have been lost already because of ERC20 tokens being sent to contracts that couldn't handle them.

Do you have a link? Its not in the ERC. (Also the reference implementation could technically be wrong, it does not replace the specs)

I provided the link already. Here it is (here they are) again: 1, 2

@lukehutch
Copy link

lukehutch commented Jun 15, 2022

(Also the reference implementation could technically be wrong, it does not replace the specs)

The specs are simply ambiguous. They do not even address the issue of what happens if an EOA is a recipient or spender.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

That is unfortunately true of many ERC. The fact that it explicitly mentions being inspired by 721 makes me think that the behavior should be the same, but that is just an interpretation.

That should indeed be clarified. Without clarification, I guess any implementation is free to do what they consider best ... (which is not great).
I don't think however that this ambiguity justifies starting another ERC from scratch. ERC4626 is ambiguous ... even ERC20 is ambiguous about some things (events emitted on mint/burn) ... but we manage to live with it.

@Amxx
Copy link
Collaborator

Amxx commented Jun 15, 2022

For the record, we have a PR for adding 1363: #3017

@TradMod
Copy link

TradMod commented Feb 7, 2023

Great Discussion. I just wanted to share a Twitter thread, a good counter to this issue.

The Thread:
https://twitter.com/dmihal/status/1251505373992845317?s=20&t=ziq0zxkVf9IedUodKgRT3w

@frangio
Copy link
Contributor

frangio commented Feb 15, 2023

The thread shows the tokensReceived hook, which is indeed analogous to ETH transfers and equivalent to ERC721 and ERC1155 "onReceived" hooks, and as pointed out it can be easily dealt with. The issue that is not mentioned in the thread is the tokensToSend hook, which can more easily lead to inadvertent checks-effects-interactions violations and reentrancy issues, especially in combination with tokensReceived.

@Amxx
Copy link
Collaborator

Amxx commented Feb 15, 2023

Also the tokensReceived would be great if it did not require ERC1822 registration ...

ERC777 is not as simple as 721 and 1155 were you "just" implement the hook.

@epowell101
Copy link

Asking for a quick pointer - if not erc777 (and I'm convinced) then how could we:

  • have a erc20 token or similar w/ governance & delegation capabilities
  • that we could blacklist addresses

Why? The OpenData Community is starting to curate lists of Sybils (yes imperfect). Nonetheless, the thinking is of a way to prevent known Sybils from gaining access to Sybil lists via the use of our governance token as a future use of the governance token will be to stake or otherwise use it to access updated Sybil lists.

@Pandapip1
Copy link
Contributor

The ERC-20 standard (and even the OZ implementation) is more than capable of doing both of the things you want:

@k06a
Copy link
Contributor Author

k06a commented Jul 22, 2023

@epowell101 I agree with @Pandapip1, existing ERC20 extensions are great way to extend functionality of ERC20 tokens.

I also would recommend taking a look at new Token Plugin ERC20 extension. It allows users to participate in on-chain plugins, which can handle delegations, votes, incentives and any other functionalities: https://github.com/1inch/token-plugins. We are also aiming to push this ERC20Plugins extension into OZ library in coming future.

@freefilm010

This comment was marked as spam.

@Amxx Amxx mentioned this issue Sep 29, 2023
3 tasks
@DaveAppleton
Copy link
Contributor

That may be the goal of 4524, but it is not the reason for 777 or 1363

Stopping tokens from getting trapped in contracts was exactly one of the reasons that ERC777 was adopted - at the time it was presented as a far better thought out standard than the ERC223 which even had bugs in the reference implementation.

@Pandapip1
Copy link
Contributor

FWIW ERC-223 now exists and is Final. I personally prefer ERC-1363 since ERC-223 breaks applications that expect an approve -> transferFrom flow, and because ERC-1363 follows a pattern that is more similar to ERC-721/ERC-1155. ERC-223 is slightly more secure, though.

@lukehutch
Copy link

@Pandapip1 As it is defined in the spec, ERC-223 is not even fully backwards-compatible with ERC-20 (or is not a superset of ERC-20) -- they forgot to include one or two of the ERC-20 functions in the API (I forget which ones).

@Pandapip1
Copy link
Contributor

It wasn't a 'forgot', it was a deliberate design choice not to be compatible. I never claimed that ERC-223 was compatible with ERC-20. In fact, I noted that it wasn't:

ERC-223 breaks applications that expect an approve -> transferFrom flow

ERC-777 is also incompatible with ERC-20. ERC-223 or ERC-1363 + ERC-20 are both generally better than ERC-777.

@lukehutch
Copy link

lukehutch commented Oct 16, 2023

It wasn't a 'forgot', it was a deliberate design choice not to be compatible.

I don't think that's true at all that they didn't forget to add all the ERC20 functions. You can in fact add ERC-223 over the top of ERC20 if you add one or two missing functions, and it does in fact work for both standards if you do that. As somebody who went over the specs with a fine-toothed comb, and created an extremely meticulous rendition of ERC-20, ERC-223, ERC-677, ERC-777, ERC-1363, and ERC-4524 in a single contract, I can tell you that ERC-223 was supposed to be a proper superset of ERC-20. But they missed the mark. The reference implementation didn't even match the ERC-223 spec (and was buggy), the spec left out important details, the spec accidentally dropped these one or two ERC-20 functions without reason or justification (I'm not motivated enough to go back and figure out which ones), and more. ERC-223 is a very amateurish "spec", as far as token ERCs go. I strongly recommend against using it. (N.B. ERC-677 was worse in some ways...)

@Pandapip1
Copy link
Contributor

Pandapip1 commented Oct 17, 2023

@Dexaran, the author of the spec, has spoken and said that it was in fact a deliberate design choice. It's a design decision I don't really like, but that doesn't change the fact that the removal was intentional. It's also not entirely without merit, even though it can and likely will break things.

I didn't say you couldn't implement everything at once. I didn't say the ERC-223 spec was particularly well-written. I do prefer ERC-1363 over ERC-223. But none of this changes the fact that:

  1. Both are valid standards; and
  2. They each have their own tradeoffs

It's up to the OZ maintainers to decide which one to use.


EDIT: Also, we're off-topic here. We should move this to a new issue, and this old one should probably be locked.

@k06a
Copy link
Contributor Author

k06a commented Oct 17, 2023

@Pandapip1 as far as I can remember ERC20 extensions authors criticized ERC20 for a few reasons:

  1. Unavoidability of two separate transactions from EOA for approve and transferFrom tokens.
  2. Ability to send tokens to non-validated receiver, which cause millions loss to Ethereum users (see motivation section of ERC223)

So ERC223 by @Dexaran was probably the very first recognizable attempt to improve ERC20: ERC223 token standard · Issue #223 · ethereum/EIPs

In Jan 2018 @AugustoL introduced ERC827 which improved ERC223 by adding notification calls to all ERC20 functions (approve/transfer/transferFrom) and making such calls indirect via proxy contract: ethereum/EIPs#827

In April 2018 I came up to a solution on how to maintain compatibility with msg.sender “spender” argument of transferFrom call. ERC1003 allowed to reinterpret msg.spender on the flight as previous token interactor: ethereum/EIPs#1003

At the end of April 2018 I came up with feeless (gasless) abstraction to resolve 2-transaction issue: ethereum/EIPs#1035. By the way GSN (Gas station Network) was proposed much later in December 2018 - ethereum/EIPs#1613

In April 2020 I came up to solution how to resolve beneficiaries collision of misused token or proxy contract address as msg.sender inside protocol smart contract. ERC2608 was not properly descibed, but was implemented as CREATE2 factory of proxies per token holder: https://github.com/k06a/ERC2608/blob/master/contracts/ERC2608.sol - it allowed users to reclaim mistakenly lost assets and values from own proxy.

After few years of experience in the DeFi and multiple discussions I still feel that problems of this approach are not resolved and implementations are a bit tricky. I believe ERC2612 (Permit) resolves issue of 2-transactions with EIP712 abstractions in a much better way - clean and scalable solution for replacing transactions/calls with indirect signed actions and merging their execution into single transaction.

Nowadays I would consider whole ERC223-like approach as anti-pattern of smart contract engineering. Moreover I believe including this such functionality in the core part or ERC721 was a mistake. Maybe we can redefine extracted core logic of ERC721 into new ERC****, which will be iconic minimalistic NFT ERC.

@Pandapip1
Copy link
Contributor

FYI to anyone reading this thread - ERC-827, ERC-1003, ERC-1035, and ERC-2608 do not exist. This wasn't always the case, but now, if a standard isn't present on https://eips.ethereum.org/, it isn't an EIP / ERC standard. If you want to create a new standard that you believes solves the relevant problems, I'd recommend you submit a new EIP at https://github.com/ethereum/EIPs.

@k06a
Copy link
Contributor Author

k06a commented Oct 18, 2023

@Pandapip1 I would say they surely exist in the form of discussions, but not properly submitted to the repo.

ERC827 even was part of OpenZeppelin smart contracts library, before it was deprecated.

@Pandapip1
Copy link
Contributor

They are discussions. They are not official standards and therefore generally shouldn't be used. Anyone can submit them to the repo, right now, and start the process of turning them into actual EIPs.

@Dexaran
Copy link

Dexaran commented Oct 18, 2023

@lukehutch

As it is defined in the spec, ERC-223 is not even fully backwards-compatible with ERC-20

I don't think that's true at all that they didn't forget to add all the ERC20 functions.

ERC-223 was designed to be a security patch to insecure ERC-20 standard. I repeat, it was not intended to be a superset or an extension of ERC-20, it was designed to be a SECURITY PATCH. Here it is written
ethereum/EIPs#610 (comment)

There are two main security problems with ERC-20.

Problem 1: placing a burden of making security decisions on a user combined with lack of error handling for erroneous users decisions

There are two methods of transferring ERC-20 tokens: (1) transfer, (2) approve & transferFrom. One is designed to be used for transfers to EOAs another is designed to be used to transfer tokens to smart-contracts. ERC-20 token contract however does not determine the method automatically and enables users to determine it themselves.

If a user will pick a wrong method - the users money gets burned even though it is known for sure that this was a mistake. As of today, at least $201M worth of ERC-20 tokens were lost in this way.

Problem 2: approve & transferFrom is a pull transacting method which was not designed for trustless systems

Here is my detailed article that describes why it is a very bad idea to use pull txs in tokens: https://medium.com/@dexaran820/erc-20-approve-transferfrom-asset-transfer-method-poses-a-threat-to-users-funds-safety-ff7195127018

TL;DR it opens up a wide range of attacks like permit scams or over-authorizing contracts to manipulate users tokens.

There is even more money lost to pull transacting flaws.

It is known that approvals are (1) totally unnecessary if you have 'transaction handling' implemented, (2) gas inefficient and (3) risky for users.

Here is my comment regarding approvals: ethereum/EIPs#20 (comment)

Why ERC-223 is designed as it is

@Pandapip1

@Dexaran, the author of the spec, has spoken and said that it was in fact a deliberate design choice.

If ERC-20 has two problems (placing a burden of making a security decision on a user and pull transacting method) the main goal of ERC-223 was to replace these. The "user-made choices" must be removed - thats why transfer function of ERC-223 automatically determines which transferring method to execute on transfer. Approve & transferFrom are also removed. If someone wants to introduce this functions in their ERC-223 token implementation they can do it but it is not obligatory by the standard because ERC-223 supports transaction handling.

It must be noted that EOS C++ native token is purely based on ERC-223 transferring logic and it works perfectly. Also ERC-721 transferring logic is based on ERC-223 logic and it also works without problems (and there is a reference to ERC-223 in the initial ERC-721 documentation).

Here is a comparison of token standards and transferring methods they support:

Transaction type ERC-223 Ether ERC-20 ERC-721 (NFT) ERC-777 ERC-1155 ERC-1363 EOS C++ token
Push tx + + - + + + + +
Pull tx (risky) - - + + + + + -
Unhandled (insecure) - - + - + - + -

@lukehutch

the spec accidentally dropped these one or two ERC-20 functions without reason or justification

For security reasons. ERC-223 is prioritizing security over backwards compatibility. Any standard that implements transfer function compatible with ERC-20 standard inherits all the security problems and therefore will cause the same financial losses to end users. Quite good reasons to avoid this I think.

ERC-223 is a very amateurish "spec", as far as token ERCs go. I strongly recommend against using it.

If you recommend using specs that are straight up insecure and cause financial losses to users - this is your right. But I recommend prioritizing security in financial systems.

@Dexaran
Copy link

Dexaran commented Oct 18, 2023

@k06a

Nowadays I would consider whole ERC223-like approach as anti-pattern of smart contract engineering.

Only on Ethereum, only among solidity devs for some reason. It is a go-for pattern in security engineering. If it is an anti-pattern in smart-contract engineering then it explains why smart-contracts suffer so much security breaches. Something is wrong with smart-contract engineering patterns it seems. And the author of ERC-20 decided not to use it in his new project because he knows about the problems.

Newer platforms like EOS utilize ERC-223 patterns without problems. And plain ether transfers work as ERC-223. It would be quite logical for developers to implement tokens that work in a similar way as ether (since they anyways need to know how ether transfers work) without developing a whole new transacting method that introduces two security problems.

Not even going deeper into discussing if 200 million dollars lost over 4 years are enough to start thinking that it is a problem.

@k06a
Copy link
Contributor Author

k06a commented Oct 18, 2023

Thanks for the interesting insights @Dexaran. But don’t you think that Ethereum smart contract engineering has the most evolved community and approaches in this industry?

I believe there are a lot of newer blockchains but the most protocol tech innovations happens on Ethereum and Ethereum-like chains, but rarely on non Ethereum-like chains. And OpenZeppelin smart contract library is probably one of the most advanced in industry in terms of wide-spreading the patterns and approaches? That fact that ERC223 and ERC827 were excluded from the library means that approach did not passed natural selection during protocol tech evolution.

@Dexaran
Copy link

Dexaran commented Oct 19, 2023

@k06a

Don’t you think that Ethereum smart contract engineering has the most evolved community and approaches in this industry?

Depends on which industry to benchmark against.

If we compare Ethereum development community to the global programming community - Ethereum is by far not well-developed. We are lacking such basic things as "error handling" in its token standards and "a process to modify final EIPs upon vulnerability disclosures": https://github.com/ethereum-cat-herders/EIPIP/issues/255#issuecomment-1671895165

Like... nobody ever expected that there can be a security vulnerability disclosed AFTER an EIP gets final status? For some reason Dexaran needs to be the first guy to raise this issue and tell the whole Ethereum community that such things can happen https://github.com/ethereum-cat-herders/EIPIP/issues/257#issuecomment-1693740271

This is a clear sign that this development community is not well-developed if such basic well-known widely adopted things are not a standard there. If we compare to wider programming community. If we compare Ethereum community to Callisto community however, Ethereum one can be considered more developed.

I believe there are a lot of newer blockchains but the most protocol tech innovations happens on Ethereum and Ethereum-like chains

Ethereum is a smart-contract platform with the highest network effect. It doesn't automatically mean it is the most technologically advanced one. Similarly Bitcoin is the highest cap crypto today but it doesn't mean it's the most technologically advanced.

Speaking of numbers the performance of EOS VM (WASM) probably 500x higher than EVM https://arxiv.org/pdf/2012.01032.pdf

And EOS has a number of security advantages over Ethereum as well https://www.eosgo.io/blog/shadowed-advantage-of-eos-that-you-might-not-know/

And OpenZeppelin smart contract library is probably one of the most advanced in industry in terms of wide-spreading the patterns and approaches?

May be it's the most advanced in terms of wide-spreading but definitely not in security. The title says the contracts are "secure" but the implementation causes end users to lose money. Personally I wouldn't call something that caused 200 million dollars loss a "secure software".

OZ_not_so_secure_contracts

What is even worse they refuse to fix it #4451 and they don't even update the documentation to add an important security warning so yeah, I'm quite unhappy about OpenZeppelins approach.

They are definitely doing good job providing a lot of code examples.

They are doing bad job not making these code examples secure and refusing to describe caveats that caused millions of dollars losses.

That fact that ERC223 and ERC827 were excluded from the library means that approach did not passed natural selection during protocol tech evolution.

I wrote a sophisticated description of what has gone wrong here #3736 (comment)

Also Ethereum's processes (EIPs and related stuff) are not properly designed to coordinate ecosystem upgrades. As the result the first adopted standard can stay "the main one" for ages even if it has such critical problems as security flaws. Have you ever seen any standard superseding an existing one on Ethereum? Personally I've never seen that. Community tends to stagnate and the current processes are not designed to allow for a healthy upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.