code423n4 commented 1 year ago

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 or LSP8 tokens.
transferOwnership() 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, "");
    }
}