Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZdravkoHr. - Edition.supportsInterface is not EIP1155 compliant #287

Open
sherlock-admin4 opened this issue Apr 26, 2024 · 2 comments
Open
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin4
Copy link
Contributor

sherlock-admin4 commented Apr 26, 2024

ZdravkoHr.

medium

Edition.supportsInterface is not EIP1155 compliant

Summary

According to the ERC-1155 specification, the smart contracts that are implementing it MUST have a supportsInferface(bytes4) function that returns true for values 0xd9b67a26 and 0x0e89341c. The current implementation of Edition.sol will return false for both these values.

Vulnerability Detail

The contract inherits from ERC1155 and ERC2981.

contract Edition is IEdition, ERC1155, ERC2981, Initializable, OwnableRoles

The supportsInterface() function of Edition returns the result of executing super.supportsInterface()

    function supportsInterface(bytes4 interfaceId)
        public
        view
        override(IEdition, ERC1155, ERC2981)
        returns (bool)
    {
        return super.supportsInterface(interfaceId);
    }

Since both ERC1155 and ERC2981 implement that function and ERC2981 is the more derived contract of the two, Edition.supportsInterface() will end up executing only ERC2981.supportsInterface().

Impact

Medium. The contract is to be a strict implementation of ERC1155, but it does not implement the mandatory ERC1155.supportsInterface() function.

Code Snippet

PoC for Edition.t.sol

    function test_interface() public {
        assertFalse(edition.supportsInterface(bytes4(0xd9b67a26)));
        assertFalse(edition.supportsInterface(bytes4(0x0e89341c)));
    }

Tool used

Foundry

Recommendation

Instead of relying on super, return the union of ERC1155.supportsInterface(interfaceId) and ERC2981.supportsInterface(interfaceId).

    function supportsInterface(bytes4 interfaceId)
        public
        view
        override(IEdition, ERC1155, ERC2981)
        returns (bool)
    {
-       return super.supportsInterface(interfaceId);
+       return ERC1155.supportsInterface(interfaceId) || ERC2981.supportsInterface(interfaceId);
    }
@sherlock-admin3 sherlock-admin3 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Apr 29, 2024
@github-actions github-actions bot closed this as completed May 6, 2024
@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels May 6, 2024
@sherlock-admin2 sherlock-admin2 changed the title Precise Carmine Carp - Edition.supportsInterface is not EIP1155 compliant ZdravkoHr. - Edition.supportsInterface is not EIP1155 compliant May 12, 2024
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label May 12, 2024
@WangSecurity WangSecurity added Has Duplicates A valid issue with 1+ other issues describing the same vulnerability and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels May 22, 2024
@WangSecurity WangSecurity reopened this May 26, 2024
@sherlock-admin2
Copy link

The protocol team fixed this issue in the following PRs/commits:
https://github.com/titlesnyc/wallflower-contract-v2/pull/1

@sherlock-admin2
Copy link

The Lead Senior Watson signed off on the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants