`Ocean.supportsInterface` LACKS SUPPORT FOR THE `IERC721Receiver` INTERFACE THUS LIMIITING THE OPERATIONS OF `_erc721Wrap` AND `_erc721Unwrap` FUNCTIONS AND BREAKING THE EXPECTED BEHAVIOUR OF THE PROTOCOL

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L305-L307

Vulnerability details

Impact

The Ocean.sol should be able to interact with the ERC721 tokens since the _erc721Wrap and _erc721Unwrap functions are implemented to wrap and unwrap the ERC721 tokens to OceanID and vice versa.

But the Ocean.supportsInterface does not indicate the Ocean contract supports the IERC721Receiver interface as shown below:

function supportsInterface(bytes4 interfaceId) public view virtual override(OceanERC1155, IERC165) returns (bool) {
        return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId);
    } 

For any users or tools which use the Ocean.supportsInterface function to verify the Ocean contract supports receiving the ERC721 tokens will not interact with the Ocean contract since Ocean.supportsInterface function will return false for the interfaceId = type(IERC721Receiver).interfaceId query.

This limits and deviates from the expected behaviour of the Ocean protocol.

Proof of Concept

    function supportsInterface(bytes4 interfaceId) public view virtual override(OceanERC1155, IERC165) returns (bool) {
        return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId);
    }

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L305-L307

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the Ocean.supportsInterface as shown below by adding the IERC721Receiver interface to indicate the Ocean protocol supports receiving the ERC721 tokens so the _erc721Wrap and _erc721Unwrap functions can operate as expected.

function supportsInterface(bytes4 interfaceId) public view virtual override(OceanERC1155, IERC165) returns (bool) {
        return interfaceId == type(IERC1155Receiver).interfaceId || interfaceId == type(IERC721Receiver).interfaceId || super.supportsInterface(interfaceId);
    } 

Assessed type

ERC721

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #218

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Moved to #277

[c4-judge]

0xA5DF marked the issue as grade-c

[udsene]

Hi @0xA5DF,

Thank you for judging this contest!

I think issue #271 and #218 are eligible to be considered as a valid medium for the following reasons.

1. In the contest documentation states that strict conformity to the ERC165 is a must, since it is mentioned under the invariants of the Ocean protocol: It further states that this invariant should never be violated under any circumstances as shown below:

Invariants The following Ocean invariants should never be violated under any circumstances:

The Ocean should conform to all standards that its code claims to (ERC-1155, ERC-165) EXCEPTION: The Ocean omits the safeTransfer callback during the mint that occurs after a ComputeInput/ComputeOutput. The contract receiving the transfer was called just before the mint, and should revert the transaction if it does not want to receive the token.

Issue #271 clearly shows why the Ocean protocol does not conform to ERC165 since it does not indicate it supports IERC721Receiver interface even after implementing the IERC721 interface in the Ocean contract. This clearly falls under not conforming to the ERC165 thus breaks the invariant set by the Ocean protocol for this contest.

2. Even if there are no erc721 adapters at the moment as the sponsor has stated in issue #218 , the invariant states that the conformity to ERC165 should not be violated under any circumstances.

3. Even if there are no erc721 adapters at the moment there is a high possibility of them being added in the future and that is why the devs have included the logic to support ERC721 in the Ocean contract.

4. Since the Ocean.supportsInterface does not indicate that Ocean contract supports the IERC721Receiver interface, and Once the erc721 adapters are added to the Ocean protocol, any external protocols or external contracts using the supportsInterface will be returned false for their query on IERC721Receiver support. This will make it impossible for the external protocols to work with Ocean protocol using the erc721 adapters thus making the Ocean non-compatible with external protocols (using ERC721 transfers) through the use of erc721 adapters.

5. Warden was not informed of erc721 adapters currently not being added to the Ocean protocol during the contest period and had audited the contest based on the proivded details to him during the contest period.

Thank You

[0xA5DF]

Hey Regarding the invariant - I don't see how this is relevant to severity. The fact that an invariant must not be broken doesn't mean that any break of the invariant is a med severity issue.

This will make it impossible for the external protocols to work with Ocean protocol using the erc721 adapters

Can you give me an example of an external protocol that wouldn't work with Ocean because of this?

Warden was not informed of erc721 adapters currently not being added to the Ocean protocol during the contest period and had audited the contest based on the proivded details to him during the contest period.

I don't see how this is relevant to this case.

[udsene]

Hi @0xA5DF,

Thank you for your reply on the previous query. Please consider the following scenario:

As it is stated in the shell protocol document, the Ocean is expected to be compatible with external protocols through the use of adapter primitives.

The goal of Shell v3 is to make the Ocean compatible with external protocols through the use of adapter primitives.

Since conformity of ERC165 is an invariant of the Ocean protocol, the external contract (User) would call on the Ocean.supportInterface() function to verify Ocean Contract Implements a certain Interface. Now Let's consider that the Ocean protocol decides to work with NFT market place in the future. Accordingly ERC721 adapter is implemented to make the Ocean compatible with the NFT market place. The users who are going to interact with the NFT market place via the Ocean protocol will initially check whether Ocean protocol can accept the ERC721 tokens by calling the Ocean.supportsInterface and checking whether IERC721Receiver interface is implemented.

Even though the Ocean protocol does implement the IERC721Receiver interface and has the onERC721Received function with the correct return value, the Ocean.supportsInterface will return false for the query on the IERC721Receiver interface Id (Since IERC721Reciever interfaceID is not included in the support interface).

Hence the external contracts which are programmed to check for the conformity of the ERC165 before interacting with the Ocean will not properly work with ERC721 token transfers due to false return value.

Hence the Ocean protocol will not be able to function properly with the NFT marketplace via the ERC721 adapter since most of the user requests are dropped during the supportInterface check for IERC721Receiver interfaceID.

There is a high possibility for the users to check the Ocean.supportInterface to check for a particular interface implementation, since the conformity to ERC165 is an invariant in the Ocean protocol.

As a result the Ocean protocol will not be able to function properly with the ERC721 protocols such as NFT market places via ERC721 adapters.

Thank You

[0xA5DF]

Again, if there are any external contracts/projects that wouldn't work with Ocean because of this - please list those projects.

As for users - they can read the project docs and UI, so what the function supportsInterface() returns is insignificant for them.