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

ERC777 mint fails when target is a smart contract not ERC1820 registered #2226

Closed
Amxx opened this issue May 6, 2020 · 2 comments
Closed

Comments

@Amxx
Copy link
Collaborator

Amxx commented May 6, 2020

I noticed this behaviour when working with the pToken contract on ropsten. I'm depositing BTC to the address provided by pToken and their bridge is, in exchange, supposed to deposit pBTC on my account. This operation corresponds to a _mint, by the bridge, of token on my account.

This operation fails when using smart contracts wallet as a target, despite me implementing a non-reverting tokensReceived method on my smart contract. See trace.

This is caused by the _callTokensReceived method, which searches the ERC1820Registry for a ERC777Recipient associated to the account address. In my case the wallet is not registered in the ERC1820Registry so the getInterfaceImplementer call returns address(0). The following check is then a verification that the address is not a (deployed) contract, which then causes the transaction to revert.

Most smart contracts wallets are not ERC1820 registered. My argent wallet is not, and I suspect that most other wallets aren't. Registering all of them would be expensive in terms of gas AND blockchain storage.

A solution would be, whenever the ERC1820Registry doesn't have an implementer address, to call the targeted address directly rather than reverting.

function _callTokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    )
        internal
    {
        address implementer = ERC1820_REGISTRY.getInterfaceImplementer(to, TOKENS_RECIPIENT_INTERFACE_HASH);
        if (implementer != address(0)) {
            IERC777Recipient(implementer).tokensReceived(operator, from, to, amount, userData, operatorData);
        } else if (requireReceptionAck) {
            require(!to.isContract(), "ERC777: token recipient contract has no implementer for ERC777TokensRecipient");
        }
    }

becomes

function _callTokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    )
        internal
    {
        address implementer = ERC1820_REGISTRY.getInterfaceImplementer(to, TOKENS_RECIPIENT_INTERFACE_HASH);
        if (implementer != address(0)) {
            IERC777Recipient(implementer).tokensReceived(operator, from, to, amount, userData, operatorData);
        } else if (requireReceptionAck && to.isContract())
            IERC777Recipient(to).tokensReceived(operator, from, to, amount, userData, operatorData);
        }
    }

This would cause contracts with a non reverting fallback() to accept tokens, but would also give a chance for wallets like argents to be made compatible by "just" adding a staticcall mechanism for the tokensReceived method at the wallet level, without having to go through ERC1820 registration.

Also, I believe this limitation should be more publicly discussed, as it is making some of the largest userbases in the DeFi ecosystem incompatible with the "Money Legos" people are building using ERC777

@nventuro
Copy link
Contributor

nventuro commented May 6, 2020

Hello @Amxx, thanks for discussing this issue!

That check is very much intentional and a non-optional part of the ERC777 specification: not having it would make the token contract non-compliant.

Quoting from the spec:

Nonetheless, the rules below MUST be respected when minting for a recipient:

The token contract MUST call the tokensReceived hook of the recipient if the recipient registers an ERC777TokensRecipient implementation via ERC1820.

The token contract MUST revert when minting in any of the following cases:

The recipient is a contract, and it does not implement the ERC777TokensRecipient interface via ERC1820.

This is what _callTokensReceived implements: it calls tokensReceived if there is an implementation registered, reverting if there isn't one and the recipient is a contract.

This is a similar mechanism to the one used by ERC721 in safeTransferFrom, and serves as a way to prevent tokens from being locked in a contract. Any smart contracts that interact with ERC777 (including the Argent wallet) need to signal this by registering an implementation.

@Amxx
Copy link
Collaborator Author

Amxx commented May 7, 2020

Most smart contract wallets, Argent included, implement onERC721Received, and that's all they need to do. However, to the best of my knowledge, most provider don't do the 1820 registration of 777. I know for a fact that Argent doesn't. I get that 777 specs is different then 721 in the sens that it requires 1820 registration (where 721 just asks the target) ... I get its in the spec, but I can't figure out why.
This as the direct consequence that projects built on top of ERC777 are NOT compatible with these wallets out of the box, and wallet don't seem to consider this registration as a priority.

Some people might think this situation is ok, I strongly believe it is NOT. We are talking money legos, but this incompatibility, which is not known by most users, caused me too burn (testnet) BTC, because some project commited to ERC777, and my wallet was not doing the registration. Not fixing that will have bad consequences both for the ERC777 based project and for the smart contract wallets as users will not be ok with losing funds due to over-conservative standards.

ERC777 is already disliked by many people after the latest "hacks" caused by the reentry ERC777 enabled (even though 777 is not strictly to blame) ... if you care about 777 you should really make sure you don't give more opportunity for people to blame losing money on it.

@Amxx Amxx closed this as completed Jan 6, 2021
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

No branches or pull requests

2 participants