From e8497da50bcbdebde9c4dc21ecb0ce70c6e535d6 Mon Sep 17 00:00:00 2001 From: b00ste Date: Fri, 27 Jan 2023 14:45:23 +0200 Subject: [PATCH 1/3] refactor: separate the LSP6 core contract in logic modules --- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 771 +----------------- .../LSP6ExecuteLogicModule.sol | 211 +++++ .../LSP6OwnershipLogicModule.sol | 21 + .../LSP6SetDataLogicModule.sol | 610 ++++++++++++++ contracts/LSP6KeyManager/LSP6Utils.sol | 49 +- .../KeyManager/KeyManagerInternalsTester.sol | 2 +- 6 files changed, 921 insertions(+), 743 deletions(-) create mode 100644 contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol create mode 100644 contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol create mode 100644 contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 1b6e3c80f..3c881f615 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -3,33 +3,38 @@ pragma solidity ^0.8.5; // interfaces import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; -import {IERC725X} from "@erc725/smart-contracts/contracts/interfaces/IERC725X.sol"; import {ILSP6KeyManager} from "./ILSP6KeyManager.sol"; // modules import {ILSP14Ownable2Step} from "../LSP14Ownable2Step/ILSP14Ownable2Step.sol"; import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; +import {LSP6SetDataLogicModule} from "./LSP6LogicModules/LSP6SetDataLogicModule.sol"; +import {LSP6ExecuteLogicModule} from "./LSP6LogicModules/LSP6ExecuteLogicModule.sol"; +import {LSP6OwnershipLogicModule} from "./LSP6LogicModules/LSP6OwnershipLogicModule.sol"; // libraries import {GasLib} from "../Utils/GasLib.sol"; import {BytesLib} from "solidity-bytes-utils/contracts/BytesLib.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; -import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import {LSP6Utils} from "./LSP6Utils.sol"; import {EIP191Signer} from "../Custom/EIP191Signer.sol"; // errors -import "./LSP6Errors.sol"; +import { + BatchExecuteParamsLengthMismatch, + BatchExecuteRelayCallParamsLengthMismatch, + LSP6BatchExcessiveValueSent, + LSP6BatchInsufficientValueSent, + InvalidPayload, + InvalidRelayNonce, + NoPermissionsSet, + InvalidERC725Function +} from "./LSP6Errors.sol"; // constants import { - OPERATION_0_CALL, - OPERATION_1_CREATE, - OPERATION_2_CREATE2, - OPERATION_3_STATICCALL, - OPERATION_4_DELEGATECALL, SETDATA_SELECTOR, SETDATA_ARRAY_SELECTOR, EXECUTE_SELECTOR @@ -39,15 +44,12 @@ import { _ERC1271_MAGICVALUE, _ERC1271_FAILVALUE } from "../LSP0ERC725Account/LSP0Constants.sol"; - import { - _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX, - _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY -} from "../LSP1UniversalReceiver/LSP1Constants.sol"; - -import "./LSP6Constants.sol"; - -import {_LSP17_EXTENSION_PREFIX} from "../LSP17ContractExtension/LSP17Constants.sol"; + LSP6_VERSION, + _INTERFACEID_LSP6, + _PERMISSION_SIGN, + _PERMISSION_REENTRANCY +} from "./LSP6Constants.sol"; /** * @title Core implementation of the LSP6 Key Manager standard. @@ -55,11 +57,15 @@ import {_LSP17_EXTENSION_PREFIX} from "../LSP17ContractExtension/LSP17Constants. * @dev This contract acts as a controller for an ERC725 Account. * Permissions for controllers are stored in the ERC725Y storage of the ERC725 Account and can be updated using `setData(...)`. */ -abstract contract LSP6KeyManagerCore is ERC165, ILSP6KeyManager { +abstract contract LSP6KeyManagerCore is + ERC165, + ILSP6KeyManager, + LSP6SetDataLogicModule, + LSP6ExecuteLogicModule, + LSP6OwnershipLogicModule +{ using LSP6Utils for *; - using Address for address; using ECDSA for bytes32; - using ERC165Checker for address; using EIP191Signer for address; using BytesLib for bytes; @@ -304,7 +310,7 @@ abstract contract LSP6KeyManagerCore is ERC165, ILSP6KeyManager { if (erc725Function == SETDATA_SELECTOR) { (bytes32 inputKey, bytes memory inputValue) = abi.decode(payload[4:], (bytes32, bytes)); - _verifyCanSetData(from, permissions, inputKey, inputValue); + _verifyCanSetData(_target, from, permissions, inputKey, inputValue); // ERC725Y.setData(bytes32[],bytes[]) } else if (erc725Function == SETDATA_ARRAY_SELECTOR) { @@ -313,738 +319,21 @@ abstract contract LSP6KeyManagerCore is ERC165, ILSP6KeyManager { (bytes32[], bytes[]) ); - _verifyCanSetData(from, permissions, inputKeys, inputValues); + _verifyCanSetData(_target, from, permissions, inputKeys, inputValues); // ERC725X.execute(uint256,address,uint256,bytes) } else if (erc725Function == EXECUTE_SELECTOR) { - _verifyCanExecute(from, permissions, payload); + _verifyCanExecute(_target, from, permissions, payload); } else if ( erc725Function == ILSP14Ownable2Step.transferOwnership.selector || erc725Function == ILSP14Ownable2Step.acceptOwnership.selector ) { - _requirePermissions(from, permissions, _PERMISSION_CHANGEOWNER); + _verifyOwnershipPermissions(from, permissions); } else { revert InvalidERC725Function(erc725Function); } } - /** - * @dev verify if the `controllerAddress` has the permissions required to set a data key on the ERC725Y storage of the `target`. - * @param controllerAddress the address who want to set the data key. - * @param controllerPermissions the permissions to be checked against. - * @param inputDataKey the data key to set on the `target`. - * @param inputDataValue the data value to set for the `inputDataKey`. - */ - function _verifyCanSetData( - address controllerAddress, - bytes32 controllerPermissions, - bytes32 inputDataKey, - bytes memory inputDataValue - ) internal view virtual { - bytes32 requiredPermission = _getPermissionRequiredToSetDataKey( - inputDataKey, - inputDataValue - ); - - // CHECK if allowed to set an ERC725Y Data Key - if (requiredPermission == _PERMISSION_SETDATA) { - // Skip if caller has SUPER permissions - if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - - _requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); - - _verifyAllowedERC725YSingleKey( - controllerAddress, - inputDataKey, - ERC725Y(_target).getAllowedERC725YDataKeysFor(controllerAddress) - ); - } else { - // Otherwise CHECK the required permission if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - _requirePermissions(controllerAddress, controllerPermissions, requiredPermission); - } - } - - /** - * @dev verify if the `controllerAddress` has the permissions required to set an array of data keys on the ERC725Y storage of the `target`. - * @param controllerAddress the address who want to set the data keys. - * @param controllerPermissions the permissions to be checked against. - * @param inputDataKeys an array of data keys to set on the `target`. - * @param inputDataValues an array of data values to set for the `inputDataKeys`. - */ - function _verifyCanSetData( - address controllerAddress, - bytes32 controllerPermissions, - bytes32[] memory inputDataKeys, - bytes[] memory inputDataValues - ) internal view virtual { - bool isSettingERC725YKeys; - bool[] memory validatedInputDataKeys = new bool[](inputDataKeys.length); - - bytes32 requiredPermission; - - uint256 ii; - do { - requiredPermission = _getPermissionRequiredToSetDataKey( - inputDataKeys[ii], - inputDataValues[ii] - ); - - if (requiredPermission == _PERMISSION_SETDATA) { - isSettingERC725YKeys = true; - } else { - // CHECK the required permissions if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - _requirePermissions(controllerAddress, controllerPermissions, requiredPermission); - validatedInputDataKeys[ii] = true; - } - - ii = GasLib.uncheckedIncrement(ii); - } while (ii < inputDataKeys.length); - - // CHECK if allowed to set one (or multiple) ERC725Y Data Keys - if (isSettingERC725YKeys) { - // Skip if caller has SUPER permissions - if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - - _requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); - - _verifyAllowedERC725YDataKeys( - controllerAddress, - inputDataKeys, - ERC725Y(_target).getAllowedERC725YDataKeysFor(controllerAddress), - validatedInputDataKeys - ); - } - } - - /** - * @dev retrieve the permission required based on the data key to be set on the `target`. - * @param inputDataKey the data key to set on the `target`. Can be related to LSP6 Permissions, LSP1 Delegate or LSP17 Extensions. - * @param inputDataValue the data value to set for the `inputDataKey`. - * @return the permission required to set the `inputDataKey` on the `target`. - */ - function _getPermissionRequiredToSetDataKey(bytes32 inputDataKey, bytes memory inputDataValue) - internal - view - virtual - returns (bytes32) - { - // AddressPermissions[] or AddressPermissions[index] - if (bytes16(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY_PREFIX) { - return _getPermissionToSetPermissionsArray(inputDataKey, inputDataValue); - - // AddressPermissions:... - } else if (bytes6(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_PREFIX) { - // AddressPermissions:Permissions:
- if (bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX) { - return _getPermissionToSetControllerPermissions(inputDataKey); - - // AddressPermissions:AllowedCalls:
- } else if (bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_ALLOWEDCALLS_PREFIX) { - return _getPermissionToSetAllowedCalls(inputDataKey, inputDataValue); - - // AddressPermissions:AllowedERC725YKeys:
- } else if ( - bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_AllowedERC725YDataKeys_PREFIX - ) { - return _getPermissionToSetAllowedERC725YDataKeys(inputDataKey, inputDataValue); - - // if the first 6 bytes of the input data key are "AddressPermissions:..." but did not match - // with anything above, this is not a standard LSP6 permission data key so we revert. - } else { - /** - * @dev more permissions types starting with `AddressPermissions:...` can be implemented by overriding this function. - * - * // AddressPermissions:MyCustomPermissions:
- * bytes12 CUSTOM_PERMISSION_PREFIX = 0x4b80742de2bf9e659ba40000 - * - * if (bytes12(dataKey) == CUSTOM_PERMISSION_PREFIX) { - * // custom logic - * } - * - * super._getPermissionRequiredToSetDataKey(...) - */ - revert NotRecognisedPermissionKey(inputDataKey); - } - - // LSP1UniversalReceiverDelegate or LSP1UniversalReceiverDelegate: - } else if ( - inputDataKey == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY || - bytes12(inputDataKey) == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX - ) { - return _getPermissionToSetLSP1Delegate(inputDataKey); - - // LSP17Extension: - } else if (bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX) { - return _getPermissionToSetLSP17Extension(inputDataKey); - } else { - return _PERMISSION_SETDATA; - } - } - - /** - * @dev retrieve the permission required to update the `AddressPermissions[]` array data key defined in LSP6. - * @param inputDataKey either `AddressPermissions[]` (array length) or `AddressPermissions[index]` (array index) - * @param inputDataValue the updated value for the `inputDataKey`. MUST be: - * - a `uint256` for `AddressPermissions[]` (array length) - * - an `address` or `0x` for `AddressPermissions[index]` (array entry). - * - * @return either ADD or CHANGE PERMISSIONS. - */ - function _getPermissionToSetPermissionsArray(bytes32 inputDataKey, bytes memory inputDataValue) - internal - view - virtual - returns (bytes32) - { - bytes memory currentValue = ERC725Y(_target).getData(inputDataKey); - - // AddressPermissions[] -> array length - if (inputDataKey == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY) { - uint256 newLength = uint256(bytes32(inputDataValue)); - - return - newLength > uint256(bytes32(currentValue)) - ? _PERMISSION_ADDCONTROLLER - : _PERMISSION_CHANGEPERMISSIONS; - } - - // AddressPermissions[index] -> array index - - // CHECK that we either ADD an address (20 bytes long) or REMOVE an address (0x) - if (inputDataValue.length != 0 && inputDataValue.length != 20) { - revert AddressPermissionArrayIndexValueNotAnAddress(inputDataKey, inputDataValue); - } - - return currentValue.length == 0 ? _PERMISSION_ADDCONTROLLER : _PERMISSION_CHANGEPERMISSIONS; - } - - /** - * @dev retrieve the permission required to set permissions for a controller address. - * @param inputPermissionDataKey `AddressPermissions:Permissions:`. - * @return either ADD or CHANGE PERMISSIONS. - */ - function _getPermissionToSetControllerPermissions(bytes32 inputPermissionDataKey) - internal - view - virtual - returns (bytes32) - { - return - // if there is nothing stored under the data key, we are trying to ADD a new controller. - // if there are already some permissions set under the data key, we are trying to CHANGE the permissions of a controller. - bytes32(ERC725Y(_target).getData(inputPermissionDataKey)) == bytes32(0) - ? _PERMISSION_ADDCONTROLLER - : _PERMISSION_CHANGEPERMISSIONS; - } - - /** - * @dev retrieve the permission required to set some AllowedCalls for a controller. - * @param dataKey `AddressPermissions:AllowedCalls:`. - * @param dataValue the updated value for the `dataKey`. MUST be a bytes28[CompactBytesArray] of Allowed Calls. - * @return either ADD or CHANGE PERMISSIONS. - */ - function _getPermissionToSetAllowedCalls(bytes32 dataKey, bytes memory dataValue) - internal - view - virtual - returns (bytes32) - { - if (!LSP6Utils.isCompactBytesArrayOfAllowedCalls(dataValue)) { - revert InvalidEncodedAllowedCalls(dataValue); - } - - // if there is nothing stored under the Allowed Calls of the controller, - // we are trying to ADD a list of restricted calls (standards + address + function selector) - // - // if there are already some data set under the Allowed Calls of the controller, - // we are trying to CHANGE (= edit) these restrictions. - return - ERC725Y(_target).getData(dataKey).length == 0 - ? _PERMISSION_ADDCONTROLLER - : _PERMISSION_CHANGEPERMISSIONS; - } - - /** - * @dev retrieve the permission required to set some AllowedCalls for a controller. - * @param dataKey or `AddressPermissions:AllowedERC725YDataKeys:`. - * @param dataValue the updated value for the `dataKey`. MUST be a bytes[CompactBytesArray] of Allowed ERC725Y Data Keys. - * @return either ADD or CHANGE PERMISSIONS. - */ - function _getPermissionToSetAllowedERC725YDataKeys(bytes32 dataKey, bytes memory dataValue) - internal - view - returns (bytes32) - { - if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(dataValue)) { - revert InvalidEncodedAllowedERC725YDataKeys(dataValue); - } - - // if there is nothing stored under the Allowed ERC725Y Data Keys of the controller, - // we are trying to ADD a list of restricted ERC725Y Data Keys. - // - // if there are already some data set under the Allowed ERC725Y Data Keys of the controller, - // we are trying to CHANGE (= edit) these restricted ERC725Y data keys. - return - ERC725Y(_target).getData(dataKey).length == 0 - ? _PERMISSION_ADDCONTROLLER - : _PERMISSION_CHANGEPERMISSIONS; - } - - /** - * @dev retrieve the permission required to either add or change the address - * of a LSP1 Universal Receiver Delegate stored under a specific LSP1 data key. - * @param lsp1DelegateDataKey either the data key for the default `LSP1UniversalReceiverDelegate`, - * or a data key for a specific `LSP1UniversalReceiverDelegate:`, starting with `_LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX`. - * @return either ADD or CHANGE UNIVERSALRECEIVERDELEGATE. - */ - function _getPermissionToSetLSP1Delegate(bytes32 lsp1DelegateDataKey) - internal - view - virtual - returns (bytes32) - { - return - ERC725Y(_target).getData(lsp1DelegateDataKey).length == 0 - ? _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE - : _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE; - } - - /** - * @dev Verify if `from` has the required permissions to either add or change the address - * of an LSP0 Extension stored under a specific LSP17Extension data key - * @param lsp17ExtensionDataKey the dataKey to set with `_LSP17_EXTENSION_PREFIX` as prefix. - */ - function _getPermissionToSetLSP17Extension(bytes32 lsp17ExtensionDataKey) - internal - view - virtual - returns (bytes32) - { - return - ERC725Y(_target).getData(lsp17ExtensionDataKey).length == 0 - ? _PERMISSION_ADDEXTENSIONS - : _PERMISSION_CHANGEEXTENSIONS; - } - - /** - * @dev Verify if the `inputKey` is present in the list of `allowedERC725KeysCompacted` for the `controllerAddress`. - * @param controllerAddress the address of the controller. - * @param inputDataKey the data key to verify against the allowed ERC725Y Data Keys for the `controllerAddress`. - * @param allowedERC725YDataKeysCompacted a CompactBytesArray of allowed ERC725Y Data Keys for the `controllerAddress`. - */ - function _verifyAllowedERC725YSingleKey( - address controllerAddress, - bytes32 inputDataKey, - bytes memory allowedERC725YDataKeysCompacted - ) internal pure virtual { - if (allowedERC725YDataKeysCompacted.length == 0) - revert NoERC725YDataKeysAllowed(controllerAddress); - - if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted)) - revert InvalidEncodedAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted); - - /** - * The pointer will always land on the length of each bytes value: - * - * ↓↓ - * 03 a00000 - * 05 fff83a0011 - * 20 aa0000000000000000000000000000000000000000000000000000000000cafe - * 12 bb000000000000000000000000000000beef - * 19 cc00000000000000000000000000000000000000000000deed - * ↑↑ - * - */ - uint256 pointer; - - // information extracted from each Allowed ERC725Y Data Key. - uint256 length; - bytes32 allowedKey; - bytes32 mask; - - /** - * iterate over each data key and update the `pointer` variable with the index where to find the length of each data key. - * - * 0x 03 a00000 03 fff83a 20 aa00...00cafe - * ↑↑ ↑↑ ↑↑ - * first | second | third - * length | length | length - */ - while (pointer < allowedERC725YDataKeysCompacted.length) { - // save the length of the allowed data key to calculate the `mask`. - length = uint16( - bytes2( - abi.encodePacked( - allowedERC725YDataKeysCompacted[pointer], - allowedERC725YDataKeysCompacted[pointer + 1] - ) - ) - ); - - /** - * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & - * It is used to compare only the relevant parts of each input data key against dynamic allowed data keys. - * - * E.g.: - * - * allowed data key = 0xa00000 - * - * compare this part - * vvvvvv - * input data key = 0xa00000cafecafecafecafecafecafecafe000000000000000000000011223344 - * - * & discard this part - * vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv - * mask = 0xffffff0000000000000000000000000000000000000000000000000000000000 - */ - mask = - bytes32(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) << - (8 * (32 - length)); - - /* - * transform the allowed data key situated from `pointer + 1` until `pointer + 1 + length` to a bytes32 value. - * E.g. 0xfff83a -> 0xfff83a0000000000000000000000000000000000000000000000000000000000 - */ - // solhint-disable-next-line no-inline-assembly - assembly { - // the first 32 bytes word in memory (where allowedERC725YDataKeysCompacted is stored) - // correspond to the total number of bytes in `allowedERC725YDataKeysCompacted` - let offset := add(add(pointer, 2), 32) - let memoryAt := mload(add(allowedERC725YDataKeysCompacted, offset)) - // MLOAD loads 32 bytes word, so we need to keep only the `length` number of bytes that makes up the allowed data key. - allowedKey := and(memoryAt, mask) - } - - // voila you found the key ;) - if (allowedKey == (inputDataKey & mask)) return; - - // move the pointer to the index of the next allowed data key - unchecked { - pointer = pointer + (length + 2); - } - } - - revert NotAllowedERC725YDataKey(controllerAddress, inputDataKey); - } - - /** - * @dev Verify if all the `inputDataKeys` are present in the list of `allowedERC725KeysCompacted` of the `controllerAddress`. - * @param controllerAddress the address of the controller. - * @param inputDataKeys the data keys to verify against the allowed ERC725Y Data Keys of the `controllerAddress`. - * @param allowedERC725YDataKeysCompacted a CompactBytesArray of allowed ERC725Y Data Keys of the `controllerAddress`. - * @param validatedInputKeys an array of booleans to store the result of the verification of each data keys checked. - */ - function _verifyAllowedERC725YDataKeys( - address controllerAddress, - bytes32[] memory inputDataKeys, - bytes memory allowedERC725YDataKeysCompacted, - bool[] memory validatedInputKeys - ) internal pure virtual { - if (allowedERC725YDataKeysCompacted.length == 0) - revert NoERC725YDataKeysAllowed(controllerAddress); - - if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted)) - revert InvalidEncodedAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted); - - uint256 allowedKeysFound; - - // cache the input data keys from the start - uint256 inputKeysLength = inputDataKeys.length; - - /** - * The pointer will always land on the length of each bytes value: - * - * ↓↓ - * 03 a00000 - * 05 fff83a0011 - * 20 aa0000000000000000000000000000000000000000000000000000000000cafe - * 12 bb000000000000000000000000000000beef - * 19 cc00000000000000000000000000000000000000000000deed - * ↑↑ - * - */ - uint256 pointer; - - // information extracted from each Allowed ERC725Y Data Key. - uint256 length; - bytes32 allowedKey; - bytes32 mask; - - /** - * iterate over each data key and update the `pointer` variable with the index where to find the length of each data key. - * - * 0x 03 a00000 03 fff83a 20 aa00...00cafe - * ↑↑ ↑↑ ↑↑ - * first | second | third - * length | length | length - */ - while (pointer < allowedERC725YDataKeysCompacted.length) { - // save the length of the allowed data key to calculate the `mask`. - length = uint16( - bytes2( - abi.encodePacked( - allowedERC725YDataKeysCompacted[pointer], - allowedERC725YDataKeysCompacted[pointer + 1] - ) - ) - ); - - /** - * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & - * It is used to compare only the relevant parts of each input data key against dynamic allowed data keys. - * - * E.g.: - * - * allowed data key = 0xa00000 - * - * compare this part - * vvvvvv - * input data key = 0xa00000cafecafecafecafecafecafecafe000000000000000000000011223344 - * - * & discard this part - * vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv - * mask = 0xffffff0000000000000000000000000000000000000000000000000000000000 - */ - mask = - bytes32(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) << - (8 * (32 - length)); - - /* - * transform the allowed data key situated from `pointer + 1` until `pointer + 1 + length` to a bytes32 value. - * E.g. 0xfff83a -> 0xfff83a0000000000000000000000000000000000000000000000000000000000 - */ - // solhint-disable-next-line no-inline-assembly - assembly { - // the first 32 bytes word in memory (where allowedERC725YDataKeysCompacted is stored) - // correspond to the length of allowedERC725YDataKeysCompacted (= total number of bytes) - let offset := add(add(pointer, 2), 32) - let memoryAt := mload(add(allowedERC725YDataKeysCompacted, offset)) - allowedKey := and(memoryAt, mask) - } - - /** - * Iterate over the `inputDataKeys` to check them against the allowed data keys. - * This until we have validated them all. - */ - for (uint256 ii; ii < inputKeysLength; ii = GasLib.uncheckedIncrement(ii)) { - // if the input data key has been marked as allowed previously, - // SKIP it and move to the next input data key. - if (validatedInputKeys[ii]) continue; - - // CHECK if the input data key is allowed. - if ((inputDataKeys[ii] & mask) == allowedKey) { - // if the input data key is allowed, mark it as allowed - // and increment the number of allowed keys found. - validatedInputKeys[ii] = true; - allowedKeysFound = GasLib.uncheckedIncrement(allowedKeysFound); - - // Continue checking until all the inputKeys` have been found. - if (allowedKeysFound == inputKeysLength) return; - } - } - - // Move the pointer to the next AllowedERC725YKey - unchecked { - pointer = pointer + (length + 2); - } - } - - // if we did not find all the input data keys, search for the first not allowed data key to revert. - for (uint256 jj; jj < inputKeysLength; jj = GasLib.uncheckedIncrement(jj)) { - if (!validatedInputKeys[jj]) { - revert NotAllowedERC725YDataKey(controllerAddress, inputDataKeys[jj]); - } - } - } - - /** - * @dev verify if `from` has the required permissions to interact with other addresses using the target. - * @param from the address who want to run the execute function on the ERC725Account - * @param permissions the permissions of the caller - * @param payload the ABI encoded payload `target.execute(...)` - */ - function _verifyCanExecute( - address from, - bytes32 permissions, - bytes calldata payload - ) internal view virtual { - // MUST be one of the ERC725X operation types. - uint256 operationType = uint256(bytes32(payload[4:36])); - - // DELEGATECALL is disallowed by default on the LSP6 Key Manager. - if (operationType == OPERATION_4_DELEGATECALL) { - revert DelegateCallDisallowedViaKeyManager(); - } - - uint256 value = uint256(bytes32(payload[68:100])); - - // prettier-ignore - bool isContractCreation = operationType == OPERATION_1_CREATE || operationType == OPERATION_2_CREATE2; - bool isCallDataPresent = payload.length > 164; - - // SUPER operation only applies to contract call, not contract creation - bool hasSuperOperation = isContractCreation - ? false - : permissions.hasPermission(_extractSuperPermissionFromOperation(operationType)); - - // CHECK if we are doing an empty call, as the receive() or fallback() function - // of the target contract could run some code. - if (!hasSuperOperation && !isCallDataPresent && value == 0) { - _requirePermissions(from, permissions, _extractPermissionFromOperation(operationType)); - } - - if (isCallDataPresent && !hasSuperOperation) { - _requirePermissions(from, permissions, _extractPermissionFromOperation(operationType)); - } - - bool hasSuperTransferValue = permissions.hasPermission(_PERMISSION_SUPER_TRANSFERVALUE); - - if (value != 0 && !hasSuperTransferValue) { - _requirePermissions(from, permissions, _PERMISSION_TRANSFERVALUE); - } - - // Skip on contract creation (CREATE or CREATE2) - if (isContractCreation) return; - - // Skip if caller has SUPER permissions for external calls, with or without calldata (empty calls) - if (hasSuperOperation && value == 0) return; - - // Skip if caller has SUPER permission for value transfers - if (hasSuperTransferValue && !isCallDataPresent && value != 0) return; - - // Skip if both SUPER permissions are present - if (hasSuperOperation && hasSuperTransferValue) return; - - _verifyAllowedCall(from, payload); - } - - function _verifyAllowedCall(address from, bytes calldata payload) internal view virtual { - // CHECK for ALLOWED CALLS - address to = address(bytes20(payload[48:68])); - - bool containsFunctionCall = payload.length >= 168; - bytes4 selector; - if (containsFunctionCall) selector = bytes4(payload[164:168]); - - bytes memory allowedCalls = ERC725Y(_target).getAllowedCallsFor(from); - uint256 allowedCallsLength = allowedCalls.length; - - if (allowedCallsLength == 0) { - revert NoCallsAllowed(from); - } - - bool isAllowedStandard; - bool isAllowedAddress; - bool isAllowedFunction; - - for (uint256 ii; ii < allowedCallsLength; ii += 30) { - if (ii + 30 > allowedCallsLength) { - revert InvalidEncodedAllowedCalls(allowedCalls); - } - bytes memory chunk = BytesLib.slice(allowedCalls, ii + 2, 28); - - if (bytes28(chunk) == 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffff) { - revert InvalidWhitelistedCall(from); - } - - bytes4 allowedStandard = bytes4(chunk); - address allowedAddress = address(bytes20(bytes28(chunk) << 32)); - bytes4 allowedFunction = bytes4(bytes28(chunk) << 192); - - isAllowedStandard = - allowedStandard == 0xffffffff || - to.supportsERC165InterfaceUnchecked(allowedStandard); - isAllowedAddress = - allowedAddress == 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF || - to == allowedAddress; - isAllowedFunction = - allowedFunction == 0xffffffff || - (containsFunctionCall && (selector == allowedFunction)); - - if (isAllowedStandard && isAllowedAddress && isAllowedFunction) return; - } - - revert NotAllowedCall(from, to, selector); - } - - /** - * @dev extract the required permission + a descriptive string, based on the `_operationType` - * being run via ERC725Account.execute(...) - * @param operationType 0 = CALL, 1 = CREATE, 2 = CREATE2, etc... See ERC725X docs for more infos. - * @return permissionsRequired (bytes32) the permission associated with the `_operationType` - */ - function _extractPermissionFromOperation(uint256 operationType) - internal - pure - virtual - returns (bytes32 permissionsRequired) - { - if (operationType == OPERATION_0_CALL) return _PERMISSION_CALL; - else if (operationType == OPERATION_1_CREATE) return _PERMISSION_DEPLOY; - else if (operationType == OPERATION_2_CREATE2) return _PERMISSION_DEPLOY; - else if (operationType == OPERATION_3_STATICCALL) return _PERMISSION_STATICCALL; - else if (operationType == OPERATION_4_DELEGATECALL) return _PERMISSION_DELEGATECALL; - } - - /** - * @dev returns the `superPermission` needed for a specific `operationType` of the `execute(..)` - */ - function _extractSuperPermissionFromOperation(uint256 operationType) - internal - pure - virtual - returns (bytes32 superPermission) - { - if (operationType == OPERATION_0_CALL) return _PERMISSION_SUPER_CALL; - else if (operationType == OPERATION_3_STATICCALL) return _PERMISSION_SUPER_STATICCALL; - else if (operationType == OPERATION_4_DELEGATECALL) return _PERMISSION_SUPER_DELEGATECALL; - } - - /** - * @dev revert if `from`'s `addressPermissions` doesn't contain `permissionsRequired` - * @param from the caller address - * @param addressPermissions the caller's permissions BitArray - * @param permissionRequired the required permission - */ - function _requirePermissions( - address from, - bytes32 addressPermissions, - bytes32 permissionRequired - ) internal pure virtual { - if (!addressPermissions.hasPermission(permissionRequired)) { - string memory permissionErrorString = _getPermissionName(permissionRequired); - revert NotAuthorised(from, permissionErrorString); - } - } - - /** - * @dev returns the name of the permission as a string - */ - function _getPermissionName(bytes32 permission) - internal - pure - virtual - returns (string memory errorMessage) - { - if (permission == _PERMISSION_CHANGEOWNER) return "TRANSFEROWNERSHIP"; - if (permission == _PERMISSION_CHANGEPERMISSIONS) return "CHANGEPERMISSIONS"; - if (permission == _PERMISSION_ADDCONTROLLER) return "ADDCONTROLLER"; - if (permission == _PERMISSION_ADDEXTENSIONS) return "ADDEXTENSIONS"; - if (permission == _PERMISSION_CHANGEEXTENSIONS) return "CHANGEEXTENSIONS"; - if (permission == _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE) - return "ADDUNIVERSALRECEIVERDELEGATE"; - if (permission == _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE) - return "CHANGEUNIVERSALRECEIVERDELEGATE"; - if (permission == _PERMISSION_REENTRANCY) return "REENTRANCY"; - if (permission == _PERMISSION_SETDATA) return "SETDATA"; - if (permission == _PERMISSION_CALL) return "CALL"; - if (permission == _PERMISSION_STATICCALL) return "STATICCALL"; - if (permission == _PERMISSION_DELEGATECALL) return "DELEGATECALL"; - if (permission == _PERMISSION_DEPLOY) return "DEPLOY"; - if (permission == _PERMISSION_TRANSFERVALUE) return "TRANSFERVALUE"; - if (permission == _PERMISSION_SIGN) return "SIGN"; - } - /** * @dev Initialise _reentrancyStatus to _NOT_ENTERED. */ @@ -1060,7 +349,7 @@ abstract contract LSP6KeyManagerCore is ERC165, ILSP6KeyManager { function _nonReentrantBefore(address from) internal virtual { if (_reentrancyStatus) { // CHECK the caller has REENTRANCY permission - _requirePermissions( + LSP6Utils.requirePermissions( from, ERC725Y(_target).getPermissionsFor(from), _PERMISSION_REENTRANCY diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol new file mode 100644 index 000000000..ea0716aef --- /dev/null +++ b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.5; + +// modules +import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol"; + +// libraries +import {LSP6Utils} from "../LSP6Utils.sol"; +import {BytesLib} from "solidity-bytes-utils/contracts/BytesLib.sol"; +import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; + +// constants +import { + _PERMISSION_TRANSFERVALUE, + _PERMISSION_SUPER_TRANSFERVALUE, + _PERMISSION_DEPLOY, + _PERMISSION_CALL, + _PERMISSION_SUPER_CALL, + _PERMISSION_STATICCALL, + _PERMISSION_SUPER_STATICCALL, + _PERMISSION_DELEGATECALL, + _PERMISSION_SUPER_DELEGATECALL +} from "../LSP6Constants.sol"; +import { + OPERATION_0_CALL, + OPERATION_1_CREATE, + OPERATION_2_CREATE2, + OPERATION_3_STATICCALL, + OPERATION_4_DELEGATECALL +} from "@erc725/smart-contracts/contracts/constants.sol"; + +// errors +import { + DelegateCallDisallowedViaKeyManager, + NoCallsAllowed, + NotAllowedCall, + InvalidEncodedAllowedCalls, + InvalidWhitelistedCall +} from "../LSP6Errors.sol"; + +abstract contract LSP6ExecuteLogicModule { + using ERC165Checker for address; + using LSP6Utils for *; + + /** + * @dev verify if `controllerAddress` has the required permissions to interact with other addresses using the controlledContract. + * @param controlledContract the address of the ERC725 contract where the payload is executed and where the permissions are verified. + * @param controllerAddress the address who want to run the execute function on the ERC725Account. + * @param controllerPermissions the permissions of the controller address. + * @param payload the ABI encoded payload `controlledContract.execute(...)`. + */ + function _verifyCanExecute( + address controlledContract, + address controllerAddress, + bytes32 controllerPermissions, + bytes calldata payload + ) internal view virtual { + // MUST be one of the ERC725X operation types. + uint256 operationType = uint256(bytes32(payload[4:36])); + + // DELEGATECALL is disallowed by default on the LSP6 Key Manager. + if (operationType == OPERATION_4_DELEGATECALL) { + revert DelegateCallDisallowedViaKeyManager(); + } + + uint256 value = uint256(bytes32(payload[68:100])); + + // prettier-ignore + bool isContractCreation = operationType == OPERATION_1_CREATE || operationType == OPERATION_2_CREATE2; + bool isCallDataPresent = payload.length > 164; + + // SUPER operation only applies to contract call, not contract creation + bool hasSuperOperation = isContractCreation + ? false + : controllerPermissions.hasPermission( + _extractSuperPermissionFromOperation(operationType) + ); + + // CHECK if we are doing an empty call, as the receive() or fallback() function + // of the controlledContract could run some code. + if (!hasSuperOperation && !isCallDataPresent && value == 0) { + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _extractPermissionFromOperation(operationType) + ); + } + + if (isCallDataPresent && !hasSuperOperation) { + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _extractPermissionFromOperation(operationType) + ); + } + + bool hasSuperTransferValue = controllerPermissions.hasPermission( + _PERMISSION_SUPER_TRANSFERVALUE + ); + + if (value != 0 && !hasSuperTransferValue) { + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _PERMISSION_TRANSFERVALUE + ); + } + + // Skip on contract creation (CREATE or CREATE2) + if (isContractCreation) return; + + // Skip if caller has SUPER permissions for external calls, with or without calldata (empty calls) + if (hasSuperOperation && value == 0) return; + + // Skip if caller has SUPER permission for value transfers + if (hasSuperTransferValue && !isCallDataPresent && value != 0) return; + + // Skip if both SUPER permissions are present + if (hasSuperOperation && hasSuperTransferValue) return; + + _verifyAllowedCall(controlledContract, controllerAddress, payload); + } + + function _verifyAllowedCall( + address controlledContract, + address controllerAddress, + bytes calldata payload + ) internal view virtual { + // CHECK for ALLOWED CALLS + address to = address(bytes20(payload[48:68])); + + bool containsFunctionCall = payload.length >= 168; + bytes4 selector; + if (containsFunctionCall) selector = bytes4(payload[164:168]); + + bytes memory allowedCalls = ERC725Y(controlledContract).getAllowedCallsFor( + controllerAddress + ); + uint256 allowedCallsLength = allowedCalls.length; + + if (allowedCallsLength == 0) { + revert NoCallsAllowed(controllerAddress); + } + + bool isAllowedStandard; + bool isAllowedAddress; + bool isAllowedFunction; + + for (uint256 ii; ii < allowedCallsLength; ii += 30) { + if (ii + 30 > allowedCallsLength) { + revert InvalidEncodedAllowedCalls(allowedCalls); + } + bytes memory chunk = BytesLib.slice(allowedCalls, ii + 2, 28); + + if (bytes28(chunk) == 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffff) { + revert InvalidWhitelistedCall(controllerAddress); + } + + bytes4 allowedStandard = bytes4(chunk); + address allowedAddress = address(bytes20(bytes28(chunk) << 32)); + bytes4 allowedFunction = bytes4(bytes28(chunk) << 192); + + isAllowedStandard = + allowedStandard == 0xffffffff || + to.supportsERC165InterfaceUnchecked(allowedStandard); + isAllowedAddress = + allowedAddress == 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF || + to == allowedAddress; + isAllowedFunction = + allowedFunction == 0xffffffff || + (containsFunctionCall && (selector == allowedFunction)); + + if (isAllowedStandard && isAllowedAddress && isAllowedFunction) return; + } + + revert NotAllowedCall(controllerAddress, to, selector); + } + + /** + * @dev extract the required permission + a descriptive string, based on the `_operationType` + * being run via ERC725Account.execute(...) + * @param operationType 0 = CALL, 1 = CREATE, 2 = CREATE2, etc... See ERC725X docs for more infos. + * @return permissionsRequired (bytes32) the permission associated with the `_operationType` + */ + function _extractPermissionFromOperation(uint256 operationType) + internal + pure + virtual + returns (bytes32 permissionsRequired) + { + if (operationType == OPERATION_0_CALL) return _PERMISSION_CALL; + else if (operationType == OPERATION_1_CREATE) return _PERMISSION_DEPLOY; + else if (operationType == OPERATION_2_CREATE2) return _PERMISSION_DEPLOY; + else if (operationType == OPERATION_3_STATICCALL) return _PERMISSION_STATICCALL; + else if (operationType == OPERATION_4_DELEGATECALL) return _PERMISSION_DELEGATECALL; + } + + /** + * @dev returns the `superPermission` needed for a specific `operationType` of the `execute(..)` + */ + function _extractSuperPermissionFromOperation(uint256 operationType) + internal + pure + virtual + returns (bytes32 superPermission) + { + if (operationType == OPERATION_0_CALL) return _PERMISSION_SUPER_CALL; + else if (operationType == OPERATION_3_STATICCALL) return _PERMISSION_SUPER_STATICCALL; + else if (operationType == OPERATION_4_DELEGATECALL) return _PERMISSION_SUPER_DELEGATECALL; + } +} diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol new file mode 100644 index 000000000..746a2a59e --- /dev/null +++ b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.5; + +// libraries +import {LSP6Utils} from "../LSP6Utils.sol"; + +// constants +import {_PERMISSION_CHANGEOWNER} from "../LSP6Constants.sol"; + +abstract contract LSP6OwnershipLogicModule { + function _verifyOwnershipPermissions(address controllerAddress, bytes32 controllerPermissions) + internal + pure + { + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _PERMISSION_CHANGEOWNER + ); + } +} diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol new file mode 100644 index 000000000..53a64ca8c --- /dev/null +++ b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol @@ -0,0 +1,610 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.5; + +// modules +import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol"; + +// libraries +import {GasLib} from "../../Utils/GasLib.sol"; +import {LSP6Utils} from "../LSP6Utils.sol"; + +// constants +import { + _LSP6KEY_ADDRESSPERMISSIONS_ARRAY, + _LSP6KEY_ADDRESSPERMISSIONS_ARRAY_PREFIX, + _LSP6KEY_ADDRESSPERMISSIONS_PREFIX, + _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX, + _LSP6KEY_ADDRESSPERMISSIONS_ALLOWEDCALLS_PREFIX, + _LSP6KEY_ADDRESSPERMISSIONS_AllowedERC725YDataKeys_PREFIX, + _PERMISSION_SETDATA, + _PERMISSION_SUPER_SETDATA, + _PERMISSION_ADDCONTROLLER, + _PERMISSION_CHANGEPERMISSIONS, + _PERMISSION_ADDEXTENSIONS, + _PERMISSION_CHANGEEXTENSIONS, + _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE, + _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE +} from "../LSP6Constants.sol"; +import { + _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX, + _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY +} from "../../LSP1UniversalReceiver/LSP1Constants.sol"; +import {_LSP17_EXTENSION_PREFIX} from "../../LSP17ContractExtension/LSP17Constants.sol"; + +// errors +import { + NotRecognisedPermissionKey, + AddressPermissionArrayIndexValueNotAnAddress, + InvalidEncodedAllowedCalls, + InvalidEncodedAllowedERC725YDataKeys, + NoERC725YDataKeysAllowed, + NotAllowedERC725YDataKey +} from "../LSP6Errors.sol"; + +abstract contract LSP6SetDataLogicModule { + using LSP6Utils for *; + + /** + * @dev verify if the `controllerAddress` has the permissions required to set a data key on the ERC725Y storage of the `controlledContract`. + * @param controlledContract the address of the ERC725Y contract where the data key is set. + * @param controllerAddress the address of the controller who wants to set the data key. + * @param controllerPermissions the permissions of the controller address. + * @param inputDataKey the data key to set on the `controlledContract`. + * @param inputDataValue the data value to set for the `inputDataKey`. + */ + function _verifyCanSetData( + address controlledContract, + address controllerAddress, + bytes32 controllerPermissions, + bytes32 inputDataKey, + bytes memory inputDataValue + ) internal view virtual { + bytes32 requiredPermission = _getPermissionRequiredToSetDataKey( + controlledContract, + inputDataKey, + inputDataValue + ); + + // CHECK if allowed to set an ERC725Y Data Key + if (requiredPermission == _PERMISSION_SETDATA) { + // Skip if caller has SUPER permissions + if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; + + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _PERMISSION_SETDATA + ); + + _verifyAllowedERC725YSingleKey( + controllerAddress, + inputDataKey, + ERC725Y(controlledContract).getAllowedERC725YDataKeysFor(controllerAddress) + ); + } else { + // Otherwise CHECK the required permission if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + requiredPermission + ); + } + } + + /** + * @dev verify if the `controllerAddress` has the permissions required to set an array of data keys on the ERC725Y storage of the `controlledContract`. + * @param controlledContract the address of the ERC725Y contract where the data key is set. + * @param controllerAddress the address of the controller who wants to set the data key. + * @param controllerPermissions the permissions of the controller address. + * @param inputDataKeys an array of data keys to set on the `controlledContract`. + * @param inputDataValues an array of data values to set for the `inputDataKeys`. + */ + function _verifyCanSetData( + address controlledContract, + address controllerAddress, + bytes32 controllerPermissions, + bytes32[] memory inputDataKeys, + bytes[] memory inputDataValues + ) internal view virtual { + bool isSettingERC725YKeys; + bool[] memory validatedInputDataKeys = new bool[](inputDataKeys.length); + + bytes32 requiredPermission; + + uint256 ii; + do { + requiredPermission = _getPermissionRequiredToSetDataKey( + controlledContract, + inputDataKeys[ii], + inputDataValues[ii] + ); + + if (requiredPermission == _PERMISSION_SETDATA) { + isSettingERC725YKeys = true; + } else { + // CHECK the required permissions if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + requiredPermission + ); + validatedInputDataKeys[ii] = true; + } + + ii = GasLib.uncheckedIncrement(ii); + } while (ii < inputDataKeys.length); + + // CHECK if allowed to set one (or multiple) ERC725Y Data Keys + if (isSettingERC725YKeys) { + // Skip if caller has SUPER permissions + if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; + + LSP6Utils.requirePermissions( + controllerAddress, + controllerPermissions, + _PERMISSION_SETDATA + ); + + _verifyAllowedERC725YDataKeys( + controllerAddress, + inputDataKeys, + ERC725Y(controlledContract).getAllowedERC725YDataKeysFor(controllerAddress), + validatedInputDataKeys + ); + } + } + + /** + * @dev retrieve the permission required based on the data key to be set on the `controlledContract`. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param inputDataKey the data key to set on the `controlledContract`. Can be related to LSP6 Permissions, LSP1 Delegate or LSP17 Extensions. + * @param inputDataValue the data value to set for the `inputDataKey`. + * @return the permission required to set the `inputDataKey` on the `controlledContract`. + */ + function _getPermissionRequiredToSetDataKey( + address controlledContract, + bytes32 inputDataKey, + bytes memory inputDataValue + ) internal view virtual returns (bytes32) { + // AddressPermissions[] or AddressPermissions[index] + if (bytes16(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY_PREFIX) { + return + _getPermissionToSetPermissionsArray( + controlledContract, + inputDataKey, + inputDataValue + ); + + // AddressPermissions:... + } else if (bytes6(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_PREFIX) { + // AddressPermissions:Permissions:
+ if (bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX) { + return _getPermissionToSetControllerPermissions(controlledContract, inputDataKey); + + // AddressPermissions:AllowedCalls:
+ } else if (bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_ALLOWEDCALLS_PREFIX) { + return + _getPermissionToSetAllowedCalls( + controlledContract, + inputDataKey, + inputDataValue + ); + + // AddressPermissions:AllowedERC725YKeys:
+ } else if ( + bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_AllowedERC725YDataKeys_PREFIX + ) { + return + _getPermissionToSetAllowedERC725YDataKeys( + controlledContract, + inputDataKey, + inputDataValue + ); + + // if the first 6 bytes of the input data key are "AddressPermissions:..." but did not match + // with anything above, this is not a standard LSP6 permission data key so we revert. + } else { + /** + * @dev more permissions types starting with `AddressPermissions:...` can be implemented by overriding this function. + * + * // AddressPermissions:MyCustomPermissions:
+ * bytes12 CUSTOM_PERMISSION_PREFIX = 0x4b80742de2bf9e659ba40000 + * + * if (bytes12(dataKey) == CUSTOM_PERMISSION_PREFIX) { + * // custom logic + * } + * + * super._getPermissionRequiredToSetDataKey(...) + */ + revert NotRecognisedPermissionKey(inputDataKey); + } + + // LSP1UniversalReceiverDelegate or LSP1UniversalReceiverDelegate: + } else if ( + inputDataKey == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY || + bytes12(inputDataKey) == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX + ) { + return _getPermissionToSetLSP1Delegate(controlledContract, inputDataKey); + + // LSP17Extension: + } else if (bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX) { + return _getPermissionToSetLSP17Extension(controlledContract, inputDataKey); + } else { + return _PERMISSION_SETDATA; + } + } + + /** + * @dev retrieve the permission required to update the `AddressPermissions[]` array data key defined in LSP6. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param inputDataKey either `AddressPermissions[]` (array length) or `AddressPermissions[index]` (array index) + * @param inputDataValue the updated value for the `inputDataKey`. MUST be: + * - a `uint256` for `AddressPermissions[]` (array length) + * - an `address` or `0x` for `AddressPermissions[index]` (array entry). + * + * @return either ADD or CHANGE PERMISSIONS. + */ + function _getPermissionToSetPermissionsArray( + address controlledContract, + bytes32 inputDataKey, + bytes memory inputDataValue + ) internal view virtual returns (bytes32) { + bytes memory currentValue = ERC725Y(controlledContract).getData(inputDataKey); + + // AddressPermissions[] -> array length + if (inputDataKey == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY) { + uint256 newLength = uint256(bytes32(inputDataValue)); + + return + newLength > uint256(bytes32(currentValue)) + ? _PERMISSION_ADDCONTROLLER + : _PERMISSION_CHANGEPERMISSIONS; + } + + // AddressPermissions[index] -> array index + + // CHECK that we either ADD an address (20 bytes long) or REMOVE an address (0x) + if (inputDataValue.length != 0 && inputDataValue.length != 20) { + revert AddressPermissionArrayIndexValueNotAnAddress(inputDataKey, inputDataValue); + } + + return currentValue.length == 0 ? _PERMISSION_ADDCONTROLLER : _PERMISSION_CHANGEPERMISSIONS; + } + + /** + * @dev retrieve the permission required to set permissions for a controller address. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param inputPermissionDataKey `AddressPermissions:Permissions:`. + * @return either ADD or CHANGE PERMISSIONS. + */ + function _getPermissionToSetControllerPermissions( + address controlledContract, + bytes32 inputPermissionDataKey + ) internal view virtual returns (bytes32) { + return + // if there is nothing stored under the data key, we are trying to ADD a new controller. + // if there are already some permissions set under the data key, we are trying to CHANGE the permissions of a controller. + bytes32(ERC725Y(controlledContract).getData(inputPermissionDataKey)) == bytes32(0) + ? _PERMISSION_ADDCONTROLLER + : _PERMISSION_CHANGEPERMISSIONS; + } + + /** + * @dev retrieve the permission required to set some AllowedCalls for a controller. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param dataKey `AddressPermissions:AllowedCalls:`. + * @param dataValue the updated value for the `dataKey`. MUST be a bytes28[CompactBytesArray] of Allowed Calls. + * @return either ADD or CHANGE PERMISSIONS. + */ + function _getPermissionToSetAllowedCalls( + address controlledContract, + bytes32 dataKey, + bytes memory dataValue + ) internal view virtual returns (bytes32) { + if (!LSP6Utils.isCompactBytesArrayOfAllowedCalls(dataValue)) { + revert InvalidEncodedAllowedCalls(dataValue); + } + + // if there is nothing stored under the Allowed Calls of the controller, + // we are trying to ADD a list of restricted calls (standards + address + function selector) + // + // if there are already some data set under the Allowed Calls of the controller, + // we are trying to CHANGE (= edit) these restrictions. + return + ERC725Y(controlledContract).getData(dataKey).length == 0 + ? _PERMISSION_ADDCONTROLLER + : _PERMISSION_CHANGEPERMISSIONS; + } + + /** + * @dev retrieve the permission required to set some AllowedCalls for a controller. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param dataKey or `AddressPermissions:AllowedERC725YDataKeys:`. + * @param dataValue the updated value for the `dataKey`. MUST be a bytes[CompactBytesArray] of Allowed ERC725Y Data Keys. + * @return either ADD or CHANGE PERMISSIONS. + */ + function _getPermissionToSetAllowedERC725YDataKeys( + address controlledContract, + bytes32 dataKey, + bytes memory dataValue + ) internal view returns (bytes32) { + if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(dataValue)) { + revert InvalidEncodedAllowedERC725YDataKeys(dataValue); + } + + // if there is nothing stored under the Allowed ERC725Y Data Keys of the controller, + // we are trying to ADD a list of restricted ERC725Y Data Keys. + // + // if there are already some data set under the Allowed ERC725Y Data Keys of the controller, + // we are trying to CHANGE (= edit) these restricted ERC725Y data keys. + return + ERC725Y(controlledContract).getData(dataKey).length == 0 + ? _PERMISSION_ADDCONTROLLER + : _PERMISSION_CHANGEPERMISSIONS; + } + + /** + * @dev retrieve the permission required to either add or change the address + * of a LSP1 Universal Receiver Delegate stored under a specific LSP1 data key. + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param lsp1DelegateDataKey either the data key for the default `LSP1UniversalReceiverDelegate`, + * or a data key for a specific `LSP1UniversalReceiverDelegate:`, starting with `_LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX`. + * @return either ADD or CHANGE UNIVERSALRECEIVERDELEGATE. + */ + function _getPermissionToSetLSP1Delegate( + address controlledContract, + bytes32 lsp1DelegateDataKey + ) internal view virtual returns (bytes32) { + return + ERC725Y(controlledContract).getData(lsp1DelegateDataKey).length == 0 + ? _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE + : _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE; + } + + /** + * @dev Verify if `controller` has the required permissions to either add or change the address + * of an LSP0 Extension stored under a specific LSP17Extension data key + * @param controlledContract the address of the ERC725Y contract where the data key is verified. + * @param lsp17ExtensionDataKey the dataKey to set with `_LSP17_EXTENSION_PREFIX` as prefix. + */ + function _getPermissionToSetLSP17Extension( + address controlledContract, + bytes32 lsp17ExtensionDataKey + ) internal view virtual returns (bytes32) { + return + ERC725Y(controlledContract).getData(lsp17ExtensionDataKey).length == 0 + ? _PERMISSION_ADDEXTENSIONS + : _PERMISSION_CHANGEEXTENSIONS; + } + + /** + * @dev Verify if the `inputKey` is present in the list of `allowedERC725KeysCompacted` for the `controllerAddress`. + * @param controllerAddress the address of the controller. + * @param inputDataKey the data key to verify against the allowed ERC725Y Data Keys for the `controllerAddress`. + * @param allowedERC725YDataKeysCompacted a CompactBytesArray of allowed ERC725Y Data Keys for the `controllerAddress`. + */ + function _verifyAllowedERC725YSingleKey( + address controllerAddress, + bytes32 inputDataKey, + bytes memory allowedERC725YDataKeysCompacted + ) internal pure virtual { + if (allowedERC725YDataKeysCompacted.length == 0) + revert NoERC725YDataKeysAllowed(controllerAddress); + + if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted)) + revert InvalidEncodedAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted); + + /** + * The pointer will always land on the length of each bytes value: + * + * ↓↓ + * 03 a00000 + * 05 fff83a0011 + * 20 aa0000000000000000000000000000000000000000000000000000000000cafe + * 12 bb000000000000000000000000000000beef + * 19 cc00000000000000000000000000000000000000000000deed + * ↑↑ + * + */ + uint256 pointer; + + // information extracted from each Allowed ERC725Y Data Key. + uint256 length; + bytes32 allowedKey; + bytes32 mask; + + /** + * iterate over each data key and update the `pointer` variable with the index where to find the length of each data key. + * + * 0x 03 a00000 03 fff83a 20 aa00...00cafe + * ↑↑ ↑↑ ↑↑ + * first | second | third + * length | length | length + */ + while (pointer < allowedERC725YDataKeysCompacted.length) { + // save the length of the allowed data key to calculate the `mask`. + length = uint16( + bytes2( + abi.encodePacked( + allowedERC725YDataKeysCompacted[pointer], + allowedERC725YDataKeysCompacted[pointer + 1] + ) + ) + ); + + /** + * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & + * It is used to compare only the relevant parts of each input data key against dynamic allowed data keys. + * + * E.g.: + * + * allowed data key = 0xa00000 + * + * compare this part + * vvvvvv + * input data key = 0xa00000cafecafecafecafecafecafecafe000000000000000000000011223344 + * + * & discard this part + * vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv + * mask = 0xffffff0000000000000000000000000000000000000000000000000000000000 + */ + mask = + bytes32(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) << + (8 * (32 - length)); + + /* + * transform the allowed data key situated from `pointer + 1` until `pointer + 1 + length` to a bytes32 value. + * E.g. 0xfff83a -> 0xfff83a0000000000000000000000000000000000000000000000000000000000 + */ + // solhint-disable-next-line no-inline-assembly + assembly { + // the first 32 bytes word in memory (where allowedERC725YDataKeysCompacted is stored) + // correspond to the total number of bytes in `allowedERC725YDataKeysCompacted` + let offset := add(add(pointer, 2), 32) + let memoryAt := mload(add(allowedERC725YDataKeysCompacted, offset)) + // MLOAD loads 32 bytes word, so we need to keep only the `length` number of bytes that makes up the allowed data key. + allowedKey := and(memoryAt, mask) + } + + // voila you found the key ;) + if (allowedKey == (inputDataKey & mask)) return; + + // move the pointer to the index of the next allowed data key + unchecked { + pointer = pointer + (length + 2); + } + } + + revert NotAllowedERC725YDataKey(controllerAddress, inputDataKey); + } + + /** + * @dev Verify if all the `inputDataKeys` are present in the list of `allowedERC725KeysCompacted` of the `controllerAddress`. + * @param controllerAddress the address of the controller. + * @param inputDataKeys the data keys to verify against the allowed ERC725Y Data Keys of the `controllerAddress`. + * @param allowedERC725YDataKeysCompacted a CompactBytesArray of allowed ERC725Y Data Keys of the `controllerAddress`. + * @param validatedInputKeys an array of booleans to store the result of the verification of each data keys checked. + */ + function _verifyAllowedERC725YDataKeys( + address controllerAddress, + bytes32[] memory inputDataKeys, + bytes memory allowedERC725YDataKeysCompacted, + bool[] memory validatedInputKeys + ) internal pure virtual { + if (allowedERC725YDataKeysCompacted.length == 0) + revert NoERC725YDataKeysAllowed(controllerAddress); + + if (!LSP6Utils.isCompactBytesArrayOfAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted)) + revert InvalidEncodedAllowedERC725YDataKeys(allowedERC725YDataKeysCompacted); + + uint256 allowedKeysFound; + + // cache the input data keys from the start + uint256 inputKeysLength = inputDataKeys.length; + + /** + * The pointer will always land on the length of each bytes value: + * + * ↓↓ + * 03 a00000 + * 05 fff83a0011 + * 20 aa0000000000000000000000000000000000000000000000000000000000cafe + * 12 bb000000000000000000000000000000beef + * 19 cc00000000000000000000000000000000000000000000deed + * ↑↑ + * + */ + uint256 pointer; + + // information extracted from each Allowed ERC725Y Data Key. + uint256 length; + bytes32 allowedKey; + bytes32 mask; + + /** + * iterate over each data key and update the `pointer` variable with the index where to find the length of each data key. + * + * 0x 03 a00000 03 fff83a 20 aa00...00cafe + * ↑↑ ↑↑ ↑↑ + * first | second | third + * length | length | length + */ + while (pointer < allowedERC725YDataKeysCompacted.length) { + // save the length of the allowed data key to calculate the `mask`. + length = uint16( + bytes2( + abi.encodePacked( + allowedERC725YDataKeysCompacted[pointer], + allowedERC725YDataKeysCompacted[pointer + 1] + ) + ) + ); + + /** + * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & + * It is used to compare only the relevant parts of each input data key against dynamic allowed data keys. + * + * E.g.: + * + * allowed data key = 0xa00000 + * + * compare this part + * vvvvvv + * input data key = 0xa00000cafecafecafecafecafecafecafe000000000000000000000011223344 + * + * & discard this part + * vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv + * mask = 0xffffff0000000000000000000000000000000000000000000000000000000000 + */ + mask = + bytes32(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) << + (8 * (32 - length)); + + /* + * transform the allowed data key situated from `pointer + 1` until `pointer + 1 + length` to a bytes32 value. + * E.g. 0xfff83a -> 0xfff83a0000000000000000000000000000000000000000000000000000000000 + */ + // solhint-disable-next-line no-inline-assembly + assembly { + // the first 32 bytes word in memory (where allowedERC725YDataKeysCompacted is stored) + // correspond to the length of allowedERC725YDataKeysCompacted (= total number of bytes) + let offset := add(add(pointer, 2), 32) + let memoryAt := mload(add(allowedERC725YDataKeysCompacted, offset)) + allowedKey := and(memoryAt, mask) + } + + /** + * Iterate over the `inputDataKeys` to check them against the allowed data keys. + * This until we have validated them all. + */ + for (uint256 ii; ii < inputKeysLength; ii = GasLib.uncheckedIncrement(ii)) { + // if the input data key has been marked as allowed previously, + // SKIP it and move to the next input data key. + if (validatedInputKeys[ii]) continue; + + // CHECK if the input data key is allowed. + if ((inputDataKeys[ii] & mask) == allowedKey) { + // if the input data key is allowed, mark it as allowed + // and increment the number of allowed keys found. + validatedInputKeys[ii] = true; + allowedKeysFound = GasLib.uncheckedIncrement(allowedKeysFound); + + // Continue checking until all the inputKeys` have been found. + if (allowedKeysFound == inputKeysLength) return; + } + } + + // Move the pointer to the next AllowedERC725YKey + unchecked { + pointer = pointer + (length + 2); + } + } + + // if we did not find all the input data keys, search for the first not allowed data key to revert. + for (uint256 jj; jj < inputKeysLength; jj = GasLib.uncheckedIncrement(jj)) { + if (!validatedInputKeys[jj]) { + revert NotAllowedERC725YDataKey(controllerAddress, inputDataKeys[jj]); + } + } + } +} diff --git a/contracts/LSP6KeyManager/LSP6Utils.sol b/contracts/LSP6KeyManager/LSP6Utils.sol index ed91064e6..7108d648b 100644 --- a/contracts/LSP6KeyManager/LSP6Utils.sol +++ b/contracts/LSP6KeyManager/LSP6Utils.sol @@ -10,7 +10,10 @@ import {LSP2Utils} from "../LSP2ERC725YJSONSchema/LSP2Utils.sol"; // constants import {SETDATA_ARRAY_SELECTOR} from "@erc725/smart-contracts/contracts/constants.sol"; -import "../LSP6KeyManager/LSP6Constants.sol"; +import "./LSP6Constants.sol"; + +// errors +import {NotAuthorised} from "./LSP6Errors.sol"; library LSP6Utils { using LSP2Utils for bytes12; @@ -196,4 +199,48 @@ library LSP6Utils { ); values[2] = abi.encodePacked(permissions); } + + /** + * @dev returns the name of the permission as a string + */ + function getPermissionName(bytes32 permission) + internal + pure + returns (string memory errorMessage) + { + if (permission == _PERMISSION_CHANGEOWNER) return "TRANSFEROWNERSHIP"; + if (permission == _PERMISSION_CHANGEPERMISSIONS) return "CHANGEPERMISSIONS"; + if (permission == _PERMISSION_ADDCONTROLLER) return "ADDCONTROLLER"; + if (permission == _PERMISSION_ADDEXTENSIONS) return "ADDEXTENSIONS"; + if (permission == _PERMISSION_CHANGEEXTENSIONS) return "CHANGEEXTENSIONS"; + if (permission == _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE) + return "ADDUNIVERSALRECEIVERDELEGATE"; + if (permission == _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE) + return "CHANGEUNIVERSALRECEIVERDELEGATE"; + if (permission == _PERMISSION_REENTRANCY) return "REENTRANCY"; + if (permission == _PERMISSION_SETDATA) return "SETDATA"; + if (permission == _PERMISSION_CALL) return "CALL"; + if (permission == _PERMISSION_STATICCALL) return "STATICCALL"; + if (permission == _PERMISSION_DELEGATECALL) return "DELEGATECALL"; + if (permission == _PERMISSION_DEPLOY) return "DEPLOY"; + if (permission == _PERMISSION_TRANSFERVALUE) return "TRANSFERVALUE"; + if (permission == _PERMISSION_SIGN) return "SIGN"; + } + + /** + * @dev revert if `controller`'s `addressPermissions` doesn't contain `permissionsRequired` + * @param controller the caller address + * @param addressPermissions the caller's permissions BitArray + * @param permissionRequired the required permission + */ + function requirePermissions( + address controller, + bytes32 addressPermissions, + bytes32 permissionRequired + ) internal pure { + if (!hasPermission(addressPermissions, permissionRequired)) { + string memory permissionErrorString = getPermissionName(permissionRequired); + revert NotAuthorised(controller, permissionErrorString); + } + } } diff --git a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol index 7f9685a78..179b46fa0 100644 --- a/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol +++ b/contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol @@ -29,7 +29,7 @@ contract KeyManagerInternalTester is LSP6KeyManager { } function verifyAllowedCall(address _sender, bytes calldata _payload) public view { - super._verifyAllowedCall(_sender, _payload); + super._verifyAllowedCall(_target, _sender, _payload); } function isCompactBytesArrayOfAllowedCalls(bytes memory allowedCallsCompacted) From 4a721ab151372b1e3289a135ab55e9b7503bd2a9 Mon Sep 17 00:00:00 2001 From: b00ste Date: Tue, 14 Feb 2023 13:04:53 +0200 Subject: [PATCH 2/3] chore: add suggested changes --- .../LSP6KeyManager/LSP6KeyManagerCore.sol | 39 +++++++++++----- .../LSP6OwnershipLogicModule.sol | 21 --------- .../LSP6ExecuteModule.sol} | 32 +++++++++---- .../LSP6Modules/LSP6OwnershipModule.sol | 25 ++++++++++ .../LSP6SetDataModule.sol} | 46 ++++++++++--------- contracts/LSP6KeyManager/LSP6Utils.sol | 17 ------- 6 files changed, 99 insertions(+), 81 deletions(-) delete mode 100644 contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol rename contracts/LSP6KeyManager/{LSP6LogicModules/LSP6ExecuteLogicModule.sol => LSP6Modules/LSP6ExecuteModule.sol} (88%) create mode 100644 contracts/LSP6KeyManager/LSP6Modules/LSP6OwnershipModule.sol rename contracts/LSP6KeyManager/{LSP6LogicModules/LSP6SetDataLogicModule.sol => LSP6Modules/LSP6SetDataModule.sol} (95%) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 3c881f615..1db4eb5b7 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -9,9 +9,9 @@ import {ILSP6KeyManager} from "./ILSP6KeyManager.sol"; import {ILSP14Ownable2Step} from "../LSP14Ownable2Step/ILSP14Ownable2Step.sol"; import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; -import {LSP6SetDataLogicModule} from "./LSP6LogicModules/LSP6SetDataLogicModule.sol"; -import {LSP6ExecuteLogicModule} from "./LSP6LogicModules/LSP6ExecuteLogicModule.sol"; -import {LSP6OwnershipLogicModule} from "./LSP6LogicModules/LSP6OwnershipLogicModule.sol"; +import {LSP6SetDataModule} from "./LSP6Modules/LSP6SetDataModule.sol"; +import {LSP6ExecuteModule} from "./LSP6Modules/LSP6ExecuteModule.sol"; +import {LSP6OwnershipModule} from "./LSP6Modules/LSP6OwnershipModule.sol"; // libraries import {GasLib} from "../Utils/GasLib.sol"; @@ -30,7 +30,8 @@ import { InvalidPayload, InvalidRelayNonce, NoPermissionsSet, - InvalidERC725Function + InvalidERC725Function, + NotAuthorised } from "./LSP6Errors.sol"; // constants @@ -60,9 +61,9 @@ import { abstract contract LSP6KeyManagerCore is ERC165, ILSP6KeyManager, - LSP6SetDataLogicModule, - LSP6ExecuteLogicModule, - LSP6OwnershipLogicModule + LSP6SetDataModule, + LSP6ExecuteModule, + LSP6OwnershipModule { using LSP6Utils for *; using ECDSA for bytes32; @@ -310,7 +311,7 @@ abstract contract LSP6KeyManagerCore is if (erc725Function == SETDATA_SELECTOR) { (bytes32 inputKey, bytes memory inputValue) = abi.decode(payload[4:], (bytes32, bytes)); - _verifyCanSetData(_target, from, permissions, inputKey, inputValue); + LSP6SetDataModule._verifyCanSetData(_target, from, permissions, inputKey, inputValue); // ERC725Y.setData(bytes32[],bytes[]) } else if (erc725Function == SETDATA_ARRAY_SELECTOR) { @@ -319,16 +320,16 @@ abstract contract LSP6KeyManagerCore is (bytes32[], bytes[]) ); - _verifyCanSetData(_target, from, permissions, inputKeys, inputValues); + LSP6SetDataModule._verifyCanSetData(_target, from, permissions, inputKeys, inputValues); // ERC725X.execute(uint256,address,uint256,bytes) } else if (erc725Function == EXECUTE_SELECTOR) { - _verifyCanExecute(_target, from, permissions, payload); + LSP6ExecuteModule._verifyCanExecute(_target, from, permissions, payload); } else if ( erc725Function == ILSP14Ownable2Step.transferOwnership.selector || erc725Function == ILSP14Ownable2Step.acceptOwnership.selector ) { - _verifyOwnershipPermissions(from, permissions); + LSP6OwnershipModule._verifyOwnershipPermissions(from, permissions); } else { revert InvalidERC725Function(erc725Function); } @@ -349,7 +350,7 @@ abstract contract LSP6KeyManagerCore is function _nonReentrantBefore(address from) internal virtual { if (_reentrancyStatus) { // CHECK the caller has REENTRANCY permission - LSP6Utils.requirePermissions( + requirePermissions( from, ERC725Y(_target).getPermissionsFor(from), _PERMISSION_REENTRANCY @@ -368,4 +369,18 @@ abstract contract LSP6KeyManagerCore is // https://eips.ethereum.org/EIPS/eip-2200) _reentrancyStatus = false; } + + /** + * @dev revert if `controller`'s `addressPermissions` doesn't contain `permissionsRequired` + * @param controller the caller address + * @param addressPermissions the caller's permissions BitArray + * @param permissionRequired the required permission + */ + function requirePermissions( + address controller, + bytes32 addressPermissions, + bytes32 permissionRequired + ) internal pure override(LSP6ExecuteModule, LSP6SetDataModule) { + LSP6ExecuteModule.requirePermissions(controller, addressPermissions, permissionRequired); + } } diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol b/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol deleted file mode 100644 index 746a2a59e..000000000 --- a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6OwnershipLogicModule.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -pragma solidity ^0.8.5; - -// libraries -import {LSP6Utils} from "../LSP6Utils.sol"; - -// constants -import {_PERMISSION_CHANGEOWNER} from "../LSP6Constants.sol"; - -abstract contract LSP6OwnershipLogicModule { - function _verifyOwnershipPermissions(address controllerAddress, bytes32 controllerPermissions) - internal - pure - { - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - _PERMISSION_CHANGEOWNER - ); - } -} diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol similarity index 88% rename from contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol rename to contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol index ea0716aef..274dab6ec 100644 --- a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6ExecuteLogicModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol @@ -35,10 +35,11 @@ import { NoCallsAllowed, NotAllowedCall, InvalidEncodedAllowedCalls, - InvalidWhitelistedCall + InvalidWhitelistedCall, + NotAuthorised } from "../LSP6Errors.sol"; -abstract contract LSP6ExecuteLogicModule { +abstract contract LSP6ExecuteModule { using ERC165Checker for address; using LSP6Utils for *; @@ -79,7 +80,7 @@ abstract contract LSP6ExecuteLogicModule { // CHECK if we are doing an empty call, as the receive() or fallback() function // of the controlledContract could run some code. if (!hasSuperOperation && !isCallDataPresent && value == 0) { - LSP6Utils.requirePermissions( + requirePermissions( controllerAddress, controllerPermissions, _extractPermissionFromOperation(operationType) @@ -87,7 +88,7 @@ abstract contract LSP6ExecuteLogicModule { } if (isCallDataPresent && !hasSuperOperation) { - LSP6Utils.requirePermissions( + requirePermissions( controllerAddress, controllerPermissions, _extractPermissionFromOperation(operationType) @@ -99,11 +100,7 @@ abstract contract LSP6ExecuteLogicModule { ); if (value != 0 && !hasSuperTransferValue) { - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - _PERMISSION_TRANSFERVALUE - ); + requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_TRANSFERVALUE); } // Skip on contract creation (CREATE or CREATE2) @@ -208,4 +205,21 @@ abstract contract LSP6ExecuteLogicModule { else if (operationType == OPERATION_3_STATICCALL) return _PERMISSION_SUPER_STATICCALL; else if (operationType == OPERATION_4_DELEGATECALL) return _PERMISSION_SUPER_DELEGATECALL; } + + /** + * @dev revert if `controller`'s `addressPermissions` doesn't contain `permissionsRequired` + * @param controller the caller address + * @param addressPermissions the caller's permissions BitArray + * @param permissionRequired the required permission + */ + function requirePermissions( + address controller, + bytes32 addressPermissions, + bytes32 permissionRequired + ) internal pure virtual { + if (!LSP6Utils.hasPermission(addressPermissions, permissionRequired)) { + string memory permissionErrorString = LSP6Utils.getPermissionName(permissionRequired); + revert NotAuthorised(controller, permissionErrorString); + } + } } diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6OwnershipModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6OwnershipModule.sol new file mode 100644 index 000000000..7e33e7959 --- /dev/null +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6OwnershipModule.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.5; + +// libraries +import {LSP6Utils} from "../LSP6Utils.sol"; + +// constants +import {_PERMISSION_CHANGEOWNER} from "../LSP6Constants.sol"; + +// errors +import {NotAuthorised} from "../LSP6Errors.sol"; + +abstract contract LSP6OwnershipModule { + function _verifyOwnershipPermissions(address controllerAddress, bytes32 controllerPermissions) + internal + pure + { + if (!LSP6Utils.hasPermission(controllerPermissions, _PERMISSION_CHANGEOWNER)) { + string memory permissionErrorString = LSP6Utils.getPermissionName( + _PERMISSION_CHANGEOWNER + ); + revert NotAuthorised(controllerAddress, permissionErrorString); + } + } +} diff --git a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol similarity index 95% rename from contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol rename to contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol index 9a7b8d7de..c5f7846e7 100644 --- a/contracts/LSP6KeyManager/LSP6LogicModules/LSP6SetDataLogicModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol @@ -38,10 +38,11 @@ import { InvalidEncodedAllowedCalls, InvalidEncodedAllowedERC725YDataKeys, NoERC725YDataKeysAllowed, - NotAllowedERC725YDataKey + NotAllowedERC725YDataKey, + NotAuthorised } from "../LSP6Errors.sol"; -abstract contract LSP6SetDataLogicModule { +abstract contract LSP6SetDataModule { using LSP6Utils for *; /** @@ -70,11 +71,7 @@ abstract contract LSP6SetDataLogicModule { // Skip if caller has SUPER permissions if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - _PERMISSION_SETDATA - ); + requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); _verifyAllowedERC725YSingleKey( controllerAddress, @@ -83,11 +80,7 @@ abstract contract LSP6SetDataLogicModule { ); } else { // Otherwise CHECK the required permission if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - requiredPermission - ); + requirePermissions(controllerAddress, controllerPermissions, requiredPermission); } } @@ -123,11 +116,7 @@ abstract contract LSP6SetDataLogicModule { isSettingERC725YKeys = true; } else { // CHECK the required permissions if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - requiredPermission - ); + requirePermissions(controllerAddress, controllerPermissions, requiredPermission); validatedInputDataKeys[ii] = true; } @@ -139,11 +128,7 @@ abstract contract LSP6SetDataLogicModule { // Skip if caller has SUPER permissions if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - LSP6Utils.requirePermissions( - controllerAddress, - controllerPermissions, - _PERMISSION_SETDATA - ); + requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); _verifyAllowedERC725YDataKeys( controllerAddress, @@ -601,4 +586,21 @@ abstract contract LSP6SetDataLogicModule { } } } + + /** + * @dev revert if `controller`'s `addressPermissions` doesn't contain `permissionsRequired` + * @param controller the caller address + * @param addressPermissions the caller's permissions BitArray + * @param permissionRequired the required permission + */ + function requirePermissions( + address controller, + bytes32 addressPermissions, + bytes32 permissionRequired + ) internal pure virtual { + if (!LSP6Utils.hasPermission(addressPermissions, permissionRequired)) { + string memory permissionErrorString = LSP6Utils.getPermissionName(permissionRequired); + revert NotAuthorised(controller, permissionErrorString); + } + } } diff --git a/contracts/LSP6KeyManager/LSP6Utils.sol b/contracts/LSP6KeyManager/LSP6Utils.sol index 7108d648b..9a86beb50 100644 --- a/contracts/LSP6KeyManager/LSP6Utils.sol +++ b/contracts/LSP6KeyManager/LSP6Utils.sol @@ -226,21 +226,4 @@ library LSP6Utils { if (permission == _PERMISSION_TRANSFERVALUE) return "TRANSFERVALUE"; if (permission == _PERMISSION_SIGN) return "SIGN"; } - - /** - * @dev revert if `controller`'s `addressPermissions` doesn't contain `permissionsRequired` - * @param controller the caller address - * @param addressPermissions the caller's permissions BitArray - * @param permissionRequired the required permission - */ - function requirePermissions( - address controller, - bytes32 addressPermissions, - bytes32 permissionRequired - ) internal pure { - if (!hasPermission(addressPermissions, permissionRequired)) { - string memory permissionErrorString = getPermissionName(permissionRequired); - revert NotAuthorised(controller, permissionErrorString); - } - } } From c24fb270469b3c4c90f86ff95c3497516200abde Mon Sep 17 00:00:00 2001 From: b00ste Date: Tue, 14 Feb 2023 13:07:08 +0200 Subject: [PATCH 3/3] chore: fix linter error --- contracts/LSP6KeyManager/LSP6KeyManagerCore.sol | 6 +++--- .../LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol | 12 ++++++++---- .../LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol | 10 +++++----- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 1db4eb5b7..353b5b22b 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -350,7 +350,7 @@ abstract contract LSP6KeyManagerCore is function _nonReentrantBefore(address from) internal virtual { if (_reentrancyStatus) { // CHECK the caller has REENTRANCY permission - requirePermissions( + _requirePermissions( from, ERC725Y(_target).getPermissionsFor(from), _PERMISSION_REENTRANCY @@ -376,11 +376,11 @@ abstract contract LSP6KeyManagerCore is * @param addressPermissions the caller's permissions BitArray * @param permissionRequired the required permission */ - function requirePermissions( + function _requirePermissions( address controller, bytes32 addressPermissions, bytes32 permissionRequired ) internal pure override(LSP6ExecuteModule, LSP6SetDataModule) { - LSP6ExecuteModule.requirePermissions(controller, addressPermissions, permissionRequired); + LSP6ExecuteModule._requirePermissions(controller, addressPermissions, permissionRequired); } } diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol index 274dab6ec..f8882a4af 100644 --- a/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol @@ -80,7 +80,7 @@ abstract contract LSP6ExecuteModule { // CHECK if we are doing an empty call, as the receive() or fallback() function // of the controlledContract could run some code. if (!hasSuperOperation && !isCallDataPresent && value == 0) { - requirePermissions( + _requirePermissions( controllerAddress, controllerPermissions, _extractPermissionFromOperation(operationType) @@ -88,7 +88,7 @@ abstract contract LSP6ExecuteModule { } if (isCallDataPresent && !hasSuperOperation) { - requirePermissions( + _requirePermissions( controllerAddress, controllerPermissions, _extractPermissionFromOperation(operationType) @@ -100,7 +100,11 @@ abstract contract LSP6ExecuteModule { ); if (value != 0 && !hasSuperTransferValue) { - requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_TRANSFERVALUE); + _requirePermissions( + controllerAddress, + controllerPermissions, + _PERMISSION_TRANSFERVALUE + ); } // Skip on contract creation (CREATE or CREATE2) @@ -212,7 +216,7 @@ abstract contract LSP6ExecuteModule { * @param addressPermissions the caller's permissions BitArray * @param permissionRequired the required permission */ - function requirePermissions( + function _requirePermissions( address controller, bytes32 addressPermissions, bytes32 permissionRequired diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol index c5f7846e7..7d41b46f3 100644 --- a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol @@ -71,7 +71,7 @@ abstract contract LSP6SetDataModule { // Skip if caller has SUPER permissions if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); + _requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); _verifyAllowedERC725YSingleKey( controllerAddress, @@ -80,7 +80,7 @@ abstract contract LSP6SetDataModule { ); } else { // Otherwise CHECK the required permission if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - requirePermissions(controllerAddress, controllerPermissions, requiredPermission); + _requirePermissions(controllerAddress, controllerPermissions, requiredPermission); } } @@ -116,7 +116,7 @@ abstract contract LSP6SetDataModule { isSettingERC725YKeys = true; } else { // CHECK the required permissions if setting LSP6 permissions, LSP1 Delegate or LSP17 Extensions. - requirePermissions(controllerAddress, controllerPermissions, requiredPermission); + _requirePermissions(controllerAddress, controllerPermissions, requiredPermission); validatedInputDataKeys[ii] = true; } @@ -128,7 +128,7 @@ abstract contract LSP6SetDataModule { // Skip if caller has SUPER permissions if (controllerPermissions.hasPermission(_PERMISSION_SUPER_SETDATA)) return; - requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); + _requirePermissions(controllerAddress, controllerPermissions, _PERMISSION_SETDATA); _verifyAllowedERC725YDataKeys( controllerAddress, @@ -593,7 +593,7 @@ abstract contract LSP6SetDataModule { * @param addressPermissions the caller's permissions BitArray * @param permissionRequired the required permission */ - function requirePermissions( + function _requirePermissions( address controller, bytes32 addressPermissions, bytes32 permissionRequired