ERC-165 is not implemented correctly in CouncilMember

Summary

ERC-165 is implemented incorrectly, since rather than call the supportsInterface methods on the parent classes, it directly checks for either of their interfaceIds.

Vulnerability Detail

ERC-165 specifies a supportsInterface method to check whether a contract implements a known standard interface using a bytes4 value corresponding to a known interface. Commonly used interfaces such as ERC721 and AccessControl are examples of contracts often used with ERC-165. The implementation of supportsInterface in CouncilMember only checks for the interface IDs of AccessControlEnumerableUpgradeable and ERC721EnumerableUpgradeable and does not call the methods in the parents.

Impact

The interfaces checked for are not interface IDs commonly used with ERC-165 and the function will return false for more commonly used implemented interfaces, which would identified with the supportsInterface methods in AccessControlEnumerableUpgradeable and ERC721EnumerableUpgradeable.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L146-L161

function supportsInterface(
        bytes4 interfaceId
    )
        public
        pure
        override(
            AccessControlEnumerableUpgradeable,
            ERC721EnumerableUpgradeable
        )
        returns (bool)
    {
        return
            interfaceId ==
            type(AccessControlEnumerableUpgradeable).interfaceId ||
            interfaceId == type(ERC721EnumerableUpgradeable).interfaceId;
    }

Tool used

Manual Review

Recommendation

Instead of checking for the two interfaceIds of the parent contracts, compare the return values of their supportsInterface methods. This will check for any well known ERC-165 implementations used in the base classes. The visibility has been changed from pure to view, to match the [ERC-165 specification](https://eips.ethereum.org/EIPS/eip-165).

function supportsInterface( bytes4 interfaceId )
        public
        view 
        override( AccessControlEnumerableUpgradeable,
            ERC721EnumerableUpgradeable )
        returns (bool) {
        return
            AccessControlEnumerableUpgradeable.supportsInterface(interfaceId) ||
            ERC721EnumerableUpgradeable.supportsInterface(interfaceId);
    }

Replace https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/test/sablier/CouncilMember.test.ts#L72-L79 with the following tests, checking for known used ERC-165 interfaceIds and for an unimplemented interfaceId.

it("has IAccessControl interface", async () => {
    expect(await councilMember.supportsInterface('0x7965db0b')).to.equal(true);
});

it("has IERC721 interface", async () => {
    expect(await councilMember.supportsInterface('0x80ac58cd')).to.equal(true);
});

it("doesn't have unsupported interface", async () => {
    expect(await councilMember.supportsInterface('0xdeadbeef')).to.equal(false);
});

Duplicate of #108

sherlock-audit / 2024-01-telcoin-judging

m4ttm - ERC-165 is not implemented correctly in CouncilMember #224