safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.84k stars 907 forks source link

ERC777 Token Compatibility for TokenCallbackHandler #655

Open QYuQianchen opened 1 year ago

QYuQianchen commented 1 year ago

Context / issue

A Safe instance can configure one fallback hander on deployment or later by calling the setFallbackHandler(address) function by itself. Among all the cool fallback handlers developed by the Safe team, the TokenCallbackHandler stands out for its usefulness when dealing with token callbacks.

However, the current implementation of "TokenCallbackHandler" falls short when handling ERC777 token transactions, particularly mint, send and operatorSend calls.

In a scenario where a Safe sets "TokenCallbackHandler" as its fallback handler and receives ERC777 tokens through the functions mentioned above, it queries the ERC1820Registry contract to retrieve the implementer contract (see OpenZeppelin's ERC777 Implementation v4.9). If the Safe hasn't previously called setInterfaceImplementer on the ERC1820 contract, the returned implementer will be address(0), resulting in a revert of the ERC777 token transaction with the error message "ERC777: token recipient contract has no implementer for ERC777TokensRecipient."

Proposed solution

  1. Implement IERC1820Implementer in the TokenCallbackHandler contract.

    contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ERC721TokenReceiver, IERC165, IERC1820Implementer {
    // ... other variables
    bytes32 private constant TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");
    bytes32 private constant ERC1820_ACCEPT_MAGIC = keccak256("ERC1820_ACCEPT_MAGIC");
    // ... other functions
    /**
     * override implementation check
     */
    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account)
        public
        view
        virtual
        override
        returns (bytes32)
    {
        return interfaceHash == TOKENS_RECIPIENT_INTERFACE_HASH ? ERC1820_ACCEPT_MAGIC : bytes32(0x00);
    }
    }
  2. Optionally, make an additional setInterfaceImplementer(safe_instance_address, TOKENS_RECIPIENT_INTERFACE_HASH, TokenCallbackHandler_deployment) call to the ERC1820Reigstry, when "TokenCallbackHandler" is set by internalSetFallbackHandler(). This step can also be left for Safe owners to decide to execute in a separate Safe transaction or not. However, it should be properly documented.

mmv08 commented 1 year ago

It seems that openzeppelin deprecated ERC777 in their latest release: https://docs.openzeppelin.com/contracts/4.x/erc777 (top of the page)

We haven't previously received any requests for adding additional token handlers. Do you have any data about erc777 usage?

SCBuergel commented 1 year ago

Given that ERC777 has been within OZ releases for quite some time, it's safe to assume that many projects do use them. We (HOPR) did deploy $HOPR as an ERC777 and our token holders would significantly benefit from having this standard properly supported within Safe out of the box.

QYuQianchen commented 1 year ago

Hey @mmv08! Indeed, it's accurate that OpenZeppelin (OZ) has deprecated ERC777, and it will be removed in the v5.0 release. However, it's worth noting that OZ has been supporting ERC777 for over four years, starting from v2.3.0. You can refer to their CHANGELOG for details.

There's still a substantial number of ERC777 tokens deployed on various EVM chains. A quick query on Dune here shows that there are at least 360 ERC777 tokens on the mainnet, around 250 on Polygon, and approximately 200 on Gnosis, see result here or below

deployed number of erc777 tokens

Moreover, I'd like to kindly emphasize that the goal isn't to add more token handlers but to address an issue within the existing code. Under the current implementation, for a Safe proxy to utilize ERC777TokensRecipient, it must undergo two transactions:

  1. Set "TokenCallbackHandler" as its callback handler
  2. Call setInterfaceImplementer to set the proxy itself as the interface implementer.

Additionally, it's worth noting that the proposed solution streamlines this process to just one step: calling setInterfaceImplementerto designate the "TokenCallbackHandler" deployment as the interface implementer.

Furthermore, the proposed solution offers the advantage of saving gas when receiving an ERC777 token upon minting, as it eliminates the need for the additional calldata and CALL operations involved in fallback().

KapeR221 commented 1 year ago

great