code-423n4 / 2023-06-lukso-findings

3 stars 1 forks source link

Avoid using the same ERC-165 interface ID for URDs and their callers #127

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol#L259-L265

Vulnerability details

Bug Description

Contracts that implement the LSP-1 standard include _INTERFACEID_LSP1 in their supportsInterface() function. This means that they have a universalReceiver() function that calls a Universal Receiver Delegate (URD), such as LSP1UniversalReceiverDelegateUP.

LSP0ERC725Account is an example of a URD caller:

LSP0ERC725AccountCore.sol#L704

            interfaceId == _INTERFACEID_LSP1 ||

Similarly, URDs are also expected to include _INTERFACEID_LSP1 in their supportsInterface() function, such as LSP1UniversalReceiverDelegateUP:

LSP1UniversalReceiverDelegateUP.sol#L259-L265

    function supportsInterface(
        bytes4 interfaceId
    ) public view virtual override returns (bool) {
        return
            interfaceId == _INTERFACEID_LSP1 ||
            super.supportsInterface(interfaceId);
    }

Therefore, URD callers will check for _INTERFACEID_LSP1 to determine if the address is a URD. LSP0ERC725AccountCore's universalReceiver() function is an example of this behavior:

LSP0ERC725AccountCore.sol#L479-L493

            // Checking LSP1 InterfaceId support
            if (
                universalReceiverDelegate.supportsERC165InterfaceUnchecked(
                    _INTERFACEID_LSP1
                )
            ) {
                // calling {universalReceiver} function on URD appending the caller and the value sent
                resultDefaultDelegate = universalReceiverDelegate
                    .callUniversalReceiverWithCallerInfos(
                        typeId,
                        receivedData,
                        msg.sender,
                        msg.value
                    );
            }

However, this is an issue as there is no way to differentiate between URDs and their callers. This could potentially allow users to perform a transaction that requires a LSP-1 compatible contract on the receiving end, such a LSP7 token transfer, but with a URD as the receiver instead.

For example, a user could send LSP7 tokens to the LSP1UniversalReceiverDelegateUP contract, and the call would succeed as its contract supports _INTERFACEID_LSP1 and has a universalReceiver() function. However, LSP1UniversalReceiverDelegateUP should not be able to receive LSP7 tokens as they will become stuck in the contract.

Additionally, some contracts might allow URD contracts to call setDataBatch() and write into its storage (eg. LSP0 accounts give the LSP1UniversalReceiverDelegateUP contract SUPER_SETDATA and REENTRANCY permissions). This could potentially lead to more serious impacts as URDs might unintentionally write into the storage of the calling contract.

Impact

Due to the URDs and their callers sharing the same ERC-165 interface IDs, URDs can be set as the receiver for transactions that require LSP-1 compatibility.

However, as URD contracts have limited functionality, this could cause undesired effects, such as tokens getting stuck or contracts having data written into their ERC725Y storage unintentionally.

Proof of Concept

Consider the following example contract, which acts as a wrapper for LSP7 tokens:

contract LSP7Wrapper is LSP7DigitalAsset {
    using LSP1Utils for address;

    address universalReceiverDelegate;

    constructor(address addr) LSP7DigitalAsset("Wrapper", "WRAP", msg.sender, true) {
        universalReceiverDelegate = addr;
    }

    // Function exchanges tokens from msg.sender for wrapped tokens
    function wrap(LSP7DigitalAsset token, uint256 amount) public {
        token.transfer(msg.sender, address(this), amount, false, "");
        _mint(msg.sender, amount, false, "");
    }

    // Override setDataBatch() to allow universalReceiverDelegateUP
    function setDataBatch(
        bytes32[] memory dataKeys,
        bytes[] memory dataValues
    ) public payable override(ERC725YCore, IERC725Y) {
        require(
            owner() == msg.sender || universalReceiverDelegate == msg.sender, 
            "Not authorized"
        );

        for (uint256 i = 0; i < dataKeys.length; ++i)
            _setData(dataKeys[i], dataValues[i]);
    }

    function universalReceiver(
        bytes32 typeId,
        bytes calldata data
    ) external payable returns (bytes memory) {
        universalReceiverDelegate.callUniversalReceiverWithCallerInfos(
            typeId,
            data,
            msg.sender,
            msg.value
        );

        return "";
    }

    function supportsInterface(
        bytes4 interfaceId
    ) public view virtual override returns (bool) {
        return interfaceId == _INTERFACEID_LSP1 || super.supportsInterface(interfaceId);
    }
}

Since it is meant to receive tokens, it allows universalReceiverDelegate (assume it is set as LSP1UniversalReceiverDelegateUP) to call setDataBatch(). However, if a users transfer WRAP tokens directly to the LSP1UniversalReceiverDelegateUP contract, this would lead to:

Recommended Mitigation

Consider having separate ERC-165 interface IDs for URDs and their callers, similar to how LSP17 has two different interface IDs for extensions and extendable contracts.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

CJ42 marked the issue as sponsor disputed

CJ42 commented 1 year ago

We do not consider this a valid issue. If you send to the wrong receiver address (e.g: the LSP1 Delegate address), this is a wrong parameter provided to the function.

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid