xiaoming90 - Incorrect `supportsInterface` (EIP-165)

[sherlock-admin3]

xiaoming90

medium

Incorrect supportsInterface (EIP-165)

Summary

The implementation of the supportsInterface (EIP-165) within Edition contract is incorrect, potentially leading to a loss of assets, as shown in the scenario below:

A marketplace wants to determine if the Edition contract supports the ERC-2981 (NFT Royalty Standard) or has royalties by calling the Edition.supportsInterface function. Since the Edition.supportsInterface function returns false, the marketplace concluded that the Edition does not support ERC-2981 or does not have royalties. Thus, the marketplace will not collect royalties from the buyers and will not forward the royalties to the creators, leading to a loss of royalty fees for them.

Vulnerability Detail

Per Line 36 below, the Edition contract supports the ERC1155 and ERC2981 interfaces. However, it was found that the Edition.supportsInterface function does not return true for ERC1155 and ERC2981 interfaces when being called, which indicates that it does not support the ERC1155 and ERC2981 interfaces.

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L465

File: Edition.sol
36: contract Edition is IEdition, ERC1155, ERC2981, Initializable, OwnableRoles {
..SNIP..
462:     /// @notice Check if the contract supports the given interface.
463:     /// @param interfaceId The interface ID to check.
464:     /// @return True if the contract supports the interface, false otherwise.
465:     function supportsInterface(bytes4 interfaceId)
466:         public
467:         view
468:         override(IEdition, ERC1155, ERC2981)
469:         returns (bool)
470:     {
471:         return super.supportsInterface(interfaceId);
472:     }

Impact

Consider the following scenario that could potentially lead to a loss of assets due to incorrect supportsInterface function.

A marketplace wants to determine if the Edition contract supports the ERC-2981 (NFT Royalty Standard) or has royalties by calling the Edition.supportsInterface function. Since the Edition.supportsInterface function returns false, the marketplace concluded that the Edition does not support ERC-2981 or does not have royalties. Thus, the marketplace will not collect royalties from the buyers and will not forward the royalties to the creators, leading to a loss of royalty fees for them.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L465

Tool used

Manual Review

Recommendation

Consider updating the supportsInterface function to return true for the interfaces that the contract supports.

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(IEdition, ERC1155, ERC2981)
    returns (bool)
{
-    return super.supportsInterface(interfaceId);
+    return
+        interfaceId == type(IERC1155).interfaceId ||
+        interfaceId == type(IERC2981).interfaceId ||
+        super.supportsInterface(interfaceId);
}

Duplicate of #287

[pqseags]

Will address

[0x73696d616f]

Escalate

This should be invalid/low as it has no impact for the protocol.

[sherlock-admin3]

Escalate

This should be invalid/low as it has no impact for the protocol.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ZdravkoHr]

It has impact depending on external integrations

[0x73696d616f]

And for the protocol?

[ZdravkoHr]

The external integrations may be still part of the protocol team. I mean that they opted-in for EIP1155 compliance, so it's important

[0x73696d616f]

Also, EIP-165 is not in the list.

strict implementation of EIPs 1271 (Graph), 712 (Graph, Edition), 2981 (Edition), 1155 (Edition)

[ZdravkoHr]

EIP-165 is included in EIP1155 Source: https://eips.ethereum.org/EIPS/eip-1155#specification

[realfugazzi]

Agree on the lack of impact. Should be informational.

[bronzepickaxe]

Run this in Edition.t.sol:

    function test_erc2981() public {
      /// bytes4 private constant _INTERFACE_ID_ERC2981 = 0x2a55205a;
      bool res = edition.supportsInterface(0x2a55205a);
      vm.assertEq(res, true);
    }

Output:

[PASS] test_erc2981() (gas: 8692)
Traces:
  [8692] EditionTest::test_erc2981()
    ├─ [480] Edition::supportsInterface(0x2a55205a00000000000000000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← [Return] true
    ├─ [0] VM::assertEq(true, true) [staticcall]
    │   └─ ← [Return]
    └─ ← [Stop]

Contrary to what the report claims, true is returned, not false. This means there is no impact since the impact is built upon the assumption that the ERC2981 interface ID returns false.

[ZdravkoHr]

The probem is that supportsInterface, doesn't work for EIP1155, not EIP2981. You can see #287

[cducrest]

I agree with @ZdravkoHr, the problem lies in non-compliance with EIP1155 and not EIP2981. As such, issues https://github.com/sherlock-audit/2024-04-titles-judging/issues/287, https://github.com/sherlock-audit/2024-04-titles-judging/issues/411, https://github.com/sherlock-audit/2024-04-titles-judging/issues/95 are valid duplicate.

https://github.com/sherlock-audit/2024-04-titles-judging/issues/64 does not properly identify the problem and should be invalid.

This issue improperly identifies the problem as well, but the real problem is still described within this issue so I would leave judges to decide.

The protocol does say that EIP1155 should be strictly implemented (from the Readme of the contest):

Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification?

strict implementation of EIPs 1271 (Graph), 712 (Graph, Edition), 2981 (Edition), 1155 (Edition)

And EIP1155 states that the function supportsInterface(bytes4) must return true for 0xd9b67a26 (from https://eips.ethereum.org/EIPS/eip-1155#specification):

Smart contracts implementing the ERC-1155 standard MUST implement the ERC-165 supportsInterface function and MUST return the constant value true if 0xd9b67a26 is passed through the interfaceID argument.

[sammy-tm]

This should be QA at best

[WangSecurity]

Firstly, the protocol said in the README, they want strict compliance with the EIP1155. Secondly, I believe the protocol shows how this issue can impact external integrations. Hence, planning to reject the escalation and leave the issue as it is.

[bronzepickaxe]

@WangSecurity The impact of this report is wrong, as clearly shown in my comment:

• https://github.com/sherlock-audit/2024-04-titles-judging/issues/274#issuecomment-2106470674

There is no potential loss or w/e as described in this report since true is returned for the ERC2981 interfaceId.

As per strict compliance with EIP1155 as described in the README.md, the severity for that is for you to judge.

[cducrest]

(The problem regarding EIP1155 is valid as stated in my comment https://github.com/sherlock-audit/2024-04-titles-judging/issues/274#issuecomment-2107406647 but https://github.com/sherlock-audit/2024-04-titles-judging/issues/64 and in a certain way this issue do not properly identify the issue as they talk about EIP2981)

[WangSecurity]

@bronzepickaxe I see your comment, but as I understand it's correct about EIP2981, but not EIP1155 which is shown in this report #287 with a POC as well. But if I'm missing, please correct.

[Hash01011122]

This issue suffice medium severity because it was clearly mentioned in README file of contest that protocol must comply with EIP1155, Also I agree with @cducrest has pointed out that #64 is not valid as root cause pointed out by watson was accurate but watson failed to explain the issue thoroughly.

[cducrest]

I would say https://github.com/sherlock-audit/2024-04-titles-judging/issues/64 does not even point to the correct root cause and this issue (https://github.com/sherlock-audit/2024-04-titles-judging/issues/274) fails to explain it thoroughly as it talks about ERC2981 in its impact section and not ERC1155.

[WangSecurity]

Thank you for these insightfule comments. Firstly, I agree that #64 should be invalid as it only talks about ERC2981, when the problem is in ERC1155.

As for this report, I believe it should remain valid cause in the Vulnerability details it explains that the problem is not only in ERC2981, but in the ERC1155 as well. Yes, I see that the impact is only about ERC2981, but the report explains there's a problem with ERC1155, hence, I believe it's sufficient to be a duplicate.

Planning to reject the escalation, but invalidate #64, make #287 the main report and all others duplicates

[jecikpo]

Folks, #64 correctly identifies the issue as the ERC2981 supportsInterface() for 0x2a55205a doesn't return true, while it must. This is a valid issue and is in line with the README requirements that strict ERC2981 must be supported. You are trying to invalidate a valid issue.

[jecikpo]

Naah, you are actually correct. let it be my lesson this time...

[Evert0x]

Result: Medium Duplicate of #287

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: rejected