From 90c647180df08730a74243289fdf9b3971373fbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Wed, 29 Nov 2023 15:36:49 +0100 Subject: [PATCH] Updated plugin setup's and conditions --- README.md | 28 ++++++++- .../contracts/src/GovernancePluginsSetup.sol | 60 ++++++------------- .../src/MemberAccessExecuteCondition.sol | 30 ++++++++-- .../src/OnlyPluginUpgraderCondition.sol | 50 ++++++++-------- packages/contracts/src/SpacePluginSetup.sol | 59 +++++++++--------- 5 files changed, 123 insertions(+), 104 deletions(-) diff --git a/README.md b/README.md index b0c1542..4f47f03 100644 --- a/README.md +++ b/README.md @@ -440,6 +440,10 @@ See `SpacePluginSetup`, `PersonalSpaceAdminPluginSetup`, `MemberAccessPluginSetu [Learn more about plugin setup's](https://devs.aragon.org/docs/osx/how-it-works/framework/plugin-management/plugin-setup/) and [preparing installations](https://devs.aragon.org/docs/sdk/examples/client/prepare-installation). +### PluginSetup contracts + +(They need to receive the PSP Address) + ### Plugin Setup install parameters In both of the cases described above, a call to `prepareInstallation()` will be made by the `PluginSetupProcessor` from OSx. @@ -561,9 +565,29 @@ The format of these settings is defined in the `packages/contracts/src/*-build.m ## Plugin upgradeability -By default, only the DAO can upgrade plugins to newer versions. This requires passing a proposal. For the 3 upgradeable plugins, their plugin setup allows to pass an optional parameter to define a plugin upgrader address. +The first step to upgrade a plugin is calling `ThePluginSetup.prepareUpdate()`, which will register an update request on the `PluginSetupProcessor`. See [Plugin Setup install parameters](#plugin-setup-install-parameters) above. + +By default, only the DAO can upgrade plugins to newer versions. This requires passing a proposal with these 3 actions: + +Action 1: + +- Grant `UPGRADE_PLUGIN_PERMISSION_ID` to the PSP on the target plugin + +Action 2: + +- Make the DAO call `PSP.applyUpdate()` with the ID generated during `prepareUpdate()` + +Action 3: + +- Revoke `UPGRADE_PLUGIN_PERMISSION_ID` to the PSP on the target plugin + +The address of the `PluginSetupProcessor` depends on the chain. The existing deployments [can be checked here](https://github.com/aragon/osx/blob/develop/active_contracts.json). + +### Plugin Upgrader + +For the 3 upgradeable plugins, their plugin setup allows to pass an optional parameter to define a plugin upgrader address. -When a zero address is passed, only the DAO can call `upgradeTo()` and `upgradeToAndCall()`. When a non-zero address is passed, the desired address will be able to upgrade to whatever newer version the developer has published. +When a zero address is passed, only a passed proposal can make the DAO call `PSP.applyUpdate()`. When a non-zero address is passed, the desired address will be able to execute the 3 actions abover to upgrade to whatever newer version the developer has published. Every new version needs to be published to the plugin's repository. diff --git a/packages/contracts/src/GovernancePluginsSetup.sol b/packages/contracts/src/GovernancePluginsSetup.sol index 9ce9fd2..9a91165 100644 --- a/packages/contracts/src/GovernancePluginsSetup.sol +++ b/packages/contracts/src/GovernancePluginsSetup.sol @@ -23,8 +23,10 @@ contract GovernancePluginsSetup is PluginSetup { /// @notice Thrown when the array of helpers does not have the correct size error InvalidHelpers(uint256 actualLength); - constructor(PluginSetupProcessor _pluginSetupProcessor) { - pluginSetupProcessor = address(_pluginSetupProcessor); + /// @notice Initializes the setup contract + /// @param pluginSetupProcessorAddress The address of the PluginSetupProcessor contract deployed by Aragon on that chain + constructor(PluginSetupProcessor pluginSetupProcessorAddress) { + pluginSetupProcessor = address(pluginSetupProcessorAddress); mainVotingPluginImplementation = address(new MainVotingPlugin()); memberAccessPluginImplementation = address(new MemberAccessPlugin()); } @@ -160,7 +162,7 @@ contract GovernancePluginsSetup is PluginSetup { address _memberAccessPlugin = _payload.currentHelpers[0]; permissionChanges = new PermissionLib.MultiTargetPermission[]( - _pluginUpgrader == address(0x0) ? 7 : 9 + _pluginUpgrader == address(0x0) ? 5 : 6 ); // Main voting plugin permissions @@ -170,7 +172,7 @@ contract GovernancePluginsSetup is PluginSetup { operation: PermissionLib.Operation.Revoke, where: _dao, who: _payload.plugin, - condition: PermissionLib.NO_CONDITION, + condition: address(0), permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); // The DAO can no longer update the plugin settings @@ -178,7 +180,7 @@ contract GovernancePluginsSetup is PluginSetup { operation: PermissionLib.Operation.Revoke, where: _payload.plugin, who: _dao, - condition: PermissionLib.NO_CONDITION, + condition: address(0), permissionId: MainVotingPlugin(mainVotingPluginImplementation) .UPDATE_VOTING_SETTINGS_PERMISSION_ID() }); @@ -187,66 +189,40 @@ contract GovernancePluginsSetup is PluginSetup { operation: PermissionLib.Operation.Revoke, where: _payload.plugin, who: _dao, - condition: PermissionLib.NO_CONDITION, + condition: address(0), permissionId: MainVotingPlugin(mainVotingPluginImplementation) .UPDATE_ADDRESSES_PERMISSION_ID() }); - // The DAO can no longer upgrade the plugin - permissionChanges[3] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Revoke, - where: _payload.plugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: MainVotingPlugin(mainVotingPluginImplementation) - .UPGRADE_PLUGIN_PERMISSION_ID() - }); // Member access plugin permissions // The plugin can no longer execute on the DAO - permissionChanges[4] = PermissionLib.MultiTargetPermission({ + permissionChanges[3] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: _dao, who: _memberAccessPlugin, - condition: PermissionLib.NO_CONDITION, + condition: address(0), permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); // The DAO can no longer update the plugin settings - permissionChanges[5] = PermissionLib.MultiTargetPermission({ + permissionChanges[4] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: _memberAccessPlugin, who: _dao, - condition: PermissionLib.NO_CONDITION, + condition: address(0), permissionId: MemberAccessPlugin(memberAccessPluginImplementation) .UPDATE_MULTISIG_SETTINGS_PERMISSION_ID() }); - // The DAO can no longer upgrade the plugin - permissionChanges[6] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Revoke, - where: _memberAccessPlugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: MemberAccessPlugin(memberAccessPluginImplementation) - .UPGRADE_PLUGIN_PERMISSION_ID() - }); if (_pluginUpgrader != address(0x0)) { - // pluginUpgrader can no longer upgrade the plugins - permissionChanges[7] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Revoke, - where: _payload.plugin, - who: _pluginUpgrader, - condition: PermissionLib.NO_CONDITION, - permissionId: MainVotingPlugin(mainVotingPluginImplementation) - .UPGRADE_PLUGIN_PERMISSION_ID() - }); - permissionChanges[8] = PermissionLib.MultiTargetPermission({ + // pluginUpgrader can no longer make the DAO execute applyUpdate + // pluginUpgrader can no longer make the DAO execute grant/revoke + permissionChanges[5] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, - where: _memberAccessPlugin, + where: _dao, who: _pluginUpgrader, - condition: PermissionLib.NO_CONDITION, - permissionId: MemberAccessPlugin(memberAccessPluginImplementation) - .UPGRADE_PLUGIN_PERMISSION_ID() + condition: address(0), + permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); } } diff --git a/packages/contracts/src/MemberAccessExecuteCondition.sol b/packages/contracts/src/MemberAccessExecuteCondition.sol index 5d6d84c..d3ebdae 100644 --- a/packages/contracts/src/MemberAccessExecuteCondition.sol +++ b/packages/contracts/src/MemberAccessExecuteCondition.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.17; +import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; import {PermissionCondition} from "@aragon/osx/core/permission/PermissionCondition.sol"; import {PermissionManager} from "@aragon/osx/core/permission/PermissionManager.sol"; import {MEMBER_PERMISSION_ID} from "./constants.sol"; @@ -17,8 +18,12 @@ contract MemberAccessExecuteCondition is PermissionCondition { targetContract = _targetContract; } - function getSelector(bytes calldata _data) public pure returns (bytes4 selector) { - selector = bytes4(_data[:4]); + function getSelector(bytes memory _data) public pure returns (bytes4 selector) { + // Slices are only supported for bytes calldata + // Bytes memory requires an assembly block + assembly { + selector := mload(add(_data, 32)) + } } /// @notice Checks whether the current action wants to grant membership on the predefined address @@ -31,15 +36,28 @@ contract MemberAccessExecuteCondition is PermissionCondition { (_where, _who, _permissionId); bytes4 _requestedFuncSig = getSelector(_data); + if (_requestedFuncSig != IDAO.execute.selector) { + return false; + } + + (, IDAO.Action[] memory _actions, ) = abi.decode( + _data[4:], + (bytes32, IDAO.Action[], uint256) + ); + + // Check actions + if (_actions.length != 1) return false; + _requestedFuncSig = getSelector(_actions[0].data); + if ( _requestedFuncSig != PermissionManager.grant.selector && _requestedFuncSig != PermissionManager.revoke.selector ) return false; - // Decode the call being requested - (address _requestedWhere, , bytes32 _requestedPermission) = abi.decode( - _data[4:], - (address, address, bytes32) + // Decode the call being requested (both have the same parameters) + (, address _requestedWhere, , bytes32 _requestedPermission) = abi.decode( + _actions[0].data, + (bytes4 /*funcSig*/, address /*where*/, address /*who*/, bytes32 /*perm*/) ); if (_requestedWhere != targetContract) return false; diff --git a/packages/contracts/src/OnlyPluginUpgraderCondition.sol b/packages/contracts/src/OnlyPluginUpgraderCondition.sol index 9aa5235..4b3b2c6 100644 --- a/packages/contracts/src/OnlyPluginUpgraderCondition.sol +++ b/packages/contracts/src/OnlyPluginUpgraderCondition.sol @@ -40,6 +40,7 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { function getSelector(bytes memory _data) public pure returns (bytes4 selector) { // Slices are only supported for bytes calldata + // Bytes memory requires an assembly block assembly { selector := mload(add(_data, 32)) } @@ -65,31 +66,25 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { ); // Check all actions - for (uint256 i; i < _actions.length; ) { - _requestedFuncSig = getSelector(_actions[i].data); - - // Can only grant/revoke UPGRADE_PLUGIN_PERMISSION_ID to the PSP on the plugins - if ( - _requestedFuncSig == PermissionManager.grant.selector || - _requestedFuncSig == PermissionManager.revoke.selector - ) { - if (!isValidExecuteGrantRevokeCalldata(_actions[i].data)) return false; - else if (_actions[i].to != dao) return false; - } - // Can only make the DAO execute applyUpdate() on the PSP - else if (_requestedFuncSig == PluginSetupProcessor.applyUpdate.selector) { - if (!isValidExecuteApplyUpdateCalldata(_actions[i].data)) return false; - else if (_actions[i].to != psp) return false; - } - // Not allowed - else { - return false; - } - - unchecked { - i++; - } - } + if (_actions.length != 3) return false; + + // Action 1: GRANT UPGRADE_PLUGIN_PERMISSION_ID to the PSP on targetPlugis[] + _requestedFuncSig = getSelector(_actions[0].data); + if (_requestedFuncSig != PermissionManager.grant.selector) return false; + else if (_actions[0].to != dao) return false; + else if (!isValidExecuteGrantRevokeCalldata(_actions[0].data)) return false; + + // Action 2: CALL PSP.applyUpdate() onto targetPlugins[] + _requestedFuncSig = getSelector(_actions[1].data); + if (_requestedFuncSig != PluginSetupProcessor.applyUpdate.selector) return false; + else if (_actions[1].to != psp) return false; + else if (!isValidExecuteApplyUpdateCalldata(_actions[1].data)) return false; + + // Action 3: REVOKE UPGRADE_PLUGIN_PERMISSION_ID to the PSP on targetPlugis[] + _requestedFuncSig = getSelector(_actions[2].data); + if (_requestedFuncSig != PermissionManager.revoke.selector) return false; + else if (_actions[2].to != dao) return false; + else if (!isValidExecuteGrantRevokeCalldata(_actions[2].data)) return false; return true; } @@ -99,7 +94,10 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { function isValidExecuteGrantRevokeCalldata(bytes memory _data) private view returns (bool) { // Decode the call being requested (, address _requestedWhere, address _requestedWho, bytes32 _requestedPermission) = abi - .decode(_data, (bytes4, address, address, bytes32)); + .decode( + _data, + (bytes4 /*funcSig*/, address /*where*/, address /*who*/, bytes32 /*perm*/) + ); if (_requestedPermission != UPGRADE_PLUGIN_PERMISSION_ID) return false; else if (_requestedWho != psp) return false; diff --git a/packages/contracts/src/SpacePluginSetup.sol b/packages/contracts/src/SpacePluginSetup.sol index 21e033e..2a92e08 100644 --- a/packages/contracts/src/SpacePluginSetup.sol +++ b/packages/contracts/src/SpacePluginSetup.sol @@ -2,18 +2,25 @@ pragma solidity ^0.8.8; +import {DAO} from "@aragon/osx/core/dao/DAO.sol"; import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; import {PermissionLib} from "@aragon/osx/core/permission/PermissionLib.sol"; import {PluginSetup, IPluginSetup} from "@aragon/osx/framework/plugin/setup/PluginSetup.sol"; +import {PluginSetupProcessor} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessor.sol"; import {SpacePlugin} from "./SpacePlugin.sol"; +import {OnlyPluginUpgraderCondition} from "./OnlyPluginUpgraderCondition.sol"; import {CONTENT_PERMISSION_ID, SUBSPACE_PERMISSION_ID} from "./constants.sol"; /// @title SpacePluginSetup /// @dev Release 1, Build 1 contract SpacePluginSetup is PluginSetup { address private immutable pluginImplementation; + address private immutable pluginSetupProcessor; - constructor() { + /// @notice Initializes the setup contract + /// @param pluginSetupProcessorAddress The address of the PluginSetupProcessor contract deployed by Aragon on that chain + constructor(PluginSetupProcessor pluginSetupProcessorAddress) { + pluginSetupProcessor = address(pluginSetupProcessorAddress); pluginImplementation = address(new SpacePlugin()); } @@ -40,7 +47,7 @@ contract SpacePluginSetup is PluginSetup { PermissionLib.MultiTargetPermission[] memory permissions = new PermissionLib.MultiTargetPermission[]( - _pluginUpgrader == address(0x0) ? 3 : 4 + _pluginUpgrader == address(0x0) ? 2 : 3 ); // The DAO can emit content @@ -60,23 +67,23 @@ contract SpacePluginSetup is PluginSetup { permissionId: SUBSPACE_PERMISSION_ID }); - // The DAO needs to be able to upgrade the plugin - permissions[2] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Grant, - where: plugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: SpacePlugin(pluginImplementation).UPGRADE_PLUGIN_PERMISSION_ID() - }); - - // pluginUpgrader needs to be able to upgrade the plugin + // pluginUpgrader permissions if (_pluginUpgrader != address(0x0)) { - permissions[3] = PermissionLib.MultiTargetPermission({ + // pluginUpgrader can make the DAO execute applyUpdate + // pluginUpgrader can make the DAO execute grant/revoke + address[] memory _targetPluginAddresses = new address[](2); + _targetPluginAddresses[0] = plugin; + OnlyPluginUpgraderCondition _onlyPluginUpgraderCondition = new OnlyPluginUpgraderCondition( + DAO(payable(_dao)), + PluginSetupProcessor(pluginSetupProcessor), + _targetPluginAddresses + ); + permissions[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, - where: plugin, + where: _dao, who: _pluginUpgrader, - condition: PermissionLib.NO_CONDITION, - permissionId: SpacePlugin(pluginImplementation).UPGRADE_PLUGIN_PERMISSION_ID() + condition: address(_onlyPluginUpgraderCondition), + permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); } @@ -92,7 +99,7 @@ contract SpacePluginSetup is PluginSetup { address _pluginUpgrader = decodeUninstallationParams(_payload.data); permissionChanges = new PermissionLib.MultiTargetPermission[]( - _pluginUpgrader == address(0x0) ? 3 : 4 + _pluginUpgrader == address(0x0) ? 2 : 3 ); // The DAO can make it emit content @@ -111,20 +118,16 @@ contract SpacePluginSetup is PluginSetup { condition: PermissionLib.NO_CONDITION, permissionId: SUBSPACE_PERMISSION_ID }); - permissionChanges[2] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Revoke, - where: _payload.plugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: SpacePlugin(_payload.plugin).UPGRADE_PLUGIN_PERMISSION_ID() - }); + if (_pluginUpgrader != address(0x0)) { - permissionChanges[3] = PermissionLib.MultiTargetPermission({ + // pluginUpgrader can no longer make the DAO execute applyUpdate + // pluginUpgrader can no longer make the DAO execute grant/revoke + permissionChanges[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, - where: _payload.plugin, + where: _dao, who: _pluginUpgrader, - condition: PermissionLib.NO_CONDITION, - permissionId: SpacePlugin(_payload.plugin).UPGRADE_PLUGIN_PERMISSION_ID() + condition: address(0), + permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); } }