Deprecate ERC777 implementation

[k06a]

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

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]

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]

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

[frangio]

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]

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]

@phbdias sure, check out ERC20Permit.sol

[Amxx]

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

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]

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

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]

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]

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

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

[frangio]

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]

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