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:
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:
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:
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.
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.
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.
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, "");
}
}
Consider using the supportsERC165() function instead, which accounts for such scenarios.
Library
The text was updated successfully, but these errors were encountered:
All reactions