[Lines of code](https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol#L480-L496) # Vulnerability details ## Bug Description Throughout the codebase, the protocol uses the supportsERC165InterfaceUnchecked() function from Openzeppelin's [ERC165Checker.sol]() to check for the support of ERC-165 interface IDs. However, supportsERC165InterfaceUnchecked() only checks if the call to supportsInterface() has a non-zero return value: [ERC165Checker.sol#L116-L122]() assembly { success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20) returnSize := returndatasize() returnValue := mload(0x00) } return success && returnSize >= 0x20 && returnValue > 0; This might cause supportsERC165InterfaceUnchecked() to incorrectly return true for contracts that meet one of the following criteria: * Has a function with the selector 0x01ffc9a7, which clashes with supportsInterface(bytes4). This function must have a non-zero return value. * Contains a fallback function that returns a non-zero value. In the protocol, supportsERC165InterfaceUnchecked() is mainly used to check if an address is compatible with LSP-1 before calling its universalReceiver() function. For example, _notifyTokenReceiver() in LSP7DigitalAssetCore.sol ensures that the address receiving tokens is capable of handling them: [LSP7DigitalAssetCore.sol#L480-L496]() if ( ERC165Checker.supportsERC165InterfaceUnchecked( to, _INTERFACEID_LSP1 ) ) { ILSP1UniversalReceiver(to).universalReceiver( _TYPEID_LSP7_TOKENSRECIPIENT, lsp1Data ); } else if (!allowNonLSP1Recipient) { if (to.code.length > 0) { revert LSP7NotifyTokenReceiverContractMissingLSP1Interface(to); } else { revert LSP7NotifyTokenReceiverIsEOA(to); } } However, if the receiving contract meets one of the criteria mentioned above, supportsERC165InterfaceUnchecked() will incorrectly return true, even if the receiving contract does not support LSP-1. This would cause the transaction to revert when attempting to call universalReceiver() at the to address. Therefore, the to address will never be able to receive LSP7 tokens, even if allowNonLSP1Recipient is set to true. Note that the scenario demonstrated above is only one of the many possible impacts. As supportsERC165InterfaceUnchecked() is widely used throughout the codebase, such as in [tryNotifyUniversalReceiver()](), this pattern affects many LSPs: * The contract might not be able to send or receive [LSP7](https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol#L452-L497>) or [LSP8]() and [acceptOwnership()]() in LSP0 and LSP14 might not work. Additionally, if the contract implements a fallback function that returns non-zero bytes, the call to universalReceiver() will actually succeed. This is because the ILSP1UniversalReceiver interface expects universalReceiver() to return bytes: [ILSP1UniversalReceiver.sol#L32-L35]() function universalReceiver( bytes32 typeId, bytes calldata data ) external payable returns (bytes memory); This makes it possible to send LSP7/LSP8 tokens to contracts that don't implement LSP-1, even though allowNonLSP1Recipient is explicitly set to false to disable such behavior. ## Impact As supportsERC165InterfaceUnchecked() might incorrectly return true for certain contracts, the functionality of LSP0, LSP1 and LSP14 might behave unexpectedly for these contracts, which could break its functionality. ### Note on severity I am aware that usage of supportsERC165InterfaceUnchecked() is listed under the [Publicly Known Issues]() in the contest's README. However, given that it could potentially break LSP functionality for certain contracts, I have decided to raise it as a medium severity finding. ## Proof of Concept The following code contains two Foundry tests: * testERC165CheckPasses() illustrates that a contract with a fallback function that returns bytes will pass both supportsERC165InterfaceUnchecked() and universalReceiver(). * testERC165CheckCausesRevert() demonstrates how a contract that contains a function with the same selector as supportsInterface(bytes4) will revert. // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; import "forge-std/Test.sol"; import "../../contracts/LSP0ERC725Account/LSP0ERC725Account.sol"; import "../../contracts/LSP7DigitalAsset/LSP7DigitalAsset.sol"; contract ClashingSelector { // This function has the same selector as "supportsInterface(bytes4)" function pizza_mandate_apology(uint256) external view returns (uint256) { return 1337; } } contract Fallback { // This fallback function returns bytes fallback() external { bytes memory b = "AAAA"; uint256 length = b.length; assembly { mstore(0x00, 0x20) mstore(0x20, length) mstore(0x40, mload(add(b, 32))) return(0, 0x60) } } } contract supportsERC165InterfaceUnchecked_POC is Test { LSP0ERC725Account account; MockLSP7Token mockToken; function setUp() public { // Deploy LSP0 account with this address as owner account = new LSP0ERC725Account(address(this)); // Setup mock LSP7 token mockToken = new MockLSP7Token(); } function testERC165CheckPasses() public { // Deploy contract with a fallback function that returns bytes Fallback fallbackContract = new Fallback(); // Minting doesn't revert although allowNonLSP1Recipient is false mockToken.mint(address(fallbackContract), 1, false); assertEq(mockToken.balanceOf(address(fallbackContract)), 1); } function testERC165CheckCausesRevert() public { // Deploy contract that has a function with a same selector ClashingSelector clashingSelector = new ClashingSelector(); // Minting reverts although allowNonLSP1Recipient is true vm.expectRevert(); mockToken.mint(address(clashingSelector), 1, true); } } contract MockLSP7Token is LSP7DigitalAsset { constructor() LSP7DigitalAsset("", "", msg.sender, true) {} function mint(address to, uint256 amount, bool allowNonLSP1Recipient) public { _mint(to, amount, allowNonLSP1Recipient, ""); } } ## Recommended Mitigation Consider using the [supportsERC165()]() function instead, which accounts for such scenarios. ## Assessed type Library --- The text was updated successfully, but these errors were encountered: All reactions