Ocean.sol is not compliant with ERC165 standard as stated in the contest page

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/OceanERC1155.sol#L393-L395 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L79 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/lib/forge-std/src/interfaces/IERC721.sol#L104

Vulnerability details

Impact

Contracts checking for the IERC721TokenReceiver interface with the supportsInterface() function will get a false return value.

Proof of Concept

On the contest page it is written that: "The Ocean should conform to all standards that its code claims to (ERC-1155, ERC-165)"

The ERC165 is a standard method to publish and detect what interfaces a smart contract implement according to the specification here: https://eips.ethereum.org/EIPS/eip-165

The Ocean.sol smart contract implements the IERC721TokenReceiver interface but does not indicate that with the supportsInterface() function.

As you can see in the comment of the code below: the ERC-165 identifier for the IERC721TokenReceiver interface is 0x150b7a02

/// @dev Note: the ERC-165 identifier for this interface is 0x150b7a02.
interface IERC721TokenReceiver {
    /// @notice Handle the receipt of an NFT
    /// @dev The ERC721 smart contract calls this function on the recipient
    /// after a `transfer`. This function MAY throw to revert and reject the
    /// transfer. Return of other than the magic value MUST result in the
    /// transaction being reverted.
    /// Note: the contract address is always the message sender.
    /// @param _operator The address which called `safeTransferFrom` function
    /// @param _from The address which previously owned the token
    /// @param _tokenId The NFT identifier which is being transferred
    /// @param _data Additional data with no specified format
    /// @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
    ///  unless throwing
    function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes calldata _data)
        external
        returns (bytes4);
}

contract Ocean is IOceanInteractions, IOceanFeeChange, OceanERC1155, IERC721Receiver, IERC1155Receiver {

Tools Used

EIP165 specification: https://eips.ethereum.org/EIPS/eip-165

Recommended Mitigation Steps

Consider adding the check for type(IERC721Receiver).interfaceId in the supportsInterface function like this.

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

Assessed type

Other

[raymondfam]

Checks is in place in the parental OceanERC1155.sol of Ocean.sol:

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/OceanERC1155.sol#L89-L95

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[0xA5DF]

I think the warden is right about this one, the function implements IERC721TokenReceiver but this isn't reflected in the supportInterface() function. I'd like to get your comment on this please, @viraj124

[viraj124]

I think we can mark this as low or qa since at this point we don't have any erc721 adapters which would need to integrate with the ocean

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[c4-sponsor]

viraj124 (sponsor) acknowledged

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

There's an open org issue regarding ERC165, I tend to agree this should be low. Marking this as low, unless any additional impact is proven.

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Low quantity