From 0825ae30b84e9ba179086c145502e1cc6f93b875 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 07:20:43 +0200 Subject: [PATCH 01/23] feat!: add callee params to lsp20 --- contracts/LSP20CallVerification/ILSP20CallVerifier.sol | 2 ++ .../LSP20CallVerification/LSP20CallVerification.sol | 10 +++++++++- contracts/LSP20CallVerification/LSP20Constants.sol | 8 ++++---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol index 2f31efe1e..90365761d 100644 --- a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol +++ b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol @@ -12,11 +12,13 @@ interface ILSP20CallVerifier { * the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should * be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`. * + * @param callee The address of the contract that implements the LSP20 interface * @param caller The address who called the function on the msg.sender * @param value The value sent by the caller to the function called on the msg.sender * @param receivedCalldata The receivedCalldata sent by the caller to the msg.sender */ function lsp20VerifyCall( + address callee, address caller, uint256 value, bytes memory receivedCalldata diff --git a/contracts/LSP20CallVerification/LSP20CallVerification.sol b/contracts/LSP20CallVerification/LSP20CallVerification.sol index 5c1477d24..2e24f63eb 100644 --- a/contracts/LSP20CallVerification/LSP20CallVerification.sol +++ b/contracts/LSP20CallVerification/LSP20CallVerification.sol @@ -29,6 +29,7 @@ abstract contract LSP20CallVerification { (bool success, bytes memory returnedData) = logicVerifier.call( abi.encodeWithSelector( ILSP20.lsp20VerifyCall.selector, + address(this), msg.sender, msg.value, msg.data @@ -56,7 +57,14 @@ abstract contract LSP20CallVerification { (bool success, bytes memory returnedData) = logicVerifier.call( abi.encodeWithSelector( ILSP20.lsp20VerifyCallResult.selector, - keccak256(abi.encodePacked(msg.sender, msg.value, msg.data)), + keccak256( + abi.encodePacked( + address(this), + msg.sender, + msg.value, + msg.data + ) + ), callResult ) ); diff --git a/contracts/LSP20CallVerification/LSP20Constants.sol b/contracts/LSP20CallVerification/LSP20Constants.sol index 65dff7f87..d6cc3a73f 100644 --- a/contracts/LSP20CallVerification/LSP20Constants.sol +++ b/contracts/LSP20CallVerification/LSP20Constants.sol @@ -4,14 +4,14 @@ pragma solidity ^0.8.4; // bytes4(keccak256("LSP20CallVerification")) bytes4 constant _INTERFACEID_LSP20_CALL_VERIFICATION = 0x1a0eb6a5; -// `lsp20VerifyCall(address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector -bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0x480c0ec2; +// `lsp20VerifyCall(addres,address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector +bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0xd553a612; // bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"01")) -bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x9bf04b01; +bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x1a238001; // bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"00")) -bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION = 0x9bf04b00; +bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION = 0x1a238000; // bytes4(ILSP20.lsp20VerifyCallResult.selector) bytes4 constant _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE = 0xd3fc45d3; From 7b27c645e83b19a7b3038d95e623b984464c14e7 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 07:21:23 +0200 Subject: [PATCH 02/23] chore: update lsp6 to with new lsp20 interface --- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 369a390ca..b4ce275b2 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -313,10 +313,11 @@ abstract contract LSP6KeyManagerCore is * on the {`target`} contract (while sending `msgValue` alongside the call). * * If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 magic value will be returned: - * - `0x9bf04b00`: LSP20 magic value **without** post verification (last byte is `0x00`). - * - `0x9bf04b01`: LSP20 magic value **with** post-verification (last byte is `0x01`). + * - `0x1a238000`: LSP20 magic value **without** post verification (last byte is `0x00`). + * - `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`). */ function lsp20VerifyCall( + address callee, address caller, uint256 msgValue, bytes calldata data @@ -330,10 +331,10 @@ abstract contract LSP6KeyManagerCore is } // If target is invoking the verification, emit the event and change the reentrancy guard - if (msg.sender == _target) { + if (msg.sender == callee) { bool isReentrantCall = _nonReentrantBefore(isSetData, caller); - _verifyPermissions(caller, msgValue, data); + _verifyPermissions(callee, caller, msgValue, data); emit PermissionsVerified(caller, msgValue, bytes4(data)); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function @@ -350,12 +351,12 @@ abstract contract LSP6KeyManagerCore is if (isReentrantCall) { _requirePermissions( caller, - ERC725Y(_target).getPermissionsFor(caller), + ERC725Y(callee).getPermissionsFor(caller), _PERMISSION_REENTRANCY ); } - _verifyPermissions(caller, msgValue, data); + _verifyPermissions(callee, caller, msgValue, data); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function return @@ -398,7 +399,7 @@ abstract contract LSP6KeyManagerCore is bool isReentrantCall = _nonReentrantBefore(isSetData, msg.sender); - _verifyPermissions(msg.sender, msgValue, payload); + _verifyPermissions(_target, msg.sender, msgValue, payload); emit PermissionsVerified(msg.sender, msgValue, bytes4(payload)); bytes memory result = _executePayload(msgValue, payload); @@ -463,7 +464,7 @@ abstract contract LSP6KeyManagerCore is bool isReentrantCall = _nonReentrantBefore(isSetData, signer); - _verifyPermissions(signer, msgValue, payload); + _verifyPermissions(_target, signer, msgValue, payload); emit PermissionsVerified(signer, msgValue, bytes4(payload)); bytes memory result = _executePayload(msgValue, payload); @@ -503,11 +504,12 @@ abstract contract LSP6KeyManagerCore is * @param payload The abi-encoded function call to execute on the {target} contract. */ function _verifyPermissions( + address callee, address from, uint256 msgValue, bytes calldata payload ) internal view virtual { - bytes32 permissions = ERC725Y(_target).getPermissionsFor(from); + bytes32 permissions = ERC725Y(callee).getPermissionsFor(from); if (permissions == bytes32(0)) revert NoPermissionsSet(from); bytes4 erc725Function = bytes4(payload); @@ -521,7 +523,7 @@ abstract contract LSP6KeyManagerCore is ); LSP6SetDataModule._verifyCanSetData( - _target, + callee, from, permissions, inputKey, @@ -535,7 +537,7 @@ abstract contract LSP6KeyManagerCore is .decode(payload[4:], (bytes32[], bytes[])); LSP6SetDataModule._verifyCanSetData( - _target, + callee, from, permissions, inputKeys, @@ -552,7 +554,7 @@ abstract contract LSP6KeyManagerCore is ) = abi.decode(payload[4:], (uint256, address, uint256, bytes)); LSP6ExecuteModule._verifyCanExecute( - _target, + callee, from, permissions, operationType, From 79b7b06043bd7953618f7407bcfd60aa654c9201 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 07:22:03 +0200 Subject: [PATCH 03/23] test: update tests to work with new lsp20 interface --- contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol | 2 +- .../Mocks/LSP20Owners/FirstCallReturnExpandedInvalidValue.sol | 3 ++- .../Mocks/LSP20Owners/FirstCallReturnInvalidMagicValue.sol | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol index dde9bfa47..3f997e0ff 100644 --- a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol +++ b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol @@ -118,7 +118,7 @@ contract KeyManagerInternalTester is LSP6KeyManager { uint256 msgValue, bytes calldata payload ) public { - super._verifyPermissions(from, msgValue, payload); + super._verifyPermissions(_target, from, msgValue, payload); // This event is emitted just for a sake of not marking this function as `view`, // as Hardhat has a bug that does not catch error that occured from failed `abi.decode` diff --git a/contracts/Mocks/LSP20Owners/FirstCallReturnExpandedInvalidValue.sol b/contracts/Mocks/LSP20Owners/FirstCallReturnExpandedInvalidValue.sol index cc5f69c7e..59ea77849 100644 --- a/contracts/Mocks/LSP20Owners/FirstCallReturnExpandedInvalidValue.sol +++ b/contracts/Mocks/LSP20Owners/FirstCallReturnExpandedInvalidValue.sol @@ -14,13 +14,14 @@ contract FirstCallReturnExpandedInvalidValue { address public target; function lsp20VerifyCall( + address callee, address caller, uint256 value, bytes memory data ) external returns (bytes32) { emit CallVerified(); - return keccak256(abi.encode(caller, value, data)); + return keccak256(abi.encode(callee, caller, value, data)); } function acceptOwnership(address newTarget) external { diff --git a/contracts/Mocks/LSP20Owners/FirstCallReturnInvalidMagicValue.sol b/contracts/Mocks/LSP20Owners/FirstCallReturnInvalidMagicValue.sol index 1a7d361c7..d2aaddff7 100644 --- a/contracts/Mocks/LSP20Owners/FirstCallReturnInvalidMagicValue.sol +++ b/contracts/Mocks/LSP20Owners/FirstCallReturnInvalidMagicValue.sol @@ -13,6 +13,7 @@ contract FirstCallReturnInvalidMagicValue { address public target; function lsp20VerifyCall( + address, address, uint256, bytes memory From b0552a4b854f92e04709a16a18c279dfa49419fe Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 07:22:52 +0200 Subject: [PATCH 04/23] refactor: update constants.ts file --- constants.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/constants.ts b/constants.ts index 884c1a54b..32ff9d730 100644 --- a/constants.ts +++ b/constants.ts @@ -61,9 +61,9 @@ export const ERC1271_VALUES = { export const LSP20_MAGIC_VALUES = { VERIFY_CALL: { // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x01" - WITH_POST_VERIFICATION: '0x9bf04b01', + WITH_POST_VERIFICATION: '0x1a238001', // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x00" - NO_POST_VERIFICATION: '0x9bf04b00', + NO_POST_VERIFICATION: '0x1a238000', }, // bytes4(keccak256("lsp20VerifyCallResult(bytes32,bytes)")) VERIFY_CALL_RESULT: '0xd3fc45d3', From f5bfd090646c0176f0b341fc262147a6f8d72ad5 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 07:56:13 +0200 Subject: [PATCH 05/23] test: fix failing tests --- constants.ts | 2 +- contracts/LSP20CallVerification/LSP20Constants.sol | 4 ++-- .../LSP6/Interactions/PermissionCall.test.ts | 2 +- .../LSP6/Interactions/Security.test.ts | 3 ++- tests/LSP6KeyManager/Interactions/PermissionCall.test.ts | 2 +- tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol | 7 ++++++- 6 files changed, 13 insertions(+), 7 deletions(-) diff --git a/constants.ts b/constants.ts index 32ff9d730..4ec2109dd 100644 --- a/constants.ts +++ b/constants.ts @@ -35,7 +35,7 @@ export const INTERFACE_IDS = { LSP17Extendable: '0xa918fa6b', LSP17Extension: '0xcee78b40', LSP20CallVerification: '0x1a0eb6a5', - LSP20CallVerifier: '0x480c0ec2', + LSP20CallVerifier: '0xc9dfc532', LSP25ExecuteRelayCall: '0x5ac79908', }; diff --git a/contracts/LSP20CallVerification/LSP20Constants.sol b/contracts/LSP20CallVerification/LSP20Constants.sol index d6cc3a73f..4134c6a89 100644 --- a/contracts/LSP20CallVerification/LSP20Constants.sol +++ b/contracts/LSP20CallVerification/LSP20Constants.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.4; // bytes4(keccak256("LSP20CallVerification")) bytes4 constant _INTERFACEID_LSP20_CALL_VERIFICATION = 0x1a0eb6a5; -// `lsp20VerifyCall(addres,address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector -bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0xd553a612; +// `lsp20VerifyCall(address,address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector +bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0xc9dfc532; // bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"01")) bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x1a238001; diff --git a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts index 1287fc62a..8bad4c69e 100644 --- a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts +++ b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts @@ -497,7 +497,7 @@ export const shouldBehaveLikePermissionCall = ( 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 + [context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments ); await expect( diff --git a/tests/LSP20CallVerification/LSP6/Interactions/Security.test.ts b/tests/LSP20CallVerification/LSP6/Interactions/Security.test.ts index ede08ed98..2381378a0 100644 --- a/tests/LSP20CallVerification/LSP6/Interactions/Security.test.ts +++ b/tests/LSP20CallVerification/LSP6/Interactions/Security.test.ts @@ -106,7 +106,7 @@ export const testSecurityScenarios = (buildContext: () => Promise { const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData( 'lsp20VerifyCall', - [context.accounts[2].address, 0, '0xaabbccdd'], // random arguments + [context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments ); await expect( @@ -290,6 +290,7 @@ export const testSecurityScenarios = (buildContext: () => Promise { const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData( 'lsp20VerifyCall', - [context.accounts[2].address, 0, '0xaabbccdd'], // random arguments + [context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments ); const executePayload = context.universalProfile.interface.encodeFunctionData('execute', [ diff --git a/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol index f8e4901f8..758465664 100644 --- a/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol +++ b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol @@ -102,7 +102,12 @@ contract LSP6SetDataTest is Test { keyManager.execute(callData); // CHECK the LSP20 verification function reverts as well - keyManager.lsp20VerifyCall(malicious, 0, functionArgs); + keyManager.lsp20VerifyCall( + address(universalProfile), + malicious, + 0, + functionArgs + ); // CHECK it reverts when calling directly the Universal Profile universalProfile.setData(dataKey, dataValue); From 5b523160d861371b3d8939c92193dfc5e62ae25b Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 08:23:40 +0200 Subject: [PATCH 06/23] test: fix foundry tests --- tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol index 758465664..0f8ef3959 100644 --- a/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol +++ b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol @@ -227,7 +227,12 @@ contract LSP6SetDataTest is Test { keyManager.execute(callData); // CHECK the LSP20 verification function reverts as well - keyManager.lsp20VerifyCall(malicious, 0, functionArgs); + keyManager.lsp20VerifyCall( + address(universalProfile), + malicious, + 0, + functionArgs + ); // CHECK it reverts when calling directly the Universal Profile universalProfile.setData(dataKey, dataValue); From 62e26796bb1fff62f19efbd384ecb5f502b0f598 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 08:25:44 +0200 Subject: [PATCH 07/23] docs: generate lsp6 new abi docs --- docs/_interface_ids_table.mdx | 2 +- .../LSP6KeyManager/LSP6KeyManager.md | 22 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/docs/_interface_ids_table.mdx b/docs/_interface_ids_table.mdx index dce5ae1a4..bfab29132 100644 --- a/docs/_interface_ids_table.mdx +++ b/docs/_interface_ids_table.mdx @@ -15,5 +15,5 @@ | **LSP17Extendable** | `0xa918fa6b` | Module to add more functionalities to a contract using extensions. | | **LSP17Extension** | `0xcee78b40` | Module to create a contract that can act as an extension. | | **LSP20CallVerification** | `0x1a0eb6a5` | Implementation of a contract calling the verification functions according to LSP20 - Call Verification standard. | -| **LSP20CallVerifier** | `0x480c0ec2` | Interface for the LSP20 Call Verification standard, a set of functions intended to perform verifications on behalf of another contract. | +| **LSP20CallVerifier** | `0xc9dfc532` | Interface for the LSP20 Call Verification standard, a set of functions intended to perform verifications on behalf of another contract. | | **LSP25ExecuteRelayCall** | `0x5ac79908` | | diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index 95e13300c..7e682e98f 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -332,8 +332,8 @@ Checks if a signature was signed by a controller that has the permission `SIGN`. - Specification details: [**LSP-6-KeyManager**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-6-KeyManager.md#lsp20verifycall) - Solidity implementation: [`LSP6KeyManager.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP6KeyManager/LSP6KeyManager.sol) -- Function signature: `lsp20VerifyCall(address,uint256,bytes)` -- Function selector: `0x9bf04b11` +- Function signature: `lsp20VerifyCall(address,address,uint256,bytes)` +- Function selector: `0x1a2380e1` ::: @@ -341,13 +341,14 @@ Checks if a signature was signed by a controller that has the permission `SIGN`. This function can call by any other address than the {`target`}. This allows to verify permissions in a _"read-only"_ manner. Anyone can call this function to verify if the `caller` has the right permissions to perform the abi-encoded function call `data` on the {`target`} contract (while sending `msgValue` alongside the call). If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 magic value will be returned: -- `0x9bf04b00`: LSP20 magic value **without** post verification (last byte is `0x00`). -- `0x9bf04b01`: LSP20 magic value **with** post-verification (last byte is `0x01`). +- `0x1a238000`: LSP20 magic value **without** post verification (last byte is `0x00`). +- `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`). ::: ```solidity function lsp20VerifyCall( + address callee, address caller, uint256 msgValue, bytes data @@ -356,11 +357,12 @@ function lsp20VerifyCall( #### Parameters -| Name | Type | Description | -| ---------- | :-------: | ----------------------------------------------------- | -| `caller` | `address` | The address who called the function on the msg.sender | -| `msgValue` | `uint256` | - | -| `data` | `bytes` | - | +| Name | Type | Description | +| ---------- | :-------: | --------------------------------------------------------------- | +| `callee` | `address` | The address of the contract that implements the LSP20 interface | +| `caller` | `address` | The address who called the function on the msg.sender | +| `msgValue` | `uint256` | - | +| `data` | `bytes` | - | #### Returns @@ -1150,6 +1152,7 @@ _Execute the `payload` passed to `execute(...)` or `executeRelayCall(...)`_ ```solidity function _verifyPermissions( + address callee, address from, uint256 msgValue, bytes payload @@ -1162,6 +1165,7 @@ Verify if the `from` address is allowed to execute the `payload` on the [`target | Name | Type | Description | | ---------- | :-------: | ------------------------------------------------------------------- | +| `callee` | `address` | - | | `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. | | `msgValue` | `uint256` | - | | `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. | From 085dbad93a83fee8e7f29d6fe70fa9ece146d044 Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 08:46:25 +0200 Subject: [PATCH 08/23] docs: update lsp6 interfaceID --- constants.ts | 2 +- contracts/LSP6KeyManager/LSP6Constants.sol | 2 +- docs/_interface_ids_table.mdx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/constants.ts b/constants.ts index 4ec2109dd..d80084b9d 100644 --- a/constants.ts +++ b/constants.ts @@ -26,7 +26,7 @@ export const INTERFACE_IDS = { ERC725Y: '0x629aa694', LSP0ERC725Account: '0x24871b3d', LSP1UniversalReceiver: '0x6bb56a14', - LSP6KeyManager: '0x66918867', + LSP6KeyManager: '0xe7424397', LSP7DigitalAsset: '0x05519512', LSP8IdentifiableDigitalAsset: '0x1ae9ba1f', LSP9Vault: '0x28af17e6', diff --git a/contracts/LSP6KeyManager/LSP6Constants.sol b/contracts/LSP6KeyManager/LSP6Constants.sol index 8d4fba6a0..fac187d61 100644 --- a/contracts/LSP6KeyManager/LSP6Constants.sol +++ b/contracts/LSP6KeyManager/LSP6Constants.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.4; // --- ERC165 interface ids -bytes4 constant _INTERFACEID_LSP6 = 0x66918867; +bytes4 constant _INTERFACEID_LSP6 = 0xe7424397; // --- ERC725Y Data Keys diff --git a/docs/_interface_ids_table.mdx b/docs/_interface_ids_table.mdx index bfab29132..674870dac 100644 --- a/docs/_interface_ids_table.mdx +++ b/docs/_interface_ids_table.mdx @@ -6,7 +6,7 @@ | **ERC725Y** | `0x629aa694` | General Data key-value store. | | **LSP0ERC725Account** | `0x24871b3d` | Interface of the [LSP-0-ERC725Account] standard, an account based smart contract that represents an identity on-chain. | | **LSP1UniversalReceiver** | `0x6bb56a14` | Interface of the LSP1 - Universal Receiver standard, an entry function for a contract to receive arbitrary information. | -| **LSP6KeyManager** | `0x66918867` | Interface of the LSP6 - Key Manager standard, a contract acting as a controller of an ERC725 Account using predfined permissions. | +| **LSP6KeyManager** | `0xe7424397` | Interface of the LSP6 - Key Manager standard, a contract acting as a controller of an ERC725 Account using predfined permissions. | | **LSP7DigitalAsset** | `0x05519512` | Interface of the LSP7 - Digital Asset standard, a fungible digital asset. | | **LSP8IdentifiableDigitalAsset** | `0x1ae9ba1f` | Interface of the LSP8 - Identifiable Digital Asset standard, a non-fungible digital asset. | | **LSP9Vault** | `0x28af17e6` | Interface of LSP9 - Vault standard, a blockchain vault that can hold assets and interact with other smart contracts. | From 5889edf4d4b98afc9d15574eff50f3ec852c869b Mon Sep 17 00:00:00 2001 From: Maxime Date: Tue, 26 Sep 2023 19:55:31 +0200 Subject: [PATCH 09/23] feat!: add EXECUTE_RELAY_CALL permission --- constants.ts | 3 +- contracts/LSP6KeyManager/LSP6Constants.sol | 3 +- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 20 ++++++++++--- .../LSP6ExecuteRelayCallModule.sol | 30 +++++++++++++++++++ .../KeyManager/KeyManagerInternalsTester.sol | 2 +- 5 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol diff --git a/constants.ts b/constants.ts index d80084b9d..4c98c5af6 100644 --- a/constants.ts +++ b/constants.ts @@ -271,7 +271,7 @@ export const CALLTYPE = { /** * @dev `bytes32` hex value for all the LSP6 permissions excluding REENTRANCY, DELEGATECALL and SUPER_DELEGATECALL for security (these should be set manually) */ -export const ALL_PERMISSIONS = '0x00000000000000000000000000000000000000000000000000000000003f3f7f'; +export const ALL_PERMISSIONS = '0x0000000000000000000000000000000000000000000000000000000000483f7f'; export type LSP6PermissionName = keyof typeof PERMISSIONS; @@ -298,6 +298,7 @@ export const PERMISSIONS = { DELEGATECALL: "0x0000000000000000000000000000000000000000000000000000000000008000", DEPLOY: "0x0000000000000000000000000000000000000000000000000000000000010000", SUPER_SETDATA: "0x0000000000000000000000000000000000000000000000000000000000020000", + EXECUTE_RELAY_CALL: '0x0000000000000000000000000000000000000000000000000000000000090000', SETDATA: "0x0000000000000000000000000000000000000000000000000000000000040000", ENCRYPT: "0x0000000000000000000000000000000000000000000000000000000000080000", DECRYPT: "0x0000000000000000000000000000000000000000000000000000000000100000", diff --git a/contracts/LSP6KeyManager/LSP6Constants.sol b/contracts/LSP6KeyManager/LSP6Constants.sol index fac187d61..3a9dd01f4 100644 --- a/contracts/LSP6KeyManager/LSP6Constants.sol +++ b/contracts/LSP6KeyManager/LSP6Constants.sol @@ -49,12 +49,13 @@ bytes32 constant _PERMISSION_DELEGATECALL = 0x000000000000000 bytes32 constant _PERMISSION_DEPLOY = 0x0000000000000000000000000000000000000000000000000000000000010000; bytes32 constant _PERMISSION_SUPER_SETDATA = 0x0000000000000000000000000000000000000000000000000000000000020000; bytes32 constant _PERMISSION_SETDATA = 0x0000000000000000000000000000000000000000000000000000000000040000; +bytes32 constant _PERMISSION_EXECUTE_RELAY_CALL = 0x0000000000000000000000000000000000000000000000000000000000090000; bytes32 constant _PERMISSION_ENCRYPT = 0x0000000000000000000000000000000000000000000000000000000000080000; bytes32 constant _PERMISSION_DECRYPT = 0x0000000000000000000000000000000000000000000000000000000000100000; bytes32 constant _PERMISSION_SIGN = 0x0000000000000000000000000000000000000000000000000000000000200000; // All Permissions currently exclude REENTRANCY, DELEGATECALL and SUPER_DELEGATECALL for security -bytes32 constant ALL_REGULAR_PERMISSIONS = 0x00000000000000000000000000000000000000000000000000000000003f3f7f; +bytes32 constant ALL_REGULAR_PERMISSIONS = 0x0000000000000000000000000000000000000000000000000000000000483f7f; // AllowedCalls types bytes4 constant _ALLOWEDCALLS_TRANSFERVALUE = 0x00000001; // 0000 0001 diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index b4ce275b2..9eb220f57 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -23,6 +23,9 @@ import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; import {LSP6SetDataModule} from "./LSP6Modules/LSP6SetDataModule.sol"; import {LSP6ExecuteModule} from "./LSP6Modules/LSP6ExecuteModule.sol"; +import { + LSP6ExecuteRelayCallModule +} from "./LSP6Modules/LSP6ExecuteRelayCallModule.sol"; import {LSP6OwnershipModule} from "./LSP6Modules/LSP6OwnershipModule.sol"; import { LSP25MultiChannelNonce @@ -83,6 +86,7 @@ abstract contract LSP6KeyManagerCore is ILSP25, LSP6SetDataModule, LSP6ExecuteModule, + LSP6ExecuteRelayCallModule, LSP6OwnershipModule, LSP25MultiChannelNonce { @@ -334,7 +338,7 @@ abstract contract LSP6KeyManagerCore is if (msg.sender == callee) { bool isReentrantCall = _nonReentrantBefore(isSetData, caller); - _verifyPermissions(callee, caller, msgValue, data); + _verifyPermissions(callee, caller, msgValue, false, data); emit PermissionsVerified(caller, msgValue, bytes4(data)); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function @@ -356,7 +360,7 @@ abstract contract LSP6KeyManagerCore is ); } - _verifyPermissions(callee, caller, msgValue, data); + _verifyPermissions(callee, caller, msgValue, false, data); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function return @@ -399,7 +403,7 @@ abstract contract LSP6KeyManagerCore is bool isReentrantCall = _nonReentrantBefore(isSetData, msg.sender); - _verifyPermissions(_target, msg.sender, msgValue, payload); + _verifyPermissions(_target, msg.sender, msgValue, false, payload); emit PermissionsVerified(msg.sender, msgValue, bytes4(payload)); bytes memory result = _executePayload(msgValue, payload); @@ -464,7 +468,7 @@ abstract contract LSP6KeyManagerCore is bool isReentrantCall = _nonReentrantBefore(isSetData, signer); - _verifyPermissions(_target, signer, msgValue, payload); + _verifyPermissions(_target, signer, msgValue, true, payload); emit PermissionsVerified(signer, msgValue, bytes4(payload)); bytes memory result = _executePayload(msgValue, payload); @@ -507,11 +511,19 @@ abstract contract LSP6KeyManagerCore is address callee, address from, uint256 msgValue, + bool isRelayedCall, bytes calldata payload ) internal view virtual { bytes32 permissions = ERC725Y(callee).getPermissionsFor(from); if (permissions == bytes32(0)) revert NoPermissionsSet(from); + if (isRelayedCall) { + LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission( + from, + permissions + ); + } + bytes4 erc725Function = bytes4(payload); // ERC725Y.setData(bytes32,bytes) diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol new file mode 100644 index 000000000..74ce8c9d5 --- /dev/null +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.5; + +// libraries +import {LSP6Utils} from "../LSP6Utils.sol"; + +// constants +import {_PERMISSION_EXECUTE_RELAY_CALL} from "../LSP6Constants.sol"; + +// errors +import {NotAuthorised} from "../LSP6Errors.sol"; + +abstract contract LSP6ExecuteRelayCallModule { + function _verifyExecuteRelayCallPermission( + address controllerAddress, + bytes32 controllerPermissions + ) internal pure { + if ( + !LSP6Utils.hasPermission( + controllerPermissions, + _PERMISSION_EXECUTE_RELAY_CALL + ) + ) { + string memory permissionErrorString = LSP6Utils.getPermissionName( + _PERMISSION_EXECUTE_RELAY_CALL + ); + revert NotAuthorised(controllerAddress, permissionErrorString); + } + } +} diff --git a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol index 3f997e0ff..d3125f966 100644 --- a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol +++ b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol @@ -118,7 +118,7 @@ contract KeyManagerInternalTester is LSP6KeyManager { uint256 msgValue, bytes calldata payload ) public { - super._verifyPermissions(_target, from, msgValue, payload); + super._verifyPermissions(_target, from, msgValue, false, payload); // This event is emitted just for a sake of not marking this function as `view`, // as Hardhat has a bug that does not catch error that occured from failed `abi.decode` From 234fb6e87cb5862fdbc51de90a30c46cf7e8ca9b Mon Sep 17 00:00:00 2001 From: CJ42 Date: Wed, 27 Sep 2023 09:41:32 +0100 Subject: [PATCH 10/23] refactor: move `EXECUTE_RELAY_CALL` permission as last bit in permissions range Co-authored-by: Skima Harvey --- constants.ts | 48 +++++++++---------- contracts/LSP6KeyManager/LSP6Constants.sol | 4 +- .../Relay/ExecuteRelayCall.test.ts | 14 ++++-- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/constants.ts b/constants.ts index 4c98c5af6..9c25e64d2 100644 --- a/constants.ts +++ b/constants.ts @@ -271,7 +271,7 @@ export const CALLTYPE = { /** * @dev `bytes32` hex value for all the LSP6 permissions excluding REENTRANCY, DELEGATECALL and SUPER_DELEGATECALL for security (these should be set manually) */ -export const ALL_PERMISSIONS = '0x0000000000000000000000000000000000000000000000000000000000483f7f'; +export const ALL_PERMISSIONS = '0x00000000000000000000000000000000000000000000000000000000007f3f7f'; export type LSP6PermissionName = keyof typeof PERMISSIONS; @@ -280,29 +280,29 @@ export type LSP6PermissionName = keyof typeof PERMISSIONS; */ // prettier-ignore export const PERMISSIONS = { - CHANGEOWNER: "0x0000000000000000000000000000000000000000000000000000000000000001", - ADDCONTROLLER: "0x0000000000000000000000000000000000000000000000000000000000000002", - EDITPERMISSIONS: "0x0000000000000000000000000000000000000000000000000000000000000004", - ADDEXTENSIONS: "0x0000000000000000000000000000000000000000000000000000000000000008", - CHANGEEXTENSIONS: "0x0000000000000000000000000000000000000000000000000000000000000010", - ADDUNIVERSALRECEIVERDELEGATE: "0x0000000000000000000000000000000000000000000000000000000000000020", - CHANGEUNIVERSALRECEIVERDELEGATE: "0x0000000000000000000000000000000000000000000000000000000000000040", - REENTRANCY: "0x0000000000000000000000000000000000000000000000000000000000000080", - SUPER_TRANSFERVALUE: "0x0000000000000000000000000000000000000000000000000000000000000100", - TRANSFERVALUE: "0x0000000000000000000000000000000000000000000000000000000000000200", - SUPER_CALL: "0x0000000000000000000000000000000000000000000000000000000000000400", - CALL: "0x0000000000000000000000000000000000000000000000000000000000000800", - SUPER_STATICCALL: "0x0000000000000000000000000000000000000000000000000000000000001000", - STATICCALL: "0x0000000000000000000000000000000000000000000000000000000000002000", - SUPER_DELEGATECALL: "0x0000000000000000000000000000000000000000000000000000000000004000", - DELEGATECALL: "0x0000000000000000000000000000000000000000000000000000000000008000", - DEPLOY: "0x0000000000000000000000000000000000000000000000000000000000010000", - SUPER_SETDATA: "0x0000000000000000000000000000000000000000000000000000000000020000", - EXECUTE_RELAY_CALL: '0x0000000000000000000000000000000000000000000000000000000000090000', - SETDATA: "0x0000000000000000000000000000000000000000000000000000000000040000", - ENCRYPT: "0x0000000000000000000000000000000000000000000000000000000000080000", - DECRYPT: "0x0000000000000000000000000000000000000000000000000000000000100000", - SIGN: "0x0000000000000000000000000000000000000000000000000000000000200000", + CHANGEOWNER: '0x0000000000000000000000000000000000000000000000000000000000000001', // .... .... .... .... .... 0001 + ADDCONTROLLER: '0x0000000000000000000000000000000000000000000000000000000000000002', // .... .... .... .... .... 0010 + EDITPERMISSIONS: '0x0000000000000000000000000000000000000000000000000000000000000004', // .... .... .... .... .... 0100 + ADDEXTENSIONS: '0x0000000000000000000000000000000000000000000000000000000000000008', // .... .... .... .... .... 1000 + CHANGEEXTENSIONS: '0x0000000000000000000000000000000000000000000000000000000000000010', // .... .... .... .... 0001 0000 + ADDUNIVERSALRECEIVERDELEGATE: '0x0000000000000000000000000000000000000000000000000000000000000020', // .... .... .... .... 0010 0000 + CHANGEUNIVERSALRECEIVERDELEGATE: '0x0000000000000000000000000000000000000000000000000000000000000040', // .... .... .... .... 0100 0000 + REENTRANCY: '0x0000000000000000000000000000000000000000000000000000000000000080', // .... .... .... .... 1000 0000 + SUPER_TRANSFERVALUE: '0x0000000000000000000000000000000000000000000000000000000000000100', // .... .... .... 0001 0000 0000 + TRANSFERVALUE: '0x0000000000000000000000000000000000000000000000000000000000000200', // .... .... .... 0010 0000 0000 + SUPER_CALL: '0x0000000000000000000000000000000000000000000000000000000000000400', // .... .... .... 0100 0000 0000 + CALL: '0x0000000000000000000000000000000000000000000000000000000000000800', // .... .... .... 1000 0000 0000 + SUPER_STATICCALL: '0x0000000000000000000000000000000000000000000000000000000000001000', // .... .... 0001 0000 0000 0000 + STATICCALL: '0x0000000000000000000000000000000000000000000000000000000000002000', // .... .... 0010 0000 0000 0000 + SUPER_DELEGATECALL: '0x0000000000000000000000000000000000000000000000000000000000004000', // .... .... 0100 0000 0000 0000 + DELEGATECALL: '0x0000000000000000000000000000000000000000000000000000000000008000', // .... .... 1000 0000 0000 0000 + DEPLOY: '0x0000000000000000000000000000000000000000000000000000000000010000', // .... 0001 0000 0000 0000 0000 + SUPER_SETDATA: '0x0000000000000000000000000000000000000000000000000000000000020000', // .... 0010 0000 0000 0000 0000 + SETDATA: '0x0000000000000000000000000000000000000000000000000000000000040000', // .... 0100 0000 0000 0000 0000 + ENCRYPT: '0x0000000000000000000000000000000000000000000000000000000000080000', // .... 1000 0000 0000 0000 0000 + DECRYPT: '0x0000000000000000000000000000000000000000000000000000000000100000', // 0001 0000 0000 0000 0000 0000 + SIGN: '0x0000000000000000000000000000000000000000000000000000000000200000', // 0010 0000 0000 0000 0000 0000 + EXECUTE_RELAY_CALL: '0x0000000000000000000000000000000000000000000000000000000000400000', // 0100 0000 0000 0000 0000 0000 } /** diff --git a/contracts/LSP6KeyManager/LSP6Constants.sol b/contracts/LSP6KeyManager/LSP6Constants.sol index 3a9dd01f4..a7d26ede9 100644 --- a/contracts/LSP6KeyManager/LSP6Constants.sol +++ b/contracts/LSP6KeyManager/LSP6Constants.sol @@ -49,13 +49,13 @@ bytes32 constant _PERMISSION_DELEGATECALL = 0x000000000000000 bytes32 constant _PERMISSION_DEPLOY = 0x0000000000000000000000000000000000000000000000000000000000010000; bytes32 constant _PERMISSION_SUPER_SETDATA = 0x0000000000000000000000000000000000000000000000000000000000020000; bytes32 constant _PERMISSION_SETDATA = 0x0000000000000000000000000000000000000000000000000000000000040000; -bytes32 constant _PERMISSION_EXECUTE_RELAY_CALL = 0x0000000000000000000000000000000000000000000000000000000000090000; bytes32 constant _PERMISSION_ENCRYPT = 0x0000000000000000000000000000000000000000000000000000000000080000; bytes32 constant _PERMISSION_DECRYPT = 0x0000000000000000000000000000000000000000000000000000000000100000; bytes32 constant _PERMISSION_SIGN = 0x0000000000000000000000000000000000000000000000000000000000200000; +bytes32 constant _PERMISSION_EXECUTE_RELAY_CALL = 0x0000000000000000000000000000000000000000000000000000000000400000; // All Permissions currently exclude REENTRANCY, DELEGATECALL and SUPER_DELEGATECALL for security -bytes32 constant ALL_REGULAR_PERMISSIONS = 0x0000000000000000000000000000000000000000000000000000000000483f7f; +bytes32 constant ALL_REGULAR_PERMISSIONS = 0x00000000000000000000000000000000000000000000000000000000007f3f7f; // AllowedCalls types bytes4 constant _ALLOWEDCALLS_TRANSFERVALUE = 0x00000001; // 0000 0001 diff --git a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts index bf3efda48..9f3d374ce 100644 --- a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts +++ b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts @@ -70,7 +70,11 @@ export const shouldBehaveLikeExecuteRelayCall = ( const permissionsValues = [ ALL_PERMISSIONS, - combinePermissions(PERMISSIONS.CALL, PERMISSIONS.TRANSFERVALUE), + combinePermissions( + PERMISSIONS.CALL, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), combineAllowedCalls( [ combineCallTypes(CALLTYPE.VALUE, CALLTYPE.CALL), @@ -80,7 +84,11 @@ export const shouldBehaveLikeExecuteRelayCall = ( ['0xffffffff', '0xffffffff'], ['0xffffffff', '0xffffffff'], ), - combinePermissions(PERMISSIONS.CALL, PERMISSIONS.TRANSFERVALUE), + combinePermissions( + PERMISSIONS.CALL, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), ]; await setupKeyManager(context, permissionKeys, permissionsValues); @@ -1160,7 +1168,7 @@ export const shouldBehaveLikeExecuteRelayCall = ( ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + minter.address.substring(2), ], [ - PERMISSIONS.CALL, + combinePermissions(PERMISSIONS.CALL, PERMISSIONS.EXECUTE_RELAY_CALL), combineAllowedCalls( [CALLTYPE.CALL], [tokenContract.address], From a0545ab811d7ab7f295f25a2363fe62734fee25b Mon Sep 17 00:00:00 2001 From: Maxime Date: Wed, 27 Sep 2023 14:33:07 +0200 Subject: [PATCH 11/23] test: fix lsp6 tests --- .../LSP6/Interactions/PermissionCall.test.ts | 16 ++++---- .../Interactions/AllowedFunctions.test.ts | 10 ++--- .../Interactions/PermissionCall.test.ts | 38 ++++++++++--------- .../Interactions/PermissionDeploy.test.ts | 16 ++++++-- .../Relay/MultiChannelNonce.test.ts | 4 +- 5 files changed, 48 insertions(+), 36 deletions(-) diff --git a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts index 8bad4c69e..4346ff2b6 100644 --- a/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts +++ b/tests/LSP20CallVerification/LSP6/Interactions/PermissionCall.test.ts @@ -99,11 +99,11 @@ export const shouldBehaveLikePermissionCall = ( ); const permissionsValues = [ - PERMISSIONS.SIGN, - PERMISSIONS.SIGN, - PERMISSIONS.CALL, - PERMISSIONS.CALL, - PERMISSIONS.SUPER_CALL, + combinePermissions(PERMISSIONS.SIGN, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.SIGN, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.CALL, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.CALL, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.SUPER_CALL, PERMISSIONS.EXECUTE_RELAY_CALL), allowedCallsValues, allowedCallsValues, ]; @@ -343,9 +343,9 @@ export const shouldBehaveLikePermissionCall = ( const permissionsValues = [ ALL_PERMISSIONS, - PERMISSIONS.CALL, - PERMISSIONS.CALL, - PERMISSIONS.SETDATA, + combinePermissions(PERMISSIONS.CALL, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.CALL, PERMISSIONS.EXECUTE_RELAY_CALL), + combinePermissions(PERMISSIONS.SETDATA, PERMISSIONS.EXECUTE_RELAY_CALL), combineAllowedCalls( [CALLTYPE.CALL], [targetContract.address], diff --git a/tests/LSP6KeyManager/Interactions/AllowedFunctions.test.ts b/tests/LSP6KeyManager/Interactions/AllowedFunctions.test.ts index 68a455f38..d99dc9ced 100644 --- a/tests/LSP6KeyManager/Interactions/AllowedFunctions.test.ts +++ b/tests/LSP6KeyManager/Interactions/AllowedFunctions.test.ts @@ -27,7 +27,7 @@ import { LSP6TestContext } from '../../utils/context'; import { setupKeyManager } from '../../utils/fixtures'; // helpers -import { LOCAL_PRIVATE_KEYS, combineAllowedCalls } from '../../utils/helpers'; +import { LOCAL_PRIVATE_KEYS, combineAllowedCalls, combinePermissions } from '../../utils/helpers'; export const shouldBehaveLikeAllowedFunctions = (buildContext: () => Promise) => { let context: LSP6TestContext; @@ -55,8 +55,8 @@ export const shouldBehaveLikeAllowedFunctions = (buildContext: () => Promise Promise Promise Promise) => { let context: LSP6TestContext; @@ -42,7 +42,7 @@ export const shouldBehaveLikeMultiChannelNonce = (buildContext: () => Promise Date: Sat, 30 Sep 2023 18:26:08 +0100 Subject: [PATCH 12/23] feat!: add LSP6ExecuteRelayCallModule to LSP6 --- contracts/LSP6KeyManager/LSP6KeyManagerCore.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 9eb220f57..4b75f33ba 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -517,13 +517,6 @@ abstract contract LSP6KeyManagerCore is bytes32 permissions = ERC725Y(callee).getPermissionsFor(from); if (permissions == bytes32(0)) revert NoPermissionsSet(from); - if (isRelayedCall) { - LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission( - from, - permissions - ); - } - bytes4 erc725Function = bytes4(payload); // ERC725Y.setData(bytes32,bytes) @@ -582,6 +575,13 @@ abstract contract LSP6KeyManagerCore is } else { revert InvalidERC725Function(erc725Function); } + + if (isRelayedCall) { + LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission( + from, + permissions + ); + } } /** From e72566b386f48e365c74c670b94005bfe70036c0 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sat, 30 Sep 2023 18:29:52 +0100 Subject: [PATCH 13/23] test: add EXECUTE_RELAY_CALL test --- .../LSP6ExecuteRelayCallModule.sol | 5 +- .../Relay/ExecuteRelayCall.test.ts | 76 ++++++++++++++++++- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol index 74ce8c9d5..4e0fd9447 100644 --- a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteRelayCallModule.sol @@ -21,10 +21,7 @@ abstract contract LSP6ExecuteRelayCallModule { _PERMISSION_EXECUTE_RELAY_CALL ) ) { - string memory permissionErrorString = LSP6Utils.getPermissionName( - _PERMISSION_EXECUTE_RELAY_CALL - ); - revert NotAuthorised(controllerAddress, permissionErrorString); + revert NotAuthorised(controllerAddress, "EXECUTE_RELAY_CALL"); } } } diff --git a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts index 9f3d374ce..e86708540 100644 --- a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts +++ b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts @@ -41,11 +41,13 @@ export const shouldBehaveLikeExecuteRelayCall = ( ) => { let context: LSP6TestContext; - describe('`executeRelayCall(..)`', () => { + describe.only('`executeRelayCall(..)`', () => { let signer: SignerWithAddress, relayer: SignerWithAddress, random: SignerWithAddress, - signerNoAllowedCalls: SignerWithAddress; + signerNoAllowedCalls: SignerWithAddress, + signerWithoutExecuteRelayCall: SignerWithAddress; + const signerPrivateKey = LOCAL_PRIVATE_KEYS.ACCOUNT1; let targetContract: TargetContract; @@ -57,6 +59,7 @@ export const shouldBehaveLikeExecuteRelayCall = ( relayer = context.accounts[2]; signerNoAllowedCalls = context.accounts[3]; random = context.accounts[4]; + signerWithoutExecuteRelayCall = context.accounts[5]; targetContract = await new TargetContract__factory(context.accounts[0]).deploy(); @@ -66,8 +69,19 @@ export const shouldBehaveLikeExecuteRelayCall = ( ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] + signer.address.substring(2), ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + signerNoAllowedCalls.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + signerWithoutExecuteRelayCall.address.substring(2), ]; + const allPermissionsWithoutExecuteRelayCall = ethers.utils.hexZeroPad( + BigNumber.from(ALL_PERMISSIONS) + .sub(BigNumber.from(PERMISSIONS.EXECUTE_RELAY_CALL)) + .toHexString(), + 32, + ); + + console.log(allPermissionsWithoutExecuteRelayCall); + const permissionsValues = [ ALL_PERMISSIONS, combinePermissions( @@ -89,11 +103,69 @@ export const shouldBehaveLikeExecuteRelayCall = ( PERMISSIONS.TRANSFERVALUE, PERMISSIONS.EXECUTE_RELAY_CALL, ), + allPermissionsWithoutExecuteRelayCall, ]; await setupKeyManager(context, permissionKeys, permissionsValues); }); + describe.only('When signer does not have EXECUTE_RELAY_CALL permission', () => { + it('should revert', async () => { + const executeRelayCallPayload = context.universalProfile.interface.encodeFunctionData( + 'execute', + [OPERATION_TYPES.CALL, random.address, 0, '0x'], + ); + + const latestNonce = await context.keyManager.callStatic.getNonce( + signerWithoutExecuteRelayCall.address, + 0, + ); + const validityTimestamps = 0; + + const signedMessageParams = { + lsp25Version: LSP25_VERSION, + chainId: 31337, // HARDHAT_CHAINID + nonce: latestNonce, + validityTimestamps, + msgValue: 0, + payload: executeRelayCallPayload, + }; + + const encodedMessage = ethers.utils.solidityPack( + ['uint256', 'uint256', 'uint256', 'uint256', 'uint256', 'bytes'], + [ + signedMessageParams.lsp25Version, + signedMessageParams.chainId, + signedMessageParams.nonce, + signedMessageParams.validityTimestamps, + signedMessageParams.msgValue, + signedMessageParams.payload, + ], + ); + + const eip191Signer = new EIP191Signer(); + + const { signature } = await eip191Signer.signDataWithIntendedValidator( + context.keyManager.address, + encodedMessage, + LOCAL_PRIVATE_KEYS.ACCOUNT5, + ); + + await expect( + context.keyManager + .connect(relayer) + .executeRelayCall( + signature, + signedMessageParams.nonce, + signedMessageParams.validityTimestamps, + signedMessageParams.payload, + { value: 0 }, + ), + ) + .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') + .withArgs(signerWithoutExecuteRelayCall.address, 'EXECUTE_RELY_CALL'); + }); + }); describe('When testing signed message', () => { describe('When testing msg.value', () => { describe('When sending more than the signed msg.value', () => { From ca9c9dd3060d67925a78de567a0e724ce4823325 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sat, 30 Sep 2023 18:30:27 +0100 Subject: [PATCH 14/23] test: modify reentrancy tests with EXECUTE_RELAY_CALL permission --- tests/Reentrancy/LSP20/reentrancyHelpers.ts | 70 ++++++++++----- tests/Reentrancy/LSP6/LSP6Reentrancy.test.ts | 6 +- tests/Reentrancy/LSP6/reentrancyHelpers.ts | 92 +++++++++++++------- 3 files changed, 115 insertions(+), 53 deletions(-) diff --git a/tests/Reentrancy/LSP20/reentrancyHelpers.ts b/tests/Reentrancy/LSP20/reentrancyHelpers.ts index 1c6598c84..63f67332e 100644 --- a/tests/Reentrancy/LSP20/reentrancyHelpers.ts +++ b/tests/Reentrancy/LSP20/reentrancyHelpers.ts @@ -71,7 +71,7 @@ export const transferValueTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, allowedCalls: false, missingPermission: 'REENTRANCY', }, @@ -83,38 +83,46 @@ export const transferValueTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedCalls: false, missingPermission: 'TRANSFERVALUE', }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedCalls: true, missingPermission: 'TRANSFERVALUE', }, { permissionsText: 'TRANSFERVALUE', - permissions: PERMISSIONS.TRANSFERVALUE, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.TRANSFERVALUE), allowedCalls: false, missingPermission: 'REENTRANCY', }, { permissionsText: 'TRANSFERVALUE', - permissions: PERMISSIONS.TRANSFERVALUE, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.TRANSFERVALUE), allowedCalls: true, missingPermission: 'REENTRANCY', }, ], NoCallsAllowed: { permissionsText: 'REENTRANCY, TRANSFERVALUE', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.TRANSFERVALUE), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedCalls: false, missingPermission: '', }, ValidCase: { permissionsText: 'REENTRANCY, TRANSFERVALUE', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.TRANSFERVALUE), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedCalls: true, missingPermission: '', }, @@ -124,7 +132,7 @@ export const setDataTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, allowedERC725YDataKeys: false, missingPermission: 'REENTRANCY', }, @@ -136,38 +144,46 @@ export const setDataTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedERC725YDataKeys: false, missingPermission: 'SETDATA', }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedERC725YDataKeys: true, missingPermission: 'SETDATA', }, { permissionsText: 'SETDATA', - permissions: PERMISSIONS.SETDATA, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.SETDATA), allowedERC725YDataKeys: false, missingPermission: 'REENTRANCY', }, { permissionsText: 'SETDATA', - permissions: PERMISSIONS.SETDATA, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.SETDATA), allowedERC725YDataKeys: true, missingPermission: 'REENTRANCY', }, ], NoERC725YDataKeysAllowed: { permissionsText: 'REENTRANCY, SETDATA', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.SETDATA), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.SETDATA, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedERC725YDataKeys: false, missingPermission: '', }, ValidCase: { permissionsText: 'REENTRANCY, SETDATA', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.SETDATA), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.SETDATA, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedERC725YDataKeys: true, missingPermission: '', }, @@ -177,7 +193,7 @@ export const addPermissionsTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -187,18 +203,22 @@ export const addPermissionsTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'ADDCONTROLLER', }, { permissionsText: 'ADDCONTROLLER', - permissions: PERMISSIONS.ADDCONTROLLER, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.ADDCONTROLLER), missingPermission: 'REENTRANCY', }, ], ValidCase: { permissionsText: 'REENTRANCY, ADDCONTROLLER', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.ADDCONTROLLER), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.ADDCONTROLLER, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), missingPermission: '', }, }; @@ -207,7 +227,7 @@ export const editPermissionsTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -217,18 +237,22 @@ export const editPermissionsTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'EDITPERMISSIONS', }, { permissionsText: 'EDITPERMISSIONS', - permissions: PERMISSIONS.EDITPERMISSIONS, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.EDITPERMISSIONS), missingPermission: 'REENTRANCY', }, ], ValidCase: { permissionsText: 'REENTRANCY, EDITPERMISSIONS', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.EDITPERMISSIONS), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.EDITPERMISSIONS, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), missingPermission: '', }, }; @@ -261,6 +285,7 @@ export const addUniversalReceiverDelegateTestCases = { permissions: combinePermissions( PERMISSIONS.REENTRANCY, PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + PERMISSIONS.EXECUTE_RELAY_CALL, ), missingPermission: '', }, @@ -294,6 +319,7 @@ export const changeUniversalReceiverDelegateTestCases = { permissions: combinePermissions( PERMISSIONS.REENTRANCY, PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + PERMISSIONS.EXECUTE_RELAY_CALL, ), missingPermission: '', }, diff --git a/tests/Reentrancy/LSP6/LSP6Reentrancy.test.ts b/tests/Reentrancy/LSP6/LSP6Reentrancy.test.ts index 0ec0943fe..13337d17b 100644 --- a/tests/Reentrancy/LSP6/LSP6Reentrancy.test.ts +++ b/tests/Reentrancy/LSP6/LSP6Reentrancy.test.ts @@ -75,7 +75,11 @@ export const shouldBehaveLikeLSP6ReentrancyScenarios = ( const permissionValues = [ ALL_PERMISSIONS, - combinePermissions(PERMISSIONS.CALL, PERMISSIONS.TRANSFERVALUE), + combinePermissions( + PERMISSIONS.CALL, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), combineAllowedCalls( // TODO: test reentrancy against the bits for the allowed calls [ diff --git a/tests/Reentrancy/LSP6/reentrancyHelpers.ts b/tests/Reentrancy/LSP6/reentrancyHelpers.ts index 7f6fa70e5..2104afa88 100644 --- a/tests/Reentrancy/LSP6/reentrancyHelpers.ts +++ b/tests/Reentrancy/LSP6/reentrancyHelpers.ts @@ -71,7 +71,7 @@ export const transferValueTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, allowedCalls: false, missingPermission: 'REENTRANCY', }, @@ -83,38 +83,46 @@ export const transferValueTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedCalls: false, missingPermission: 'TRANSFERVALUE', }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedCalls: true, missingPermission: 'TRANSFERVALUE', }, { permissionsText: 'TRANSFERVALUE', - permissions: PERMISSIONS.TRANSFERVALUE, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.TRANSFERVALUE), allowedCalls: false, missingPermission: 'REENTRANCY', }, { permissionsText: 'TRANSFERVALUE', - permissions: PERMISSIONS.TRANSFERVALUE, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.TRANSFERVALUE), allowedCalls: true, missingPermission: 'REENTRANCY', }, ], NoCallsAllowed: { permissionsText: 'REENTRANCY, TRANSFERVALUE', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.TRANSFERVALUE), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedCalls: false, missingPermission: '', }, ValidCase: { permissionsText: 'REENTRANCY, TRANSFERVALUE', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.TRANSFERVALUE), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.TRANSFERVALUE, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedCalls: true, missingPermission: '', }, @@ -124,7 +132,7 @@ export const setDataTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, allowedERC725YDataKeys: false, missingPermission: 'REENTRANCY', }, @@ -136,38 +144,46 @@ export const setDataTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedERC725YDataKeys: false, missingPermission: 'SETDATA', }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), allowedERC725YDataKeys: true, missingPermission: 'SETDATA', }, { permissionsText: 'SETDATA', - permissions: PERMISSIONS.SETDATA, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.SETDATA), allowedERC725YDataKeys: false, missingPermission: 'REENTRANCY', }, { permissionsText: 'SETDATA', - permissions: PERMISSIONS.SETDATA, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.SETDATA), allowedERC725YDataKeys: true, missingPermission: 'REENTRANCY', }, ], NoERC725YDataKeysAllowed: { permissionsText: 'REENTRANCY, SETDATA', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.SETDATA), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.SETDATA, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedERC725YDataKeys: false, missingPermission: '', }, ValidCase: { permissionsText: 'REENTRANCY, SETDATA', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.SETDATA), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.SETDATA, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), allowedERC725YDataKeys: true, missingPermission: '', }, @@ -177,7 +193,7 @@ export const addPermissionsTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -187,18 +203,22 @@ export const addPermissionsTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'ADDCONTROLLER', }, { permissionsText: 'ADDCONTROLLER', - permissions: PERMISSIONS.ADDCONTROLLER, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.ADDCONTROLLER), missingPermission: 'REENTRANCY', }, ], ValidCase: { permissionsText: 'REENTRANCY, ADDCONTROLLER', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.ADDCONTROLLER), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.ADDCONTROLLER, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), missingPermission: '', }, }; @@ -207,7 +227,7 @@ export const editPermissionsTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -217,18 +237,22 @@ export const editPermissionsTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'EDITPERMISSIONS', }, { permissionsText: 'EDITPERMISSIONS', - permissions: PERMISSIONS.EDITPERMISSIONS, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.EDITPERMISSIONS), missingPermission: 'REENTRANCY', }, ], ValidCase: { permissionsText: 'REENTRANCY, EDITPERMISSIONS', - permissions: combinePermissions(PERMISSIONS.REENTRANCY, PERMISSIONS.EDITPERMISSIONS), + permissions: combinePermissions( + PERMISSIONS.REENTRANCY, + PERMISSIONS.EDITPERMISSIONS, + PERMISSIONS.EXECUTE_RELAY_CALL, + ), missingPermission: '', }, }; @@ -237,7 +261,7 @@ export const addUniversalReceiverDelegateTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -247,12 +271,15 @@ export const addUniversalReceiverDelegateTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'ADDUNIVERSALRECEIVERDELEGATE', }, { permissionsText: 'ADDUNIVERSALRECEIVERDELEGATE', - permissions: PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + permissions: combinePermissions( + PERMISSIONS.EXECUTE_RELAY_CALL, + PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + ), missingPermission: 'REENTRANCY', }, ], @@ -261,6 +288,7 @@ export const addUniversalReceiverDelegateTestCases = { permissions: combinePermissions( PERMISSIONS.REENTRANCY, PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + PERMISSIONS.EXECUTE_RELAY_CALL, ), missingPermission: '', }, @@ -270,7 +298,7 @@ export const changeUniversalReceiverDelegateTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -280,12 +308,15 @@ export const changeUniversalReceiverDelegateTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'CHANGEUNIVERSALRECEIVERDELEGATE', }, { permissionsText: 'CHANGEUNIVERSALRECEIVERDELEGATE', - permissions: PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + permissions: combinePermissions( + PERMISSIONS.EXECUTE_RELAY_CALL, + PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + ), missingPermission: 'REENTRANCY', }, ], @@ -294,6 +325,7 @@ export const changeUniversalReceiverDelegateTestCases = { permissions: combinePermissions( PERMISSIONS.REENTRANCY, PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + PERMISSIONS.EXECUTE_RELAY_CALL, ), missingPermission: '', }, @@ -327,7 +359,7 @@ export const buildReentrancyContext = async (context: LSP6TestContext) => { const permissionValues = [ ALL_PERMISSIONS, - PERMISSIONS.CALL, + combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.CALL), combineAllowedCalls( // allow controller to call the 3 x addresses listed below [CALLTYPE.CALL, CALLTYPE.CALL, CALLTYPE.CALL], @@ -335,7 +367,7 @@ export const buildReentrancyContext = async (context: LSP6TestContext) => { ['0xffffffff', '0xffffffff', '0xffffffff'], ['0xffffffff', '0xffffffff', '0xffffffff'], ), - PERMISSIONS.CALL, + combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.CALL), combineAllowedCalls( // allow controller to call the 3 x addresses listed below [CALLTYPE.CALL, CALLTYPE.CALL, CALLTYPE.CALL], From 873687352b3304dbf2eaf6239872c8951a520037 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sat, 30 Sep 2023 19:00:21 +0100 Subject: [PATCH 15/23] refactor: use uint8 for reentrancyStatus --- contracts/LSP6KeyManager/LSP6KeyManagerCore.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 97da026d6..57069c7ac 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -335,7 +335,11 @@ abstract contract LSP6KeyManagerCore is // If target is invoking the verification, emit the event and change the reentrancy guard if (msg.sender == callee) { - bool isReentrantCall = _nonReentrantBefore(isSetData, caller); + uint8 reentrancyStatus = _nonReentrantBefore( + targetContract, + isSetData, + caller + ); _verifyPermissions(callee, caller, msgValue, false, data); emit PermissionsVerified(caller, msgValue, bytes4(data)); From 550b6082a77e50aad8d77db489dc7f43a54f01c4 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sat, 30 Sep 2023 19:21:06 +0100 Subject: [PATCH 16/23] docs: build lsp6 docs --- .../LSP6KeyManager/LSP6KeyManager.md | 27 ++++++++++++++----- .../Relay/ExecuteRelayCall.test.ts | 8 +++--- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index 53a82ea0e..53fd9aae9 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -36,7 +36,7 @@ When marked as 'public', a method can be called both externally and internally, constructor(address target_); ``` -_Deploying a LSP6KeyManager linked to the contract at address `target_`.\_ +_Deploying a LSP6KeyManager linked to the contract at address `target_`._ Deploy a Key Manager and set the `target_` address in the contract storage, making this Key Manager linked to this `target_` contract. @@ -943,6 +943,17 @@ function _isAllowedCallType(
+### \_verifyExecuteRelayCallPermission + +```solidity +function _verifyExecuteRelayCallPermission( + address controllerAddress, + bytes32 controllerPermissions +) internal pure; +``` + +
+ ### \_verifyOwnershipPermissions ```solidity @@ -1157,6 +1168,7 @@ function _verifyPermissions( address callee, address from, uint256 msgValue, + bool isRelayedCall, bytes payload ) internal view; ``` @@ -1165,12 +1177,13 @@ Verify if the `from` address is allowed to execute the `payload` on the [`target #### Parameters -| Name | Type | Description | -| ---------- | :-------: | ------------------------------------------------------------------- | -| `callee` | `address` | - | -| `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. | -| `msgValue` | `uint256` | - | -| `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. | +| Name | Type | Description | +| --------------- | :-------: | ------------------------------------------------------------------- | +| `callee` | `address` | - | +| `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. | +| `msgValue` | `uint256` | - | +| `isRelayedCall` | `bool` | - | +| `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. |
diff --git a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts index bf8447f11..29a71b9cc 100644 --- a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts +++ b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts @@ -41,7 +41,7 @@ export const shouldBehaveLikeExecuteRelayCall = ( ) => { let context: LSP6TestContext; - describe.only('`executeRelayCall(..)`', () => { + describe('`executeRelayCall(..)`', () => { let signer: SignerWithAddress, relayer: SignerWithAddress, random: SignerWithAddress, @@ -80,8 +80,6 @@ export const shouldBehaveLikeExecuteRelayCall = ( 32, ); - console.log(allPermissionsWithoutExecuteRelayCall); - const permissionsValues = [ ALL_PERMISSIONS, combinePermissions( @@ -108,7 +106,7 @@ export const shouldBehaveLikeExecuteRelayCall = ( await setupKeyManager(context, permissionKeys, permissionsValues); }); - describe.only('When signer does not have EXECUTE_RELAY_CALL permission', () => { + describe('When signer does not have EXECUTE_RELAY_CALL permission', () => { it('should revert', async () => { const executeRelayCallPayload = context.universalProfile.interface.encodeFunctionData( 'execute', @@ -163,7 +161,7 @@ export const shouldBehaveLikeExecuteRelayCall = ( ), ) .to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised') - .withArgs(signerWithoutExecuteRelayCall.address, 'EXECUTE_RELY_CALL'); + .withArgs(signerWithoutExecuteRelayCall.address, 'EXECUTE_RELAY_CALL'); }); }); describe('When testing signed message', () => { From 09c283e391fd3a16711642a586938bdeab77acd0 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sun, 1 Oct 2023 09:21:12 +0100 Subject: [PATCH 17/23] test: fix reentrancy test --- contracts/LSP6KeyManager/LSP6KeyManagerCore.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 57069c7ac..a113a3eb7 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -401,13 +401,14 @@ abstract contract LSP6KeyManagerCore is address targetContract = _target; - _verifyPermissions(_target, msg.sender, msgValue, false, payload); uint8 reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, msg.sender ); + _verifyPermissions(_target, msg.sender, msgValue, false, payload); + emit PermissionsVerified(msg.sender, msgValue, bytes4(payload)); bytes memory result = _executePayload( From 32ae896fd12514d0e401f6b4a450264b27288c8e Mon Sep 17 00:00:00 2001 From: Maxime Date: Sun, 1 Oct 2023 14:24:07 +0100 Subject: [PATCH 18/23] refactor: use mapping for reentrancyStatus --- contracts/LSP6KeyManager/LSP6KeyManager.sol | 2 +- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 32 ++++++++++++------- .../LSP6KeyManagerInitAbstract.sol | 2 +- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManager.sol b/contracts/LSP6KeyManager/LSP6KeyManager.sol index d6c6d75d5..8bc1811dc 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManager.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManager.sol @@ -21,6 +21,6 @@ contract LSP6KeyManager is LSP6KeyManagerCore { constructor(address target_) { if (target_ == address(0)) revert InvalidLSP6Target(); _target = target_; - _setupLSP6ReentrancyGuard(); + _setupLSP6ReentrancyGuard(target_); } } diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index a113a3eb7..9276674d3 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -98,7 +98,7 @@ abstract contract LSP6KeyManagerCore is // Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly. // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol - uint8 internal _reentrancyStatus; + mapping(address => uint8) internal _reentrancyStatus; uint8 internal constant _NOT_ENTERED = 1; uint8 internal constant _ENTERED = 2; @@ -353,7 +353,7 @@ abstract contract LSP6KeyManagerCore is /// @dev If a different address is invoking the verification, /// do not change the state or emit the event to allow read-only verification else { - uint8 reentrancyStatus = _reentrancyStatus; + uint8 reentrancyStatus = _reentrancyStatus[callee]; if (reentrancyStatus == _ENTERED) { _requirePermissions( @@ -383,7 +383,7 @@ abstract contract LSP6KeyManagerCore is // If it's the target calling, set back the reentrancy guard // to false, if not return the magic value if (msg.sender == _target) { - _nonReentrantAfter(); + _nonReentrantAfter(msg.sender); } return _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE; } @@ -407,7 +407,13 @@ abstract contract LSP6KeyManagerCore is msg.sender ); - _verifyPermissions(_target, msg.sender, msgValue, false, payload); + _verifyPermissions( + targetContract, + msg.sender, + msgValue, + false, + payload + ); emit PermissionsVerified(msg.sender, msgValue, bytes4(payload)); @@ -418,7 +424,7 @@ abstract contract LSP6KeyManagerCore is ); if (reentrancyStatus == _NOT_ENTERED && !isSetData) { - _nonReentrantAfter(); + _nonReentrantAfter(targetContract); } return result; @@ -488,7 +494,7 @@ abstract contract LSP6KeyManagerCore is ); if (reentrancyStatus == _NOT_ENTERED && !isSetData) { - _nonReentrantAfter(); + _nonReentrantAfter(targetContract); } return result; @@ -603,8 +609,10 @@ abstract contract LSP6KeyManagerCore is /** * @dev Initialise _reentrancyStatus to _NOT_ENTERED. */ - function _setupLSP6ReentrancyGuard() internal virtual { - _reentrancyStatus = 1; + function _setupLSP6ReentrancyGuard( + address targetContract + ) internal virtual { + _reentrancyStatus[targetContract] = 1; } /** @@ -617,7 +625,7 @@ abstract contract LSP6KeyManagerCore is bool isSetData, address from ) internal virtual returns (uint8 reentrancyStatus) { - reentrancyStatus = _reentrancyStatus; + reentrancyStatus = _reentrancyStatus[targetContract]; if (reentrancyStatus == _ENTERED) { // CHECK the caller has REENTRANCY permission _requirePermissions( @@ -627,7 +635,7 @@ abstract contract LSP6KeyManagerCore is ); } else { if (!isSetData) { - _reentrancyStatus = _ENTERED; + _reentrancyStatus[targetContract] = _ENTERED; } } } @@ -636,10 +644,10 @@ abstract contract LSP6KeyManagerCore is * @dev Resets the status to `_NOT_ENTERED` * Used in the end of the `nonReentrant` modifier after the method execution is terminated */ - function _nonReentrantAfter() internal virtual { + function _nonReentrantAfter(address targetContract) internal virtual { // By storing the original value once again, a refund is triggered (see // https://eips.ethereum.org/EIPS/eip-2200) - _reentrancyStatus = _NOT_ENTERED; + _reentrancyStatus[targetContract] = _NOT_ENTERED; } /** diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol b/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol index 87f44138d..73e4116dd 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol @@ -20,6 +20,6 @@ abstract contract LSP6KeyManagerInitAbstract is function _initialize(address target_) internal virtual onlyInitializing { if (target_ == address(0)) revert InvalidLSP6Target(); _target = target_; - _setupLSP6ReentrancyGuard(); + _setupLSP6ReentrancyGuard(target_); } } From 8cb66d44ef7268d35a3a252490c4ced24c53116b Mon Sep 17 00:00:00 2001 From: Maxime Date: Sun, 1 Oct 2023 14:27:50 +0100 Subject: [PATCH 19/23] refactor: rename callee to targetContract --- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 9276674d3..68544c9b5 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -323,7 +323,7 @@ abstract contract LSP6KeyManagerCore is * - `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`). */ function lsp20VerifyCall( - address callee, + address targetContract, address caller, uint256 msgValue, bytes calldata data @@ -331,17 +331,15 @@ abstract contract LSP6KeyManagerCore is bool isSetData = bytes4(data) == IERC725Y.setData.selector || bytes4(data) == IERC725Y.setDataBatch.selector; - address targetContract = _target; - // If target is invoking the verification, emit the event and change the reentrancy guard - if (msg.sender == callee) { + if (msg.sender == targetContract) { uint8 reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, caller ); - _verifyPermissions(callee, caller, msgValue, false, data); + _verifyPermissions(targetContract, caller, msgValue, false, data); emit PermissionsVerified(caller, msgValue, bytes4(data)); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function @@ -353,17 +351,17 @@ abstract contract LSP6KeyManagerCore is /// @dev If a different address is invoking the verification, /// do not change the state or emit the event to allow read-only verification else { - uint8 reentrancyStatus = _reentrancyStatus[callee]; + uint8 reentrancyStatus = _reentrancyStatus[targetContract]; if (reentrancyStatus == _ENTERED) { _requirePermissions( caller, - ERC725Y(callee).getPermissionsFor(caller), + ERC725Y(targetContract).getPermissionsFor(caller), _PERMISSION_REENTRANCY ); } - _verifyPermissions(callee, caller, msgValue, false, data); + _verifyPermissions(targetContract, caller, msgValue, false, data); // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function return @@ -484,7 +482,7 @@ abstract contract LSP6KeyManagerCore is signer ); - _verifyPermissions(_target, signer, msgValue, true, payload); + _verifyPermissions(targetContract, signer, msgValue, true, payload); emit PermissionsVerified(signer, msgValue, bytes4(payload)); bytes memory result = _executePayload( @@ -529,13 +527,13 @@ abstract contract LSP6KeyManagerCore is * @param payload The abi-encoded function call to execute on the {target} contract. */ function _verifyPermissions( - address callee, + address targetContract, address from, uint256 msgValue, bool isRelayedCall, bytes calldata payload ) internal view virtual { - bytes32 permissions = ERC725Y(callee).getPermissionsFor(from); + bytes32 permissions = ERC725Y(targetContract).getPermissionsFor(from); if (permissions == bytes32(0)) revert NoPermissionsSet(from); bytes4 erc725Function = bytes4(payload); @@ -549,7 +547,7 @@ abstract contract LSP6KeyManagerCore is ); LSP6SetDataModule._verifyCanSetData( - callee, + targetContract, from, permissions, inputKey, @@ -563,7 +561,7 @@ abstract contract LSP6KeyManagerCore is .decode(payload[4:], (bytes32[], bytes[])); LSP6SetDataModule._verifyCanSetData( - callee, + targetContract, from, permissions, inputKeys, @@ -580,7 +578,7 @@ abstract contract LSP6KeyManagerCore is ) = abi.decode(payload[4:], (uint256, address, uint256, bytes)); LSP6ExecuteModule._verifyCanExecute( - callee, + targetContract, from, permissions, operationType, From 91fcc4707f3bf739089410fecbbf66b0891128e6 Mon Sep 17 00:00:00 2001 From: Maxime Date: Sun, 1 Oct 2023 14:34:15 +0100 Subject: [PATCH 20/23] docs: update docs with targetContract --- .../LSP6KeyManager/LSP6KeyManager.md | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index 53fd9aae9..3a34b63e6 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -348,7 +348,7 @@ This function can call by any other address than the {`target`}. This allows to ```solidity function lsp20VerifyCall( - address callee, + address targetContract, address caller, uint256 msgValue, bytes data @@ -357,12 +357,12 @@ function lsp20VerifyCall( #### Parameters -| Name | Type | Description | -| ---------- | :-------: | --------------------------------------------------------------- | -| `callee` | `address` | The address of the contract that implements the LSP20 interface | -| `caller` | `address` | The address who called the function on the msg.sender | -| `msgValue` | `uint256` | - | -| `data` | `bytes` | - | +| Name | Type | Description | +| ---------------- | :-------: | ----------------------------------------------------- | +| `targetContract` | `address` | - | +| `caller` | `address` | The address who called the function on the msg.sender | +| `msgValue` | `uint256` | - | +| `data` | `bytes` | - | #### Returns @@ -1165,7 +1165,7 @@ _Execute the `payload` passed to `execute(...)` or `executeRelayCall(...)`_ ```solidity function _verifyPermissions( - address callee, + address targetContract, address from, uint256 msgValue, bool isRelayedCall, @@ -1177,20 +1177,20 @@ Verify if the `from` address is allowed to execute the `payload` on the [`target #### Parameters -| Name | Type | Description | -| --------------- | :-------: | ------------------------------------------------------------------- | -| `callee` | `address` | - | -| `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. | -| `msgValue` | `uint256` | - | -| `isRelayedCall` | `bool` | - | -| `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. | +| Name | Type | Description | +| ---------------- | :-------: | ------------------------------------------------------------------- | +| `targetContract` | `address` | - | +| `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. | +| `msgValue` | `uint256` | - | +| `isRelayedCall` | `bool` | - | +| `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. |
### \_setupLSP6ReentrancyGuard ```solidity -function _setupLSP6ReentrancyGuard() internal nonpayable; +function _setupLSP6ReentrancyGuard(address targetContract) internal nonpayable; ``` Initialise \_reentrancyStatus to \_NOT_ENTERED. @@ -1216,7 +1216,7 @@ Used in the beginning of the `nonReentrant` modifier, before the method executio ### \_nonReentrantAfter ```solidity -function _nonReentrantAfter() internal nonpayable; +function _nonReentrantAfter(address targetContract) internal nonpayable; ``` Resets the status to `_NOT_ENTERED` From 685872c09dbd47e5d5006ff4f2406b61e6afb10d Mon Sep 17 00:00:00 2001 From: maxvia87 Date: Mon, 2 Oct 2023 14:03:18 +0200 Subject: [PATCH 21/23] refactor: use uint256 for `reentrancyStatus` --- constants.ts | 6 ++-- .../ILSP20CallVerifier.sol | 2 +- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 30 +++++++++---------- .../LSP6KeyManager/LSP6KeyManager.md | 2 +- tests/Reentrancy/LSP20/reentrancyHelpers.ts | 18 +++++++---- 5 files changed, 32 insertions(+), 26 deletions(-) diff --git a/constants.ts b/constants.ts index f8e0f7271..613937e7b 100644 --- a/constants.ts +++ b/constants.ts @@ -60,10 +60,10 @@ export const ERC1271_VALUES = { */ export const LSP20_MAGIC_VALUES = { VERIFY_CALL: { - // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x01" - WITH_POST_VERIFICATION: '0x1a238001', - // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x00" + // bytes3(keccak256("lsp20VerifyCall(address,address,uint256,bytes)")) + "0x00" NO_POST_VERIFICATION: '0x1a238000', + // bytes3(keccak256("lsp20VerifyCall(address,address,uint256,bytes)")) + "0x01" + WITH_POST_VERIFICATION: '0x1a238001', }, // bytes4(keccak256("lsp20VerifyCallResult(bytes32,bytes)")) VERIFY_CALL_RESULT: '0xd3fc45d3', diff --git a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol index 90365761d..4cfb870ee 100644 --- a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol +++ b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol @@ -12,7 +12,7 @@ interface ILSP20CallVerifier { * the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should * be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`. * - * @param callee The address of the contract that implements the LSP20 interface + * @param callee The address of the contract that implements the `LSP20CallVerifier` interface * @param caller The address who called the function on the msg.sender * @param value The value sent by the caller to the function called on the msg.sender * @param receivedCalldata The receivedCalldata sent by the caller to the msg.sender diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 68544c9b5..51d483caa 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -98,9 +98,9 @@ abstract contract LSP6KeyManagerCore is // Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly. // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol - mapping(address => uint8) internal _reentrancyStatus; - uint8 internal constant _NOT_ENTERED = 1; - uint8 internal constant _ENTERED = 2; + mapping(address => uint256) internal _reentrancyStatus; + uint256 internal constant _NOT_ENTERED = 1; + uint256 internal constant _ENTERED = 2; /** * @inheritdoc ILSP6 @@ -333,7 +333,7 @@ abstract contract LSP6KeyManagerCore is // If target is invoking the verification, emit the event and change the reentrancy guard if (msg.sender == targetContract) { - uint8 reentrancyStatus = _nonReentrantBefore( + uint256 reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, caller @@ -351,7 +351,7 @@ abstract contract LSP6KeyManagerCore is /// @dev If a different address is invoking the verification, /// do not change the state or emit the event to allow read-only verification else { - uint8 reentrancyStatus = _reentrancyStatus[targetContract]; + uint256 reentrancyStatus = _reentrancyStatus[targetContract]; if (reentrancyStatus == _ENTERED) { _requirePermissions( @@ -399,7 +399,7 @@ abstract contract LSP6KeyManagerCore is address targetContract = _target; - uint8 reentrancyStatus = _nonReentrantBefore( + uint256 reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, msg.sender @@ -476,7 +476,7 @@ abstract contract LSP6KeyManagerCore is bool isSetData = bytes4(payload) == IERC725Y.setData.selector || bytes4(payload) == IERC725Y.setDataBatch.selector; - uint8 reentrancyStatus = _nonReentrantBefore( + uint256 reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, signer @@ -536,6 +536,13 @@ abstract contract LSP6KeyManagerCore is bytes32 permissions = ERC725Y(targetContract).getPermissionsFor(from); if (permissions == bytes32(0)) revert NoPermissionsSet(from); + if (isRelayedCall) { + LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission( + from, + permissions + ); + } + bytes4 erc725Function = bytes4(payload); // ERC725Y.setData(bytes32,bytes) @@ -595,13 +602,6 @@ abstract contract LSP6KeyManagerCore is } else { revert InvalidERC725Function(erc725Function); } - - if (isRelayedCall) { - LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission( - from, - permissions - ); - } } /** @@ -622,7 +622,7 @@ abstract contract LSP6KeyManagerCore is address targetContract, bool isSetData, address from - ) internal virtual returns (uint8 reentrancyStatus) { + ) internal virtual returns (uint256 reentrancyStatus) { reentrancyStatus = _reentrancyStatus[targetContract]; if (reentrancyStatus == _ENTERED) { // CHECK the caller has REENTRANCY permission diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index 3a34b63e6..f757928c4 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -1204,7 +1204,7 @@ function _nonReentrantBefore( address targetContract, bool isSetData, address from -) internal nonpayable returns (uint8 reentrancyStatus); +) internal nonpayable returns (uint256 reentrancyStatus); ``` Update the status from `_NON_ENTERED` to `_ENTERED` and checks if diff --git a/tests/Reentrancy/LSP20/reentrancyHelpers.ts b/tests/Reentrancy/LSP20/reentrancyHelpers.ts index 63f67332e..bdef32d0c 100644 --- a/tests/Reentrancy/LSP20/reentrancyHelpers.ts +++ b/tests/Reentrancy/LSP20/reentrancyHelpers.ts @@ -261,7 +261,7 @@ export const addUniversalReceiverDelegateTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -271,12 +271,15 @@ export const addUniversalReceiverDelegateTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'ADDUNIVERSALRECEIVERDELEGATE', }, { permissionsText: 'ADDUNIVERSALRECEIVERDELEGATE', - permissions: PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + permissions: combinePermissions( + PERMISSIONS.EXECUTE_RELAY_CALL, + PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE, + ), missingPermission: 'REENTRANCY', }, ], @@ -295,7 +298,7 @@ export const changeUniversalReceiverDelegateTestCases = { NotAuthorised: [ { permissionsText: 'NO Permissions', - permissions: '0x', + permissions: PERMISSIONS.EXECUTE_RELAY_CALL, missingPermission: 'REENTRANCY', }, { @@ -305,12 +308,15 @@ export const changeUniversalReceiverDelegateTestCases = { }, { permissionsText: 'REENTRANCY', - permissions: PERMISSIONS.REENTRANCY, + permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY), missingPermission: 'CHANGEUNIVERSALRECEIVERDELEGATE', }, { permissionsText: 'CHANGEUNIVERSALRECEIVERDELEGATE', - permissions: PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + permissions: combinePermissions( + PERMISSIONS.EXECUTE_RELAY_CALL, + PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE, + ), missingPermission: 'REENTRANCY', }, ], From 7ae60517c85d11c0db3195a7f4154464eaf7a62d Mon Sep 17 00:00:00 2001 From: maxvia87 Date: Mon, 2 Oct 2023 16:25:32 +0200 Subject: [PATCH 22/23] refactor: use bool for reentrancyStatus --- contracts/LSP6KeyManager/LSP6KeyManager.sol | 1 - .../LSP6KeyManager/LSP6KeyManagerCore.sol | 42 +++++++------------ .../LSP6KeyManagerInitAbstract.sol | 1 - .../LSP6KeyManager/LSP6KeyManager.md | 14 +------ 4 files changed, 18 insertions(+), 40 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManager.sol b/contracts/LSP6KeyManager/LSP6KeyManager.sol index 8bc1811dc..6e9e9cc32 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManager.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManager.sol @@ -21,6 +21,5 @@ contract LSP6KeyManager is LSP6KeyManagerCore { constructor(address target_) { if (target_ == address(0)) revert InvalidLSP6Target(); _target = target_; - _setupLSP6ReentrancyGuard(target_); } } diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 51d483caa..7e48f5975 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -98,9 +98,7 @@ abstract contract LSP6KeyManagerCore is // Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly. // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol - mapping(address => uint256) internal _reentrancyStatus; - uint256 internal constant _NOT_ENTERED = 1; - uint256 internal constant _ENTERED = 2; + mapping(address => bool) internal _reentrancyStatus; /** * @inheritdoc ILSP6 @@ -333,7 +331,7 @@ abstract contract LSP6KeyManagerCore is // If target is invoking the verification, emit the event and change the reentrancy guard if (msg.sender == targetContract) { - uint256 reentrancyStatus = _nonReentrantBefore( + bool reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, caller @@ -344,16 +342,16 @@ abstract contract LSP6KeyManagerCore is // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function return - isSetData || (reentrancyStatus == _ENTERED) + isSetData || reentrancyStatus ? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION : _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION; } /// @dev If a different address is invoking the verification, /// do not change the state or emit the event to allow read-only verification else { - uint256 reentrancyStatus = _reentrancyStatus[targetContract]; + bool reentrancyStatus = _reentrancyStatus[targetContract]; - if (reentrancyStatus == _ENTERED) { + if (reentrancyStatus) { _requirePermissions( caller, ERC725Y(targetContract).getPermissionsFor(caller), @@ -365,7 +363,7 @@ abstract contract LSP6KeyManagerCore is // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function return - isSetData || (reentrancyStatus == _ENTERED) + isSetData || reentrancyStatus ? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION : _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION; } @@ -399,7 +397,7 @@ abstract contract LSP6KeyManagerCore is address targetContract = _target; - uint256 reentrancyStatus = _nonReentrantBefore( + bool reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, msg.sender @@ -421,7 +419,7 @@ abstract contract LSP6KeyManagerCore is payload ); - if (reentrancyStatus == _NOT_ENTERED && !isSetData) { + if (reentrancyStatus && !isSetData) { _nonReentrantAfter(targetContract); } @@ -476,7 +474,7 @@ abstract contract LSP6KeyManagerCore is bool isSetData = bytes4(payload) == IERC725Y.setData.selector || bytes4(payload) == IERC725Y.setDataBatch.selector; - uint256 reentrancyStatus = _nonReentrantBefore( + bool reentrancyStatus = _nonReentrantBefore( targetContract, isSetData, signer @@ -491,7 +489,7 @@ abstract contract LSP6KeyManagerCore is payload ); - if (reentrancyStatus == _NOT_ENTERED && !isSetData) { + if (reentrancyStatus && !isSetData) { _nonReentrantAfter(targetContract); } @@ -604,15 +602,6 @@ abstract contract LSP6KeyManagerCore is } } - /** - * @dev Initialise _reentrancyStatus to _NOT_ENTERED. - */ - function _setupLSP6ReentrancyGuard( - address targetContract - ) internal virtual { - _reentrancyStatus[targetContract] = 1; - } - /** * @dev Update the status from `_NON_ENTERED` to `_ENTERED` and checks if * the status is `_ENTERED` in order to revert the call unless the caller has the REENTRANCY permission @@ -622,9 +611,10 @@ abstract contract LSP6KeyManagerCore is address targetContract, bool isSetData, address from - ) internal virtual returns (uint256 reentrancyStatus) { + ) internal virtual returns (bool reentrancyStatus) { reentrancyStatus = _reentrancyStatus[targetContract]; - if (reentrancyStatus == _ENTERED) { + + if (reentrancyStatus) { // CHECK the caller has REENTRANCY permission _requirePermissions( from, @@ -633,19 +623,19 @@ abstract contract LSP6KeyManagerCore is ); } else { if (!isSetData) { - _reentrancyStatus[targetContract] = _ENTERED; + _reentrancyStatus[targetContract] = true; } } } /** - * @dev Resets the status to `_NOT_ENTERED` + * @dev Resets the status to `false` * Used in the end of the `nonReentrant` modifier after the method execution is terminated */ function _nonReentrantAfter(address targetContract) internal virtual { // By storing the original value once again, a refund is triggered (see // https://eips.ethereum.org/EIPS/eip-2200) - _reentrancyStatus[targetContract] = _NOT_ENTERED; + _reentrancyStatus[targetContract] = false; } /** diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol b/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol index 73e4116dd..7d29da922 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerInitAbstract.sol @@ -20,6 +20,5 @@ abstract contract LSP6KeyManagerInitAbstract is function _initialize(address target_) internal virtual onlyInitializing { if (target_ == address(0)) revert InvalidLSP6Target(); _target = target_; - _setupLSP6ReentrancyGuard(target_); } } diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md index f757928c4..90eef8aea 100644 --- a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md +++ b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md @@ -1187,16 +1187,6 @@ Verify if the `from` address is allowed to execute the `payload` on the [`target
-### \_setupLSP6ReentrancyGuard - -```solidity -function _setupLSP6ReentrancyGuard(address targetContract) internal nonpayable; -``` - -Initialise \_reentrancyStatus to \_NOT_ENTERED. - -
- ### \_nonReentrantBefore ```solidity @@ -1204,7 +1194,7 @@ function _nonReentrantBefore( address targetContract, bool isSetData, address from -) internal nonpayable returns (uint256 reentrancyStatus); +) internal nonpayable returns (bool reentrancyStatus); ``` Update the status from `_NON_ENTERED` to `_ENTERED` and checks if @@ -1219,7 +1209,7 @@ Used in the beginning of the `nonReentrant` modifier, before the method executio function _nonReentrantAfter(address targetContract) internal nonpayable; ``` -Resets the status to `_NOT_ENTERED` +Resets the status to `false` Used in the end of the `nonReentrant` modifier after the method execution is terminated
From 8e9963cb72fb3b32ec461f6da8b699e7dcc3cb41 Mon Sep 17 00:00:00 2001 From: maxvia87 Date: Mon, 2 Oct 2023 16:28:23 +0200 Subject: [PATCH 23/23] reactor: remove cast to bytes1 in _verifyCall --- contracts/LSP20CallVerification/ILSP20CallVerifier.sol | 2 +- contracts/LSP20CallVerification/LSP20CallVerification.sol | 2 +- contracts/LSP6KeyManager/LSP6KeyManagerCore.sol | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol index 4cfb870ee..e550a8fa9 100644 --- a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol +++ b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol @@ -12,7 +12,7 @@ interface ILSP20CallVerifier { * the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should * be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`. * - * @param callee The address of the contract that implements the `LSP20CallVerifier` interface + * @param callee The address of the contract that implements the `LSP20CallVerification` interface * @param caller The address who called the function on the msg.sender * @param value The value sent by the caller to the function called on the msg.sender * @param receivedCalldata The receivedCalldata sent by the caller to the msg.sender diff --git a/contracts/LSP20CallVerification/LSP20CallVerification.sol b/contracts/LSP20CallVerification/LSP20CallVerification.sol index d28eb4ab8..1a864c28e 100644 --- a/contracts/LSP20CallVerification/LSP20CallVerification.sol +++ b/contracts/LSP20CallVerification/LSP20CallVerification.sol @@ -47,7 +47,7 @@ abstract contract LSP20CallVerification { if (bytes3(magicValue) != bytes3(ILSP20.lsp20VerifyCall.selector)) revert LSP20InvalidMagicValue(false, returnedData); - return bytes1(magicValue[3]) == 0x01; + return magicValue[3] == 0x01; } /** diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 7e48f5975..a1ddadb37 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -96,8 +96,6 @@ abstract contract LSP6KeyManagerCore is address internal _target; - // Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly. - // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol mapping(address => bool) internal _reentrancyStatus; /** @@ -419,7 +417,7 @@ abstract contract LSP6KeyManagerCore is payload ); - if (reentrancyStatus && !isSetData) { + if (!reentrancyStatus && !isSetData) { _nonReentrantAfter(targetContract); } @@ -489,7 +487,7 @@ abstract contract LSP6KeyManagerCore is payload ); - if (reentrancyStatus && !isSetData) { + if (!reentrancyStatus && !isSetData) { _nonReentrantAfter(targetContract); }