[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) { // = 28 bytes x 8 bits = 224 bits // // function // <---------------------------------------------->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 --- The text was updated successfully, but these errors were encountered: All reactions