DelegateToken is not EIP-721 compliant

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L65-L71 https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L82-L86

Vulnerability details

Impact

DelegateToken is not EIP-721 compliant: this makes integrations with other contracts harder (if not impossible), as they expect to interact with a fully compliant contract.

Proof of Concept

DelegateToken is a ERC721TokenReceiver:

/// @inheritdoc IERC721Receiver
function onERC721Received(address operator, address, uint256, bytes calldata) external view returns (bytes4) {
    if (address(this) == operator) return IERC721Receiver.onERC721Received.selector;
    revert Errors.InvalidERC721TransferOperator();
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L82-L86

According to the official EIP-721, the contract should also fully implement the EIP-165 interface:

Every ERC-721 compliant contract must implement the ERC721 and ERC165 interfaces

However, the supported interfaces are missing the ERC721TokenReceiver ID:

function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
    return interfaceId == 0x2a55205a // ERC165 Interface ID for ERC2981
        || interfaceId == 0x01ffc9a7 // ERC165 Interface ID for ERC165
        || interfaceId == 0x80ac58cd // ERC165 Interface ID for ERC721
        || interfaceId == 0x5b5e139f // ERC165 Interface ID for ERC721Metadata
        || interfaceId == 0x4e2312e0; // ERC165 Interface ID for ERC1155 Token receiver
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L65-L71

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the missing interface ID:

    function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
        return interfaceId == 0x2a55205a // ERC165 Interface ID for ERC2981
            || interfaceId == 0x01ffc9a7 // ERC165 Interface ID for ERC165
            || interfaceId == 0x80ac58cd // ERC165 Interface ID for ERC721
            || interfaceId == 0x5b5e139f // ERC165 Interface ID for ERC721Metadata
            || interfaceId == 0x4e2312e0; // ERC165 Interface ID for ERC1155 Token receiver
+           || interfaceId == 0x150b7a02; // ERC165 Interface ID for ERC721 Token receiver
    }

Assessed type

ERC721

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Diff -> Primary

[c4-sponsor]

0xfoobar marked the issue as disagree with severity

[0xfoobar]

Will fix but this is QA not medium.

[c4-sponsor]

0xfoobar (sponsor) confirmed

[GalloDaSballo]

Downgrading to QA as best practice, that won't cause a loss of funds

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[DadeKuma]

@GalloDaSballo C4 guidelines don't require a direct loss of funds for a valid medium:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#estimating-risk

This issue can definitely impact the protocol, as it can cause interoperability issues between contracts if they rely on EIP-165: if that's the case, those contracts wouldn't be able to use Delegate.

Other similar findings as an example: https://github.com/code-423n4/2023-06-lukso-findings/issues/122

[0xfoobar]

Neither the protocol nor its availability are impacted here. No value is leaked. Appreciate you quoting the standards, but the standards don't support your value judgments.

[DadeKuma]

I respectfully disagree with the sponsor, for the following reasons:

Neither the protocol nor its availability are impacted here

The protocol will work fine in isolation. However, if an external contract relies on EIP-165 to check if said functionality is supported (which is the purpose of EIP-165), it does impact the availability. The supportsInterface will return false, and thus, it won't be possible to use it.

I won't dispute further, and leave this as the judge's decision.

[GalloDaSballo]

I'm unable to verify the Lukso finding since both code and documentation have matching IDs

Quoting the rules, the availability of the contracts in-scope wouldn't be impacted, it would be the functionality of an external system that requires token receivers to explicitly state their IDs instead of simply implementing the callback, that would be impacted

Moreover the contract in scope uses safeTransfer as a way to receive callbacks

The main rationalization for the lukso finding is that the code was trying to implement a spec they wrote

The code in scope simply wants a callback from the transfer and the one available is from safeTransfer

More specifically the code in scope doesn't want to receive any tokens, it only wants to receive specific tokens

[GalloDaSballo]

After talking to two additional judges, we agree that the main rationale for QA vs Med is that the reliance on the interface is hypothetical

In order for a contract to be EIP721 they must implement multiple interfaces

The spec states that to be a compliant recipient A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers. the interface being the function onERC721Received

Whereas signaling the implementation of the interface via ERC-165 is not mandatory