diff --git a/contracts/LSP6KeyManager/LSP6Errors.sol b/contracts/LSP6KeyManager/LSP6Errors.sol index 4f1e057b8..17d6855d9 100644 --- a/contracts/LSP6KeyManager/LSP6Errors.sol +++ b/contracts/LSP6KeyManager/LSP6Errors.sol @@ -114,10 +114,9 @@ error AddressPermissionArrayIndexValueNotAnAddress( error NoERC725YDataKeysAllowed(address from); /** - * @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has Allowed Calls set. + * @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has no Allowed Calls set. * - * @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address - * using the linked {target}. + * @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address using the linked {target}. * * @param from The address that has no AllowedCalls. */ diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 8a39463ea..ac60e9c6c 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -153,7 +153,7 @@ abstract contract LSP6KeyManagerCore is /** * @inheritdoc ILSP6KeyManager * - * @custom:events VerifiedCall event when the permissions related to `payload` have been verified successfully. + * @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully. */ function execute( bytes calldata payload @@ -164,7 +164,7 @@ abstract contract LSP6KeyManagerCore is /** * @inheritdoc ILSP6KeyManager * - * @custom:events VerifiedCall event for each permissions related to each `payload` that have been verified successfully. + * @custom:events {PermissionsVerified} event for each permissions related to each `payload` that have been verified successfully. */ function executeBatch( uint256[] calldata values, @@ -199,7 +199,7 @@ abstract contract LSP6KeyManagerCore is /** * @inheritdoc ILSP6KeyManager * - * @custom:events {VerifiedCall} event when the permissions related to `payload` have been verified successfully. + * @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully. * * @custom:hint You can use `validityTimestamps == 0` to define an `executeRelayCall` transaction that is indefinitely valid, * meaning that does not require to start from a specific date/time, or that has an expiration date/time/ @@ -528,11 +528,21 @@ abstract contract LSP6KeyManagerCore is // ERC725X.execute(uint256,address,uint256,bytes) } else if (erc725Function == IERC725X.execute.selector) { + ( + uint256 operationType, + address to, + uint256 value, + bytes memory data + ) = abi.decode(payload[4:], (uint256, address, uint256, bytes)); + LSP6ExecuteModule._verifyCanExecute( _target, from, permissions, - payload + operationType, + to, + value, + data ); } else if ( erc725Function == ILSP14Ownable2Step.transferOwnership.selector || diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol index 727c6f9e1..84c85939a 100644 --- a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol @@ -54,32 +54,16 @@ abstract contract LSP6ExecuteModule { * @param controlledContract the address of the ERC725 contract where the payload is executed and where the permissions are verified. * @param controller the address who want to run the execute function on the ERC725Account. * @param permissions the permissions of the controller address. - * @param payload the ABI encoded payload `controlledContract.execute(...)`. */ function _verifyCanExecute( address controlledContract, address controller, bytes32 permissions, - bytes calldata payload + uint256 operationType, + address to, + uint256 value, + bytes memory data ) internal view virtual { - // CHECK the offset of `data` is not pointing to the previous parameters - // - // offsets in calldata for ERC725X.execute(...) parameters (excluding function selector) - // - // - `operationType`: index 0 in calldata - // - `to`: index 32 - // - `value`: index 64 - // - `data`'s offset location: index 96 - // - `data` starts at: index 128 (= 0x0000...0080) - if (bytes32(payload[100:132]) != bytes32(uint256(128))) { - revert InvalidPayload(payload); - } - - // MUST be one of the ERC725X operation types. - uint256 operationType = uint256(bytes32(payload[4:36])); - - address to = address(bytes20(payload[48:68])); - // if to is the KeyManager address revert if (to == address(this)) { revert CallingKeyManagerNotAllowed(); @@ -92,10 +76,10 @@ abstract contract LSP6ExecuteModule { // Check to restrict controllers with execute permissions to call lsp20 functions // to avoid setting the reentrancy guard to a non-valid state - // if (payload.length >= 168 && to == address(this)) { + // if (data.length >= 4 && to == address(this)) { // if ( - // bytes4(payload[164:168]) == ILSP20.lsp20VerifyCall.selector || - // bytes4(payload[164:168]) == ILSP20.lsp20VerifyCallResult.selector + // bytes4(data) == ILSP20.lsp20VerifyCall.selector || + // bytes4(data) == ILSP20.lsp20VerifyCallResult.selector // ) { // revert CallingLSP20FunctionsOnLSP6NotAllowed(); // } @@ -108,7 +92,9 @@ abstract contract LSP6ExecuteModule { controlledContract, controller, permissions, - payload + to, + value, + data ); } @@ -119,7 +105,7 @@ abstract contract LSP6ExecuteModule { ) { // required to check for permission TRANSFERVALUE if we are funding // the contract on deployment via a payable constructor - bool isFundingContract = uint256(bytes32(payload[68:100])) != 0; + bool isFundingContract = value != 0; return _verifyCanDeployContract( @@ -138,7 +124,9 @@ abstract contract LSP6ExecuteModule { controlledContract, controller, permissions, - payload + to, + value, + data ); } @@ -169,7 +157,9 @@ abstract contract LSP6ExecuteModule { address controlledContract, address controller, bytes32 permissions, - bytes calldata payload + address to, + uint256 value, + bytes memory data ) internal view virtual { bool hasSuperStaticCall = permissions.hasPermission( _PERMISSION_SUPER_STATICCALL @@ -180,36 +170,35 @@ abstract contract LSP6ExecuteModule { _requirePermissions(controller, permissions, _PERMISSION_STATICCALL); - _verifyAllowedCall(controlledContract, controller, payload); + _verifyAllowedCall( + controlledContract, + controller, + OPERATION_3_STATICCALL, + to, + value, + data + ); } function _verifyCanCall( address controlledContract, address controller, bytes32 permissions, - bytes calldata payload + address to, + uint256 value, + bytes memory data ) internal view virtual { - bool isTransferringValue = uint256(bytes32(payload[68:100])) != 0; + bool isValueTransfer = value != 0; bool hasSuperTransferValue = permissions.hasPermission( _PERMISSION_SUPER_TRANSFERVALUE ); - // all the parameters are abi-encoded (padded to 32 bytes words) - // - // 4 (ERC725X.execute selector) - // + 32 (uint256 operationType) - // + 32 (address to/target) - // + 32 (uint256 value) - // + 32 (`data` offset) - // + 32 (`data` length) - // -------------------- - // = 164 bytes in total - bool isCallDataPresent = payload.length > 164; + bool isEmptyCall = data.length == 0; bool hasSuperCall = permissions.hasPermission(_PERMISSION_SUPER_CALL); - if (isTransferringValue && !hasSuperTransferValue) { + if (isValueTransfer && !hasSuperTransferValue) { _requirePermissions( controller, permissions, @@ -219,40 +208,41 @@ abstract contract LSP6ExecuteModule { // CHECK if we are doing an empty call, as the receive() or fallback() function // of the controlledContract could run some code. - if (!hasSuperCall && !isCallDataPresent && !isTransferringValue) { + if (isEmptyCall && !isValueTransfer && !hasSuperCall) { _requirePermissions(controller, permissions, _PERMISSION_CALL); } - if (isCallDataPresent && !hasSuperCall) { + if (!isEmptyCall && !hasSuperCall) { _requirePermissions(controller, permissions, _PERMISSION_CALL); } // Skip if caller has SUPER permissions for external calls, with or without calldata (empty calls) - if (hasSuperCall && !isTransferringValue) return; + if (!isValueTransfer && hasSuperCall) return; // Skip if caller has SUPER permission for value transfers - if (hasSuperTransferValue && !isCallDataPresent && isTransferringValue) - return; + if (isEmptyCall && isValueTransfer && hasSuperTransferValue) return; // Skip if both SUPER permissions are present if (hasSuperCall && hasSuperTransferValue) return; - _verifyAllowedCall(controlledContract, controller, payload); + _verifyAllowedCall( + controlledContract, + controller, + OPERATION_0_CALL, + to, + value, + data + ); } function _verifyAllowedCall( address controlledContract, address controllerAddress, - bytes calldata payload + uint256 operationType, + address to, + uint256 value, + bytes memory data ) internal view virtual { - ( - uint256 operationType, - address to, - uint256 value, - bytes4 selector, - bool isEmptyCall - ) = _extractExecuteParameters(payload); - // CHECK for ALLOWED CALLS bytes memory allowedCalls = ERC725Y(controlledContract) .getAllowedCallsFor(controllerAddress); @@ -261,6 +251,11 @@ abstract contract LSP6ExecuteModule { revert NoCallsAllowed(controllerAddress); } + bool isEmptyCall = data.length == 0; + + // CHECK if there is at least a 4 bytes function selector + bytes4 selector = data.length >= 4 ? bytes4(data) : bytes4(0); + bytes4 requiredCallTypes = _extractCallType( operationType, value, @@ -334,33 +329,6 @@ abstract contract LSP6ExecuteModule { } } - function _extractExecuteParameters( - bytes calldata executeCalldata - ) internal pure returns (uint256, address, uint256, bytes4, bool) { - uint256 operationType = uint256(bytes32(executeCalldata[4:36])); - - // CHECK that it is a valid address left-padded with `00` on the 12 upper bytes - if (bytes12(executeCalldata[36:48]) != bytes12(0)) { - revert InvalidPayload(executeCalldata); - } - address to = address(bytes20(executeCalldata[48:68])); - - uint256 value = uint256(bytes32(executeCalldata[68:100])); - - // CHECK if there is at least a 4 bytes function selector - bytes4 selector = executeCalldata.length >= 168 - ? bytes4(executeCalldata[164:168]) - : bytes4(0); - - return ( - operationType, - to, - value, - selector, - executeCalldata.length == 164 - ); - } - function _isAllowedAddress( bytes memory allowedCall, address to diff --git a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol index c80517fe6..b16538adc 100644 --- a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol +++ b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol @@ -34,9 +34,19 @@ contract KeyManagerInternalTester is LSP6KeyManager { function verifyAllowedCall( address _sender, - bytes calldata _payload + uint256 operationType, + address to, + uint256 value, + bytes memory data ) public view { - super._verifyAllowedCall(_target, _sender, _payload); + super._verifyAllowedCall( + _target, + _sender, + operationType, + to, + value, + data + ); } function isCompactBytesArrayOfAllowedCalls( @@ -103,9 +113,11 @@ contract KeyManagerInternalTester is LSP6KeyManager { return _addressPermission.hasPermission(_permissions); } - function extractExecuteParameters( - bytes calldata executeCalldata - ) public pure returns (uint256, address, uint256, bytes4, bool) { - return super._extractExecuteParameters(executeCalldata); + function verifyPermissions( + address from, + uint256 msgValue, + bytes calldata payload + ) public view { + super._verifyPermissions(from, msgValue, payload); } } diff --git a/contracts/Mocks/TargetContract.sol b/contracts/Mocks/TargetContract.sol index a9c90e7b1..225bba71f 100644 --- a/contracts/Mocks/TargetContract.sol +++ b/contracts/Mocks/TargetContract.sol @@ -27,6 +27,10 @@ contract TargetContract { } function setNamePayable(string memory name) public payable { + _name = name; + } + + function setNamePayableMinimumValue(string memory name) public payable { require(msg.value >= 50, "Not enough value provided"); _name = name; } diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index 7d529735f..6bfaf4181 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -71,7 +71,7 @@ Execute A `payload` on the linked [`target`](#target) contract after having veri **Emitted events:** -- VerifiedCall event when the permissions related to `payload` have been verified successfully. +- [`PermissionsVerified`](#permissionsverified) event when the permissions related to `payload` have been verified successfully. @@ -119,7 +119,7 @@ Same as [`execute`](#execute) but execute a batch of payloads (abi-encoded funct **Emitted events:** -- VerifiedCall event for each permissions related to each `payload` that have been verified successfully. +- [`PermissionsVerified`](#permissionsverified) event for each permissions related to each `payload` that have been verified successfully. @@ -172,7 +172,7 @@ Allows any address (executor) to execute a payload (= abi-encoded function call) **Emitted events:** -- [`VerifiedCall`](#verifiedcall) event when the permissions related to `payload` have been verified successfully. +- [`PermissionsVerified`](#permissionsverified) event when the permissions related to `payload` have been verified successfully. @@ -776,7 +776,10 @@ function _verifyCanExecute( address controlledContract, address controller, bytes32 permissions, - bytes payload + uint256 operationType, + address to, + uint256 value, + bytes data ) internal view; ``` @@ -789,7 +792,10 @@ verify if `controllerAddress` has the required permissions to interact with othe | `controlledContract` | `address` | the address of the ERC725 contract where the payload is executed and where the permissions are verified. | | `controller` | `address` | the address who want to run the execute function on the ERC725Account. | | `permissions` | `bytes32` | the permissions of the controller address. | -| `payload` | `bytes` | the ABI encoded payload `controlledContract.execute(...)`. | +| `operationType` | `uint256` | - | +| `to` | `address` | - | +| `value` | `uint256` | - | +| `data` | `bytes` | - |
@@ -812,7 +818,9 @@ function _verifyCanStaticCall( address controlledContract, address controller, bytes32 permissions, - bytes payload + address to, + uint256 value, + bytes data ) internal view; ``` @@ -825,7 +833,9 @@ function _verifyCanCall( address controlledContract, address controller, bytes32 permissions, - bytes payload + address to, + uint256 value, + bytes data ) internal view; ``` @@ -837,7 +847,10 @@ function _verifyCanCall( function _verifyAllowedCall( address controlledContract, address controllerAddress, - bytes payload + uint256 operationType, + address to, + uint256 value, + bytes data ) internal view; ``` @@ -871,16 +884,6 @@ extract the bytes4 representation of a single bit for the type of call according
-### \_extractExecuteParameters - -```solidity -function _extractExecuteParameters( - bytes executeCalldata -) internal pure returns (uint256, address, uint256, bytes4, bool); -``` - -
- ### \_isAllowedAddress ```solidity @@ -1071,19 +1074,19 @@ Used in the end of the `nonReentrant` modifier after the method execution is ter ## Events -### VerifiedCall +### PermissionsVerified :::note References -- Specification details: [**LSP-6-KeyManager**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-6-KeyManager.md#verifiedcall) +- Specification details: [**LSP-6-KeyManager**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-6-KeyManager.md#permissionsverified) - Solidity implementation: [`LSP6KeyManager.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP6KeyManager/LSP6KeyManager.sol) -- Event signature: `VerifiedCall(address,uint256,bytes4)` -- Event topic hash: `0xa54458b75709e42f79700ffb6cfc57c7e224d8a77a52c457ee7ecb8e22636280` +- Event signature: `PermissionsVerified(address,uint256,bytes4)` +- Event topic hash: `0xc0a62328f6bf5e3172bb1fcb2019f54b2c523b6a48e3513a2298fbf0150b781e` ::: ```solidity -event VerifiedCall(address indexed signer, uint256 indexed value, bytes4 indexed selector); +event PermissionsVerified(address indexed signer, uint256 indexed value, bytes4 indexed selector); ``` _Verified the permissions of `signer` for calling function `selector` on the linked account and sending `value` of native token._ @@ -1453,6 +1456,25 @@ Reverts when a `from` address has _"any whitelisted call"_ as allowed call set.
+### KeyManagerCannotBeSetAsExtensionForLSP20Functions + +:::note References + +- Specification details: [**LSP-6-KeyManager**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-6-KeyManager.md#keymanagercannotbesetasextensionforlsp20functions) +- Solidity implementation: [`LSP6KeyManager.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP6KeyManager/LSP6KeyManager.sol) +- Error signature: `KeyManagerCannotBeSetAsExtensionForLSP20Functions()` +- Error hash: `0x4a9fa8cf` + +::: + +```solidity +error KeyManagerCannotBeSetAsExtensionForLSP20Functions(); +``` + +reverts when the address of the Key Manager is being set as extensions for lsp20 functions + +
+ ### LSP6BatchExcessiveValueSent :::note References @@ -1528,7 +1550,7 @@ This error occurs when there was not enough funds sent to the batch functions `e error NoCallsAllowed(address from); ``` -_The address `from` is not authorised to use the linked account contract to make external calls, because it has Allowed Calls set._ +_The address `from` is not authorised to use the linked account contract to make external calls, because it has no Allowed Calls set._ Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address using the linked [`target`](#target). diff --git a/tests/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP.behaviour.ts b/tests/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP.behaviour.ts index 83177b789..593418596 100644 --- a/tests/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP.behaviour.ts +++ b/tests/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP.behaviour.ts @@ -33,7 +33,7 @@ import { ERC725YDataKeys, INTERFACE_IDS, OPERATION_TYPES, LSP1_TYPE_IDS } from ' // fixtures import { callPayload, getLSP5MapAndArrayKeysValue, setupKeyManager } from '../utils/fixtures'; import { LSP6TestContext } from '../utils/context'; -import { BigNumber, BytesLike, ContractTransaction, Transaction } from 'ethers'; +import { BigNumber, BytesLike, Transaction } from 'ethers'; export type LSP1TestAccounts = { owner1: SignerWithAddress; diff --git a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts index 501d5db67..e5f275068 100644 --- a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts +++ b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; import { ethers } from 'hardhat'; +import { BigNumber } from 'ethers'; import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; import { @@ -25,9 +26,17 @@ import { LSP6TestContext } from '../../../utils/context'; import { setupKeyManager } from '../../../utils/fixtures'; // helpers -import { abiCoder, combineAllowedCalls } from '../../../utils/helpers'; - -export const shouldBehaveLikePermissionCall = (buildContext: () => Promise) => { +import { + abiCoder, + combineAllowedCalls, + combineCallTypes, + combinePermissions, + provider, +} from '../../../utils/helpers'; + +export const shouldBehaveLikePermissionCall = ( + buildContext: (initialFunding?: BigNumber) => Promise, +) => { let context: LSP6TestContext; describe('when making an empty call via `ERC25X.execute(...)` -> (`data` = `0x`, `value` = 0)', () => { @@ -348,32 +357,6 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise { - it('should revert', async () => { - let payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContract.address, - 0, - '0xcafecafe', - ]); - - // edit the `data` offset - payload = payload.replace( - '0000000000000000000000000000000000000000000000000000000000000080', - '0000000000000000000000000000000000000000000000000000000000000040', - ); - - await expect( - addressCanMakeCallWithAllowedCalls.sendTransaction({ - to: context.universalProfile.address, - data: payload, - }), - ) - .to.be.revertedWithCustomError(context.keyManager, 'InvalidPayload') - .withArgs(payload); - }); - }); - describe('when interacting via `execute(...)`', () => { describe('when caller has ALL PERMISSIONS', () => { it('should pass and change state at the target contract', async () => { @@ -497,4 +480,410 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise { + let targetContract: TargetContract; + let addressWithNoPermissions: SignerWithAddress; + + before(async () => { + context = await buildContext(); + + addressWithNoPermissions = context.accounts[1]; + + targetContract = await new TargetContract__factory(context.accounts[0]).deploy(); + + const permissionKeys = [ + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + context.owner.address.substring(2), + ]; + + const permissionValues = [ALL_PERMISSIONS]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + it('Should revert when caller calls the KeyManager through execute', async () => { + const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData( + 'lsp20VerifyCall', + [context.accounts[2].address, 0, '0xaabbccdd'], // random arguments + ); + + await expect( + context.universalProfile.execute( + OPERATION_TYPES.CALL, + context.keyManager.address, + 0, + lsp20VerifyCallPayload, + ), + ).to.be.revertedWithCustomError(context.keyManager, 'CallingKeyManagerNotAllowed'); + }); + + describe('when the offset of the `data` payload is not `0x00...80`', () => { + describe('if the offset points backwards to the `value` parameter', () => { + // We add the target in the allowed calls for each of these controller + let controllerCanTransferValue; + let controllerCanTransferValueAndCall; + let controllerCanCall; + + let controllerCanOnlySign; + + let controllerCanSuperCall; + let controllerCanSuperTransferValue; + + let targetContract: FallbackInitializer; + + let executePayload; + + before(async () => { + context = await buildContext(ethers.utils.parseEther('50')); + + const accounts = await ethers.getSigners(); + + controllerCanTransferValue = accounts[1]; + controllerCanTransferValueAndCall = accounts[2]; + controllerCanCall = accounts[3]; + + controllerCanOnlySign = accounts[4]; + + controllerCanSuperCall = accounts[5]; + controllerCanSuperTransferValue = accounts[6]; + + targetContract = await new FallbackInitializer__factory(context.accounts[0]).deploy(); + + const permissionKeys = [ + // permissions + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanTransferValue.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanTransferValueAndCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanOnlySign.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperTransferValue.address.substring(2), + // allowed calls + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanTransferValue.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanTransferValueAndCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanCall.address.substring(2), + ]; + + const allowedCall = combineAllowedCalls( + [combineCallTypes(CALLTYPE.CALL, CALLTYPE.VALUE)], + [targetContract.address], + ['0xffffffff'], + ['0xffffffff'], + ); + + const permissionValues = [ + // permissions + PERMISSIONS.TRANSFERVALUE, + combinePermissions(PERMISSIONS.TRANSFERVALUE, PERMISSIONS.CALL), + PERMISSIONS.CALL, + PERMISSIONS.SIGN, + PERMISSIONS.SUPER_CALL, + PERMISSIONS.SUPER_TRANSFERVALUE, + // allowed calls, + allowedCall, + allowedCall, + allowedCall, + ]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + afterEach('clearing target contract storage', async () => { + await context.accounts[0].sendTransaction({ + to: targetContract.address, + data: '0xcafecafe', + }); + }); + + describe('when the `value` parameter has some number, that points to some `data == 0x0000...04deadbe`', () => { + before(async () => { + executePayload = context.universalProfile.interface.encodeFunctionData('execute', [ + OPERATION_TYPES.CALL, + targetContract.address, + 36, + '0xdeadbeef', + ]); + + // edit the `data` offset to points to the `value` parameter + executePayload = executePayload.replace( + '0000000000000000000000000000000000000000000000000000000000000080', + '0000000000000000000000000000000000000000000000000000000000000040', + ); + }); + + describe('when caller has permission TRANSFERVALUE only', () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + // We need to do low level send transactions as the data offset is not standard + controllerCanTransferValue.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanTransferValue.address, 'CALL'); + }); + }); + + describe('when caller has permission CALL only', () => { + it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE'", async () => { + await expect( + controllerCanCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanCall.address, 'TRANSFERVALUE'); + }); + }); + + describe('when caller does not have neither CALL nor TRANSFERVALUE permissions', () => { + it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE' (as value transfer is the first thing being checked", async () => { + await expect( + controllerCanOnlySign.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'TRANSFERVALUE'); + }); + }); + + describe('when caller has both permissions CALL + TRANSFERVALUE', () => { + it('should pass and allow to call the contract', async () => { + expect(await provider.getBalance(targetContract.address)).to.equal(0); + + await controllerCanTransferValueAndCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + expect(await provider.getBalance(targetContract.address)).to.equal(36); + }); + }); + }); + + describe('when the `value` parameter is 0, meaning calldata is empty (= empty call)', () => { + before(async () => { + executePayload = context.universalProfile.interface.encodeFunctionData('execute', [ + OPERATION_TYPES.CALL, + targetContract.address, + 0, + '0x', + ]); + + // edit the `data` offset to points to the `value` parameter + executePayload = executePayload.replace( + '0000000000000000000000000000000000000000000000000000000000000080', + '0000000000000000000000000000000000000000000000000000000000000040', + ); + }); + + describe('when controller has permission CALL only', () => { + it('should pass', async () => { + await controllerCanCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when controller has SUPER_CALL', () => { + it('should pass', async () => { + await controllerCanSuperCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when controller does not have permission CALL', () => { + it('should revert with `NotAuthorised` error to `CALL`', async () => { + await expect( + controllerCanOnlySign.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + }); + + describe("if the offset points forwards (there are 32 random bytes between the data's offset and the data's length", () => { + // We add the target in the allowed calls for each of these controller + let controllerCanCall; + let controllerCanSuperCall; + let controllerCanOnlySign; + + let targetContract: FallbackInitializer; + + let executePayload; + + before(async () => { + context = await buildContext(ethers.utils.parseEther('50')); + + const accounts = await ethers.getSigners(); + + controllerCanCall = accounts[1]; + controllerCanSuperCall = accounts[2]; + controllerCanOnlySign = accounts[3]; + + targetContract = await new FallbackInitializer__factory(context.accounts[0]).deploy(); + + const permissionKeys = [ + // permissions + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanOnlySign.address.substring(2), + // allowed calls + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanCall.address.substring(2), + ]; + + const allowedCall = combineAllowedCalls( + [combineCallTypes(CALLTYPE.CALL)], + [targetContract.address], + ['0xffffffff'], + ['0xffffffff'], + ); + + const permissionValues = [ + // permissions + PERMISSIONS.CALL, + PERMISSIONS.SUPER_CALL, + PERMISSIONS.SIGN, + // allowed calls, + allowedCall, + ]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + afterEach('clearing target contract storage', async () => { + await context.accounts[0].sendTransaction({ + to: targetContract.address, + data: '0xcafecafe', + }); + }); + + describe('the length byte points to some number', () => { + before('constructing manually the payload', async () => { + // 0x44c028fe --> ERC725X.execute(uint256,address,uint256,bytes) selector + // 0000000000000000000000000000000000000000000000000000000000000000 --> operationType = CALL (0) + // 0000000000000000000000004ed7c70f96b99c776995fb64377f0d4ab3b0e1c1 --> target = targetContract.address + // 0000000000000000000000000000000000000000000000000000000000000000 --> value = 0 + // 00000000000000000000000000000000000000000000000000000000000000a0 --> offset = 160 + // cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe --> 32 random bytes in between + // 0000000000000000000000000000000000000000000000000000000000000004 --> `data.length` = 4 + // deadbeef00000000000000000000000000000000000000000000000000000000 --> `data` = 0xdeadbeef + executePayload = + '0x44c028fe0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' + + targetContract.address.substring(2).toLowerCase() + + '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe0000000000000000000000000000000000000000000000000000000000000004deadbeef00000000000000000000000000000000000000000000000000000000'; + }); + + describe('when caller has permission CALL', () => { + it('should pass', async () => { + await controllerCanCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when caller has permission SUPER_CALL', () => { + it('should pass', async () => { + await controllerCanSuperCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe("when caller does not have permission 'CALL' nor 'SUPER_CALL'", () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + controllerCanOnlySign.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + + describe('the length byte points to `0x0000...0000` (= no data, this is an empty call)', () => { + before('constructing manually the payload', async () => { + // 0x44c028fe --> ERC725X.execute(uint256,address,uint256,bytes) selector + // 0000000000000000000000000000000000000000000000000000000000000000 --> operationType = CALL (0) + // 0000000000000000000000004ed7c70f96b99c776995fb64377f0d4ab3b0e1c1 --> target = targetContract.address + // 0000000000000000000000000000000000000000000000000000000000000000 --> value = 0 + // 00000000000000000000000000000000000000000000000000000000000000a0 --> offset = 160 + // cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe --> 32 random bytes in between + // 0000000000000000000000000000000000000000000000000000000000000000 --> `data.length` = 0 + executePayload = + '0x44c028fe0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' + + targetContract.address.substring(2).toLowerCase() + + '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe0000000000000000000000000000000000000000000000000000000000000000'; + }); + + describe('when caller has permission CALL', () => { + it('should pass', async () => { + await controllerCanCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when caller has permission SUPER_CALL', () => { + it('should pass', async () => { + await controllerCanSuperCall.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe("when caller does not have permission 'CALL' nor 'SUPER_CALL'", () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + controllerCanOnlySign.sendTransaction({ + to: context.universalProfile.address, + data: executePayload, + }), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + }); + }); + }); }; diff --git a/tests/LSP6KeyManager/Interactions/PermissionCall.test.ts b/tests/LSP6KeyManager/Interactions/PermissionCall.test.ts index 7a9a6c3f4..7732a0dcb 100644 --- a/tests/LSP6KeyManager/Interactions/PermissionCall.test.ts +++ b/tests/LSP6KeyManager/Interactions/PermissionCall.test.ts @@ -27,9 +27,19 @@ import { LSP6TestContext } from '../../utils/context'; import { setupKeyManager } from '../../utils/fixtures'; // helpers -import { abiCoder, combineAllowedCalls, LOCAL_PRIVATE_KEYS } from '../../utils/helpers'; - -export const shouldBehaveLikePermissionCall = (buildContext: () => Promise) => { +import { + abiCoder, + combineAllowedCalls, + combineCallTypes, + combinePermissions, + LOCAL_PRIVATE_KEYS, + provider, +} from '../../utils/helpers'; +import { BigNumber } from 'ethers'; + +export const shouldBehaveLikePermissionCall = ( + buildContext: (initialFunding?: BigNumber) => Promise, +) => { let context: LSP6TestContext; describe('when making an empty call via `ERC25X.execute(...)` -> (`data` = `0x`, `value` = 0)', () => { @@ -407,29 +417,6 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise { - it('should revert', async () => { - let payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContract.address, - 0, - '0xcafecafe', - ]); - - // edit the `data` offset - payload = payload.replace( - '0000000000000000000000000000000000000000000000000000000000000080', - '0000000000000000000000000000000000000000000000000000000000000040', - ); - - await expect( - context.keyManager.connect(addressCanMakeCallWithAllowedCalls).execute(payload), - ) - .to.be.revertedWithCustomError(context.keyManager, 'InvalidPayload') - .withArgs(payload); - }); - }); - describe('when interacting via `execute(...)`', () => { describe('when caller has ALL PERMISSIONS', () => { it('should pass and change state at the target contract', async () => { @@ -1050,5 +1037,334 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise { + describe('if the offset points backwards to the `value` parameter', () => { + // We add the target in the allowed calls for each of these controller + let controllerCanTransferValue; + let controllerCanTransferValueAndCall; + let controllerCanCall; + + let controllerCanOnlySign; + + let controllerCanSuperCall; + let controllerCanSuperTransferValue; + + let targetContract: FallbackInitializer; + + let executePayload; + + before(async () => { + context = await buildContext(ethers.utils.parseEther('50')); + + const accounts = await ethers.getSigners(); + + controllerCanTransferValue = accounts[1]; + controllerCanTransferValueAndCall = accounts[2]; + controllerCanCall = accounts[3]; + + controllerCanOnlySign = accounts[4]; + + controllerCanSuperCall = accounts[5]; + controllerCanSuperTransferValue = accounts[6]; + + targetContract = await new FallbackInitializer__factory(context.accounts[0]).deploy(); + + const permissionKeys = [ + // permissions + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanTransferValue.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanTransferValueAndCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanOnlySign.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperTransferValue.address.substring(2), + // allowed calls + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanTransferValue.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanTransferValueAndCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanCall.address.substring(2), + ]; + + const allowedCall = combineAllowedCalls( + [combineCallTypes(CALLTYPE.CALL, CALLTYPE.VALUE)], + [targetContract.address], + ['0xffffffff'], + ['0xffffffff'], + ); + + const permissionValues = [ + // permissions + PERMISSIONS.TRANSFERVALUE, + combinePermissions(PERMISSIONS.TRANSFERVALUE, PERMISSIONS.CALL), + PERMISSIONS.CALL, + PERMISSIONS.SIGN, + PERMISSIONS.SUPER_CALL, + PERMISSIONS.SUPER_TRANSFERVALUE, + // allowed calls, + allowedCall, + allowedCall, + allowedCall, + ]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + afterEach('clearing target contract storage', async () => { + await context.accounts[0].sendTransaction({ + to: targetContract.address, + data: '0xcafecafe', + }); + }); + + describe('when the `value` parameter has some number, that points to some `data == 0x0000...04deadbe`', () => { + before(async () => { + executePayload = context.universalProfile.interface.encodeFunctionData('execute', [ + OPERATION_TYPES.CALL, + targetContract.address, + 36, + '0xdeadbeef', + ]); + + // edit the `data` offset to points to the `value` parameter + executePayload = executePayload.replace( + '0000000000000000000000000000000000000000000000000000000000000080', + '0000000000000000000000000000000000000000000000000000000000000040', + ); + }); + + describe('when caller has permission TRANSFERVALUE only', () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + context.keyManager.connect(controllerCanTransferValue).execute(executePayload), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanTransferValue.address, 'CALL'); + }); + }); + + describe('when caller has permission CALL only', () => { + it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE'", async () => { + await expect(context.keyManager.connect(controllerCanCall).execute(executePayload)) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanCall.address, 'TRANSFERVALUE'); + }); + }); + + describe('when caller does not have neither CALL nor TRANSFERVALUE permissions', () => { + it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE' (as value transfer is the first thing being checked", async () => { + await expect( + context.keyManager.connect(controllerCanOnlySign).execute(executePayload), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'TRANSFERVALUE'); + }); + }); + + describe('when caller has both permissions CALL + TRANSFERVALUE', () => { + it('should pass and allow to call the contract', async () => { + expect(await provider.getBalance(targetContract.address)).to.equal(0); + + await context.keyManager + .connect(controllerCanTransferValueAndCall) + .execute(executePayload); + + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + expect(await provider.getBalance(targetContract.address)).to.equal(36); + }); + }); + }); + + describe('when the `value` parameter is 0, meaning calldata is empty (= empty call)', () => { + before(async () => { + executePayload = context.universalProfile.interface.encodeFunctionData('execute', [ + OPERATION_TYPES.CALL, + targetContract.address, + 0, + '0x', + ]); + + // edit the `data` offset to points to the `value` parameter + executePayload = executePayload.replace( + '0000000000000000000000000000000000000000000000000000000000000080', + '0000000000000000000000000000000000000000000000000000000000000040', + ); + }); + + describe('when controller has permission CALL only', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when controller has SUPER_CALL', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanSuperCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when controller does not have permission CALL', () => { + it('should revert with `NotAuthorised` error to `CALL`', async () => { + await expect( + context.keyManager.connect(controllerCanOnlySign).execute(executePayload), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + }); + + describe("if the offset points forwards (there are 32 random bytes between the data's offset and the data's length", () => { + // We add the target in the allowed calls for each of these controller + let controllerCanCall; + let controllerCanSuperCall; + let controllerCanOnlySign; + + let targetContract: FallbackInitializer; + + let executePayload; + + before(async () => { + context = await buildContext(ethers.utils.parseEther('50')); + + const accounts = await ethers.getSigners(); + + controllerCanCall = accounts[1]; + controllerCanSuperCall = accounts[2]; + controllerCanOnlySign = accounts[3]; + + targetContract = await new FallbackInitializer__factory(context.accounts[0]).deploy(); + + const permissionKeys = [ + // permissions + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSuperCall.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanOnlySign.address.substring(2), + // allowed calls + ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + + controllerCanCall.address.substring(2), + ]; + + const allowedCall = combineAllowedCalls( + [combineCallTypes(CALLTYPE.CALL)], + [targetContract.address], + ['0xffffffff'], + ['0xffffffff'], + ); + + const permissionValues = [ + // permissions + PERMISSIONS.CALL, + PERMISSIONS.SUPER_CALL, + PERMISSIONS.SIGN, + // allowed calls, + allowedCall, + ]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + afterEach('clearing target contract storage', async () => { + await context.accounts[0].sendTransaction({ + to: targetContract.address, + data: '0xcafecafe', + }); + }); + + describe('the length byte points to some number', () => { + before('constructing manually the payload', async () => { + // 0x44c028fe --> ERC725X.execute(uint256,address,uint256,bytes) selector + // 0000000000000000000000000000000000000000000000000000000000000000 --> operationType = CALL (0) + // 0000000000000000000000004ed7c70f96b99c776995fb64377f0d4ab3b0e1c1 --> target = targetContract.address + // 0000000000000000000000000000000000000000000000000000000000000000 --> value = 0 + // 00000000000000000000000000000000000000000000000000000000000000a0 --> offset = 160 + // cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe --> 32 random bytes in between + // 0000000000000000000000000000000000000000000000000000000000000004 --> `data.length` = 4 + // deadbeef00000000000000000000000000000000000000000000000000000000 --> `data` = 0xdeadbeef + executePayload = + '0x44c028fe0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' + + targetContract.address.substring(2).toLowerCase() + + '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe0000000000000000000000000000000000000000000000000000000000000004deadbeef00000000000000000000000000000000000000000000000000000000'; + }); + + describe('when caller has permission CALL', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when caller has permission SUPER_CALL', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanSuperCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe("when caller does not have permission 'CALL' nor 'SUPER_CALL'", () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + context.keyManager.connect(controllerCanOnlySign).execute(executePayload), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + + describe('the length byte points to `0x0000...0000` (= no data, this is an empty call)', () => { + before('constructing manually the payload', async () => { + // 0x44c028fe --> ERC725X.execute(uint256,address,uint256,bytes) selector + // 0000000000000000000000000000000000000000000000000000000000000000 --> operationType = CALL (0) + // 0000000000000000000000004ed7c70f96b99c776995fb64377f0d4ab3b0e1c1 --> target = targetContract.address + // 0000000000000000000000000000000000000000000000000000000000000000 --> value = 0 + // 00000000000000000000000000000000000000000000000000000000000000a0 --> offset = 160 + // cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe --> 32 random bytes in between + // 0000000000000000000000000000000000000000000000000000000000000000 --> `data.length` = 0 + executePayload = + '0x44c028fe0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' + + targetContract.address.substring(2).toLowerCase() + + '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0cafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe0000000000000000000000000000000000000000000000000000000000000000'; + }); + + describe('when caller has permission CALL', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe('when caller has permission SUPER_CALL', () => { + it('should pass', async () => { + await context.keyManager.connect(controllerCanSuperCall).execute(executePayload); + expect(await targetContract.caller()).to.equal(context.universalProfile.address); + }); + }); + + describe("when caller does not have permission 'CALL' nor 'SUPER_CALL'", () => { + it("should revert with 'NotAuthorised' error to 'CALL'", async () => { + await expect( + context.keyManager.connect(controllerCanOnlySign).execute(executePayload), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(controllerCanOnlySign.address, 'CALL'); + }); + }); + }); + }); + }); }); }; diff --git a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts index 78309cd6d..09e0d7a93 100644 --- a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts +++ b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts @@ -336,11 +336,11 @@ export const shouldBehaveLikeExecuteRelayCall = ( it('should revert', async () => { const nameToSet = 'Alice'; const targetContractPayload = targetContract.interface.encodeFunctionData( - 'setNamePayable', + 'setNamePayableMinimumValue', [nameToSet], ); - const requiredValueForExecution = 51; // specified in `setNamePayable(..)` + const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)` const latestNonce = await context.keyManager.callStatic.getNonce(signer.address, 0); @@ -413,11 +413,11 @@ export const shouldBehaveLikeExecuteRelayCall = ( it('should pass if signer has the target contract address in its list of allowed calls', async () => { const nameToSet = 'Alice'; const targetContractPayload = targetContract.interface.encodeFunctionData( - 'setNamePayable', + 'setNamePayableMinimumValue', [nameToSet], ); - const requiredValueForExecution = 51; // specified in `setNamePayable(..)` + const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)` const latestNonce = await context.keyManager.callStatic.getNonce(signer.address, 0); @@ -481,11 +481,11 @@ export const shouldBehaveLikeExecuteRelayCall = ( it("should revert with 'NotAllowedCall' error if signer does not have any listed under its allowed calls", async () => { const nameToSet = 'Alice'; const targetContractPayload = targetContract.interface.encodeFunctionData( - 'setNamePayable', + 'setNamePayableMinimumValue', [nameToSet], ); - const requiredValueForExecution = 51; // specified in `setNamePayable(..)` + const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)` const latestNonce = await context.keyManager.callStatic.getNonce( signerNoAllowedCalls.address, diff --git a/tests/LSP6KeyManager/internals/AllowedCalls.internal.ts b/tests/LSP6KeyManager/internals/AllowedCalls.internal.ts index 265916e22..26b6f5017 100644 --- a/tests/LSP6KeyManager/internals/AllowedCalls.internal.ts +++ b/tests/LSP6KeyManager/internals/AllowedCalls.internal.ts @@ -2,7 +2,7 @@ import { expect } from 'chai'; import { ethers } from 'hardhat'; import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; -import { TargetContract, TargetContract__factory, UniversalProfile__factory } from '../../../types'; +import { TargetContract, TargetContract__factory } from '../../../types'; // constants import { @@ -199,17 +199,20 @@ export const testAllowedCallsInternals = ( describe('`verifyAllowedCall(...)`', () => { describe('when the ERC725X payload (transfer 1 LYX) is for an address listed in the allowed calls', () => { it('should pass', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - allowedEOA.address, - ethers.utils.parseEther('1'), - '0x', - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: allowedEOA.address, + value: ethers.utils.parseEther('1'), + data: '0x', + }; await expect( context.keyManagerInternalTester.verifyAllowedCall( canCallOnlyTwoAddresses.address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ).to.not.be.reverted; }); @@ -221,17 +224,20 @@ export const testAllowedCallsInternals = ( '0xdeadbeefdeadbeefdeaddeadbeefdeadbeefdead', ); - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - disallowedAddress, - ethers.utils.parseEther('1'), - '0x', - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: disallowedAddress, + value: ethers.utils.parseEther('1'), + data: '0x', + }; await expect( context.keyManagerInternalTester.verifyAllowedCall( canCallOnlyTwoAddresses.address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') @@ -243,17 +249,20 @@ export const testAllowedCallsInternals = ( it('should revert', async () => { const randomAddress = ethers.Wallet.createRandom().address; - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - randomAddress, - ethers.utils.parseEther('1'), - '0x', - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: randomAddress, + value: ethers.utils.parseEther('1'), + data: '0x', + }; await expect( context.keyManagerInternalTester.verifyAllowedCall( canCallNoAllowedCalls.address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NoCallsAllowed') @@ -332,58 +341,82 @@ export const testAllowedCallsInternals = ( }); it('should fail with `NotAllowedCall` error when the allowed address has `v` permission only (`v` = VALUE)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContractValue.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: targetContractValue.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractValue.address, randomPayload); }); it('should pass when the allowed address has `c` permission only (`c` = CALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContractCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: targetContractCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ).to.not.be.reverted; }); it('should fail with `NotAllowedCall` error when the allowed address has `s` permission only (`s` = STATICCALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContractStaticCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: targetContractStaticCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractStaticCall.address, randomPayload); }); it('should fail with `NotAllowedCall` error when the allowed address has `d` permission only (`d` = DELEGATECALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - targetContractDelegateCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: targetContractDelegateCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractDelegateCall.address, randomPayload); @@ -419,58 +452,82 @@ export const testAllowedCallsInternals = ( }); it('should fail with `NotAllowedCall` error when the allowed address has `v` permission only (`v` = VALUE)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.STATICCALL, - targetContractValue.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.STATICCALL, + to: targetContractValue.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractValue.address, randomPayload); }); it('should fail with `NotAllowedCall` error when the allowed address has `c` permission only (`c` = CALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.STATICCALL, - targetContractCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.STATICCALL, + to: targetContractCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractCall.address, randomPayload); }); it('should pass when the allowed address has `s` permission only (`s` = STATICCALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.STATICCALL, - targetContractStaticCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.STATICCALL, + to: targetContractStaticCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ).to.not.be.reverted; }); it('should fail with `NotAllowedCall` error when the allowed address has `d` permission only (`d` = DELEGATECALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.STATICCALL, - targetContractDelegateCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.STATICCALL, + to: targetContractDelegateCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractDelegateCall.address, randomPayload); @@ -506,60 +563,84 @@ export const testAllowedCallsInternals = ( }); it('should fail with `NotAllowedCall` error when the allowed address has `v` permission only (`v` = VALUE)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.DELEGATECALL, - targetContractValue.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.DELEGATECALL, + to: targetContractValue.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractValue.address, randomPayload); }); it('should fail with `NotAllowedCall` error when the allowed address has `c` permission only (`c` = CALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.DELEGATECALL, - targetContractCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.DELEGATECALL, + to: targetContractCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractCall.address, randomPayload); }); it('should fail with `NotAllowedCall` error when the allowed address has `s` permission only (`s` = STATICCALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.DELEGATECALL, - targetContractStaticCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.DELEGATECALL, + to: targetContractStaticCall.address, + value: 0, + data: randomPayload, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') .withArgs(controller.address, targetContractStaticCall.address, randomPayload); }); it('should pass when the allowed address has `d` permission only (`d` = DELEGATECALL)', async () => { - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.DELEGATECALL, - targetContractDelegateCall.address, - 0, - randomPayload, // random payload - ]); + const executeParams = { + operationType: OPERATION_TYPES.DELEGATECALL, + to: targetContractDelegateCall.address, + value: 0, + data: randomPayload, // random payload + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(controller.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + controller.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ).to.not.be.reverted; }); }); @@ -617,8 +698,6 @@ export const testAllowedCallsInternals = ( permissionValues = permissionValues.concat(zeroBytesValues); - console.log(permissionValues); - await setupKeyManagerHelper(context, permissionKeys, permissionValues); }); @@ -626,21 +705,22 @@ export const testAllowedCallsInternals = ( const randomAddress = ethers.Wallet.createRandom().address.toLowerCase(); const randomData = '0xaabbccdd'; - const universalProfileInterface = UniversalProfile__factory.createInterface(); - - const payload: string = universalProfileInterface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - randomAddress, - ethers.utils.parseEther('1'), - randomData, - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: randomAddress, + value: ethers.utils.parseEther('1'), + data: randomData, + }; describe('should revert with `NoCallsAllowed` error', () => { it(`when AllowedCalls contain noBytes -> 0x`, async () => { await expect( context.keyManagerInternalTester.verifyAllowedCall( controllers[0].account.address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ).to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NoCallsAllowed'); }); @@ -655,7 +735,10 @@ export const testAllowedCallsInternals = ( await expect( context.keyManagerInternalTester.verifyAllowedCall( controllers[index + 1].account.address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ) .to.be.revertedWithCustomError( @@ -681,18 +764,16 @@ export const testAllowedCallsInternals = ( const randomAddress = ethers.Wallet.createRandom().address.toLowerCase(); const randomData = '0xaabbccdd'; - let payload: string; + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: randomAddress, + value: ethers.utils.parseEther('1'), + data: randomData, + }; before(async () => { context = await buildContext(); - payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - randomAddress, - ethers.utils.parseEther('1'), - randomData, - ]); - const permissionKeys = [ ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + context.owner.address.substring(2), ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + @@ -729,7 +810,10 @@ export const testAllowedCallsInternals = ( context.keyManagerInternalTester.verifyAllowedCall( // `index + 1` because `accounts[0]` has all permissions context.accounts[index + 1].address, - payload, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'NotAllowedCall') @@ -781,15 +865,21 @@ export const testAllowedCallsInternals = ( const randomData = '0xaabbccdd'; const randomAddress = ethers.Wallet.createRandom().address.toLowerCase(); - const payload = context.universalProfile.interface.encodeFunctionData('execute', [ - OPERATION_TYPES.CALL, - randomAddress, - ethers.utils.parseEther('1'), - randomData, - ]); + const executeParams = { + operationType: OPERATION_TYPES.CALL, + to: randomAddress, + value: ethers.utils.parseEther('1'), + data: randomData, + }; await expect( - context.keyManagerInternalTester.verifyAllowedCall(anyAllowedCalls.address, payload), + context.keyManagerInternalTester.verifyAllowedCall( + anyAllowedCalls.address, + executeParams.operationType, + executeParams.to, + executeParams.value, + executeParams.data, + ), ) .to.be.revertedWithCustomError(context.keyManagerInternalTester, 'InvalidWhitelistedCall') .withArgs(anyAllowedCalls.address); diff --git a/tests/LSP6KeyManager/internals/Execute.internal.ts b/tests/LSP6KeyManager/internals/Execute.internal.ts index 1d0242a64..066cbcd8b 100644 --- a/tests/LSP6KeyManager/internals/Execute.internal.ts +++ b/tests/LSP6KeyManager/internals/Execute.internal.ts @@ -22,7 +22,7 @@ export const testExecuteInternals = (buildContext: () => Promise { + describe('`_verifyPermissions(address,uint256,bytes)`', () => { it('should pass when the function is called with valid parameters', async () => { const executeParameters = { operationType: OPERATION_TYPES.CALL, @@ -38,17 +38,9 @@ export const testExecuteInternals = (buildContext: () => Promise { @@ -76,9 +68,11 @@ export const testExecuteInternals = (buildContext: () => Promise