`AmbireAccount.supportsInterface` function breaks EIP-721 specification for supporting `ERC721TokenReceiver` for a wallet application that accepts safe transfers of ERC721 tokens

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/ad7d99b2b30b6d79959b6767da933bf01c58ade7/contracts/AmbireAccount.sol#L252-L256

Vulnerability details

Impact

The AmbireAccount contract does not comply with the EIP-721 specification for supporting ERC721TokenReceiver for a wallet application that accepts safe transfers of ERC721 tokens while the competitor wallet product does comply with. This would cause other applications that expect such compliance to have integration issues with the AmbireAccount contract while not having such issues with the competitor product.

Proof of Concept

https://eips.ethereum.org/EIPS/eip-721#specification states that "a wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers" and "the ERC-165 identifier for this interface is 0x150b7a02". As shown below, Safe that is similar to Ambire Wallet does execute interfaceId == type(ERC721TokenReceiver).interfaceId in its TokenCallbackHandler.supportsInterface function.

https://github.com/safe-global/safe-contracts/blob/main/contracts/handler/TokenCallbackHandler.sol#L57-L62

    function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
        return
            interfaceId == type(ERC1155TokenReceiver).interfaceId ||
            interfaceId == type(ERC721TokenReceiver).interfaceId ||
            interfaceId == type(IERC165).interfaceId;
    }

However, the following AmbireAccount.supportsInterface function does not execute interfaceID == 0x150b7a02, which breaks the EIP-721 specification for supporting ERC721TokenReceiver for a wallet application that accepts safe transfers of ERC721 tokens.

https://github.com/AmbireTech/ambire-common/blob/ad7d99b2b30b6d79959b6767da933bf01c58ade7/contracts/AmbireAccount.sol#L252-L256

    function supportsInterface(bytes4 interfaceID) external pure returns (bool) {
        return
            interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).
            interfaceID == 0x4e2312e0; // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/AmbireTech/ambire-common/blob/ad7d99b2b30b6d79959b6767da933bf01c58ade7/contracts/AmbireAccount.sol#L252-L256 can be updated to the following code.

    function supportsInterface(bytes4 interfaceID) external pure returns (bool) {
        return
            interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).
                        interfaceID == 0x150b7a02 ||
            interfaceID == 0x4e2312e0; // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).
    }

Assessed type

Other

[Picodes]

When checking the EIPs, for 721: A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

And for 1155: Smart contracts MUST implement all of the functions in the ERC1155TokenReceiver interface to accept transfers. See “Safe Transfer Rules” for further detail. Smart contracts MUST implement the ERC-165 supportsInterface function and signify support for the ERC1155TokenReceiver interface to accept transfers. See “ERC1155TokenReceiver ERC-165 rules” for further detail.

So it seems that strictly speaking ERC-165 support is required only for 1155, in which case this would be of QA severity

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[rbserver]

Hi @Picodes,

Since https://eips.ethereum.org/EIPS/eip-721#specification mentions that a wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers, which is then followed by /// @dev Note: the ERC-165 identifier for this interface is 0x150b7a02, my understanding regarding this specification is that a wallet application that accepts safe transfers of ERC721 tokens must additionally check interfaceID == 0x150b7a02 in the supportsInterface function though other applications that are not this kind of wallet application do not need to provide such ERC721TokenReceiver support. In this case, Ambire Wallet is this kind of wallet application.

Moreover, not just Safe, Sequence also includes the additional check for supporting ERC721TokenReceiver in the supportsInterface function as shown below. Hence, it appears to be that multiple competitors of Ambire Wallet comply with the EIP-721 specification for supporting ERC721TokenReceiver as this kind of wallet application while Ambire Wallet currently does not.

https://github.com/0xsequence/wallet-contracts/blob/master/src/contracts/modules/commons/ModuleHooks.sol#L135

  function supportsInterface(bytes4 _interfaceID) public override virtual pure returns (bool) {
    if (
      _interfaceID == type(IModuleHooks).interfaceId ||
      _interfaceID == type(IERC1155Receiver).interfaceId ||
      _interfaceID == type(IERC721Receiver).interfaceId ||
      _interfaceID == type(IERC223Receiver).interfaceId
    ) {
      return true;
    }

In the previous contests, I have seen non-compliances to EIP standards as medium risks. Hence, I would like to ask if you can re-evaluate this report to see if the current implementation of the supportsInterface function breaks the EIP-721 specification for this kind of wallet application or not and if this issue can be considered as a medium risk or not.

Thanks!

[Picodes]

The argument about competitors is interesting, but still, strictly speaking, I believe the ERC165 support is not required by the EIP721. The @dev comment you're referring to doesn't say that a wallet must implement the ERC165, it only gives the identifier. For example, openzeppelin didn't implement it. See:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/utils/ERC721Holder.sol versus https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/utils/ERC1155Receiver.sol