Deprecate ERC777 implementation

k06a commented 3 years ago

🧐 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:

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.

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 {
    // ...
}

TradMod commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

https://docs.openzeppelin.com/contracts/4.x/governance#erc20votes_erc20votescomp
https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20-_beforeTokenTransfer-address-address-uint256- (using it to revert if an address is on the blacklist)

k06a commented 1 year ago

@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 commented 1 year ago

wouldn't that be 1820? sorry to jump in. I'm learning and what you all do is nit easy at all..

erc 20 is token standard as is 721...

777 goes backwards with the erc20 and fixes the double approval issue into one single action. technically better than erc 20... oh yeah it can pay out to two transactions with one action. thus fir commission and such its paid same time and also doest get stuck like erc20... I probably sound like an idiot but I know I am close..

I was reading through all the different final token types. so many awesome ones like the token backed by hard assets. then there is royalty assets.

long story short I am working on a global project ND have also filed for grants, and will be fundraising, crowdfunding, for this global collaboration. interestingly open zeppelin falls inline with my preferred selection to contract all the services of issuing contracts, monitoring, backtesting, auditing, security, to all relating to mining and minting for the project. also a research amd development fund to collaborate with a few special groups to see if we can create a newer ecosystem. blockchain development from start to living adaptable concept...

I like and need to break down every token type to stretch what is and is not feasible .. when applied to a proprietary model that I can present to the group once proper docs are signed by all parties. I feel there is a special element to be added and timing is critical as the first to market usually is top product.

I plan to reach out to HQ in NYC on the coming days. like to speak with a contracting specialist, project manager, lead blochchain developer.

if some one could point me to a well rounded individual that can help establish the requirements and services into a quotable schedule of prices, swiftly as I have a collaboration of about 10 entities (should have half that when completed), 7 contents, and very complicated corporate structure to keep all parties humble.

d.rey 202.830.5331

On Sat, Jul 22, 2023, 1:52 PM Anton Bukov @.***> wrote:

@epowell101 https://github.com/epowell101 I agree with @Pandapip1 https://github.com/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.

— Reply to this email directly, view it on GitHub https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2620#issuecomment-1646638772, or unsubscribe https://github.com/notifications/unsubscribe-auth/BA6IQJ7WYXW22IAHNY4OPOTXRQHOZANCNFSM4Z72GWVQ . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

DaveAppleton commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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:

Both are valid standards; and
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 commented 1 year ago

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

Unavoidability of two separate transactions from EOA for approve and transferFrom tokens.
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: https://github.com/ethereum/EIPs/issues/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: https://github.com/ethereum/EIPs/issues/1003

At the end of April 2018 I came up with feeless (gasless) abstraction to resolve 2-transaction issue: https://github.com/ethereum/EIPs/issues/1035. By the way GSN (Gas station Network) was proposed much later in December 2018 - https://github.com/ethereum/EIPs/pull/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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 https://github.com/ethereum/EIPs/pull/610#issuecomment-296711733

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.

For example this accident: https://www.reddit.com/r/0xProject/comments/7lpc5n/ive_sent_my_tokens_to_the_zrx_smart_contract/
And this accident: https://beincrypto.com/1-million-usdt-nearly-lost-to-simple-user-error/
And this accident: https://www.reddit.com/r/ethereum/comments/16bqvj8/4_hours_ago_240k_worth_of_cro_were_deposited_to/

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.

For example this accident: https://twitter.com/realScamSniffer/status/1686761980365832197
And this accident: https://twitter.com/realScamSniffer/status/1710423552380502331
And this accident: https://twitter.com/realScamSniffer/status/1686161606328401923

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: https://github.com/ethereum/EIPs/issues/20#issuecomment-289227754

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/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 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3736#issuecomment-1648404823

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.

OpenZeppelin / openzeppelin-contracts