Updated plugin setup's and conditions

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.
 

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,15 +172,15 @@ 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
         permissionChanges[1] = PermissionLib.MultiTargetPermission({
             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()
             });
         }
     }

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;

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;

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()
             });
         }
     }