Allowed calls in `LSP6KeyManager` doesn't allow calls with empty calldata

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol#L399-L416

Vulnerability details

Bug Description

Whenever a controller attempts to call a LSP0 account's execute() function without the relevant SUPER permissions, LSP6ExecuteModule will check that the call is one of the whitelisted allowed calls.

If the controller is trying to perform a call with no calldata (eg. .call("")), _extractExecuteParameters() decodes its function selector to 0x00000000:

LSP6ExecuteModule.sol#L352-L355

        // CHECK if there is at least a 4 bytes function selector
        bytes4 selector = executeCalldata.length >= 168
            ? bytes4(executeCalldata[164:168])
            : bytes4(0);

However, the _isAllowedFunction() will always return false for a 0x00000000 function selector if the allowed call only allows a specific function to be called:

LSP6ExecuteModule.sol#L410-L415

        bool isFunctionCall = requiredFunction != bytes4(0);

        // ANY function = 0xffffffff
        return
            allowedFunction == bytes4(type(uint32).max) ||
            (isFunctionCall && (requiredFunction == allowedFunction));

As _isAllowedFunction() is used to verify if the call being executed is whitelisted in allowed calls, this means that execute() calls with empty calldata will always fail.

This is an issue as:

• Users that have TRANSFERVALUE permission but don't have CALL permission are only allowed to call execute() with empty calldata.
• Setting the allowedFunction of an allowed call to 0xffffffff will allow users with both permissions to call other functions in the contract at the same address.

For instance, if a user wants to allow other users to send LYX to a contract with a receive() function (eg. other LSP0 accounts), but doesn't want them to be able to call other functions in the same contract, he will be unable to do so.

Impact

Due to how calls with empty calldata are handled in LSP6ExecuteModule, LSP6KeyManager cannot be configured to only allow only transfers of LYX to a contract with a receive() function for users without SUPER_TRANSFERVALUE permission.

Proof of Concept

The following Foundry test demonstrates how calling execute() with empty calldata to transfer LYX will revert with the NotAllowedCall() error:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../../contracts/LSP0ERC725Account/LSP0ERC725Account.sol";
import "../../contracts/LSP6KeyManager/LSP6KeyManager.sol";
import "../../contracts/LSP6KeyManager/LSP6Utils.sol";

contract Receiver {
    receive() external payable {}
}

contract KeyManagerTransferValue_POC is Test {
    LSP0ERC725Account account;
    LSP6KeyManager keyManager;

    address USER;
    Receiver receiver;

    function setUp() public {
        // Setup LSP0 account give it some LYX
        setupAccountWithKeyManager(); 
        deal(address(account), 10 ether);

        // Setup user address
        USER = makeAddr("USER");

        // Deploy contract with receive() function
        receiver = new Receiver();
    }

    function testCannotTransferValue() public {
        // Give user TRANSFERVALUE permission
        (bytes32[] memory keys, bytes[] memory values) = LSP6Utils.generateNewPermissionsKeys(
            IERC725Y(account), 
            USER,
            _PERMISSION_TRANSFERVALUE
        );
        keyManager.execute(abi.encodeWithSelector(
            LSP0ERC725AccountCore.setDataBatch.selector,
            keys,
            values
        ));

        // Add allowed call for LYX transfers to the receiver contract
        bytes32 key =  LSP2Utils.generateMappingWithGroupingKey(
            _LSP6KEY_ADDRESSPERMISSIONS_ALLOWEDCALLS_PREFIX,
            bytes20(USER)
        );
        bytes memory value = encodeAllowedCall(
            _ALLOWEDCALLS_TRANSFERVALUE,
            address(receiver),
            bytes4(0xffffffff),
            bytes4(0x00000000) // Only allow receive() to be called
        );
        keyManager.execute(abi.encodeWithSelector(
            LSP0ERC725AccountCore.setData.selector,
            key,
            value
        ));

        // USER attempts to transfer LYX to the receiver contract, but it reverts
        vm.prank(USER);
        vm.expectRevert(); // reverts with NotAllowedCall(), check with -vvvv
        account.execute(
            0, // OPERATION_0_CALL
            address(receiver),
            10 ether,
            ""
        );
    }

    // Helper function to deploy a LSP0 account with LSP6 KeyManager
    function setupAccountWithKeyManager() internal {
        // Deploy LSP0 account with this address as owner
        account = new LSP0ERC725Account(address(this));

        // Deploy LSP6 key manager for the account
        keyManager = new LSP6KeyManager(address(account));

        // Give this address all permissions
        (bytes32[] memory keys, bytes[] memory values) = LSP6Utils.generateNewPermissionsKeys(
            IERC725Y(account), 
            address(this),
            ALL_REGULAR_PERMISSIONS
        );
        account.setDataBatch(keys, values);

        // Transfer account ownership to the key manager
        account.transferOwnership(address(keyManager));
        vm.roll(block.number + 200);
        keyManager.execute(abi.encodeWithSelector(LSP0ERC725AccountCore.acceptOwnership.selector));
    }

    // Helper function to encode allowed calls
    function encodeAllowedCall(
        bytes4 callType, 
        address to, 
        bytes4 interfaceId, 
        bytes4 functionSelector
    ) internal pure returns (bytes memory) {
        return abi.encodePacked(uint16(0x20), callType, to, interfaceId, functionSelector);
    }
}

Recommended Mitigation

In _isAllowedFunction(), consider allowing the 0x00000000 function selector if the call has empty calldata and allowedFunction is set to bytes(0):

LSP6ExecuteModule.sol#L399-L416

    function _isAllowedFunction(
        bytes memory allowedCall,
        bytes4 requiredFunction,
+       bool isEmptyCall
    ) internal pure returns (bool) {
        // <offset> = 28 bytes x 8 bits = 224 bits
        //
        //                                                         function
        // <------------------------<offset>---------------------->v------v
        // 0000000ncafecafecafecafecafecafecafecafecafecafe5a5a5a5af1f1f1f1
        bytes4 allowedFunction = bytes4(bytes32(allowedCall) << 224);

-       bool isFunctionCall = requiredFunction != bytes4(0);
+       bool isFunctionCall = requiredFunction != bytes4(0) || isEmptyCall;

        // ANY function = 0xffffffff
        return
            allowedFunction == bytes4(type(uint32).max) ||
            (isFunctionCall && (requiredFunction == allowedFunction));
    }

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as high quality report

[c4-sponsor]

skimaharvey marked the issue as sponsor disputed

[skimaharvey]

If you want to achieve this you can just use 0xffffffff as selector and 00000001 (transfer value) as calltype. This will only let you make a transfer to the receive() or fallback() (if there is not receive function) functions of the receiver. The problem here is that you are using 0x00000000 as selector

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid