fix: remove upgrade plugin permission granted to Dao

packages/contracts/CHANGELOG.md

@@ -16,3 +16,5 @@ and this project adheres to the [Aragon OSx Plugin Versioning Convention](https:
 - Bumped OpenZepplin to `4.9.6`.
 - Used `ProxyLib` from `osx-commons-contracts` for the UUPS proxy deployment in `MultisigSetup`.
 - Hard-coded the `bytes32 internal constant EXECUTE_PERMISSION_ID` constant in `MultisigSetup` until it is available in `PermissionLib`.
+- Changed the `prepareInstallation` function in `MultisigSetup` to not unnecessarily grant the `UPGRADE_PLUGIN_PERMISSION_ID` permission to the installing DAO.
+- Changed the `prepareUpdate` function in `MultisigSetup` to revoke the previously unnecessarily granted `UPGRADE_PLUGIN_PERMISSION_ID` permission from the installing DAO.

packages/contracts/src/MultisigSetup.sol

@@ -46,7 +46,7 @@ contract MultisigSetup is PluginUpgradeableSetup {
 
         // Prepare permissions
         PermissionLib.MultiTargetPermission[]
-            memory permissions = new PermissionLib.MultiTargetPermission[](3);
+            memory permissions = new PermissionLib.MultiTargetPermission[](2);
 
         // Set permissions to be granted.
         // Grant the list of permissions of the plugin to the DAO.
@@ -58,16 +58,8 @@ contract MultisigSetup is PluginUpgradeableSetup {
             permissionId: Multisig(IMPLEMENTATION).UPDATE_MULTISIG_SETTINGS_PERMISSION_ID()
         });
 
-        permissions[1] = PermissionLib.MultiTargetPermission({
-            operation: PermissionLib.Operation.Grant,
-            where: plugin,
-            who: _dao,
-            condition: PermissionLib.NO_CONDITION,
-            permissionId: Multisig(IMPLEMENTATION).UPGRADE_PLUGIN_PERMISSION_ID()
-        });
-
         // Grant `EXECUTE_PERMISSION` of the DAO to the plugin.
-        permissions[2] = PermissionLib.MultiTargetPermission({
+        permissions[1] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Grant,
             where: _dao,
             who: plugin,
@@ -79,19 +71,32 @@ contract MultisigSetup is PluginUpgradeableSetup {
     }
 
     /// @inheritdoc IPluginSetup
-    /// @dev Nothing needs to happen for the update.
+    /// @dev Revoke the upgrade plugin permission to the DAO for all builds prior the current one (3).
     function prepareUpdate(
         address _dao,
-        uint16 _currentBuild,
+        uint16 _fromBuild,
         SetupPayload calldata _payload
     )
         external
-        pure
+        view
         override
         returns (bytes memory initData, PreparedSetupData memory preparedSetupData)
-    // solhint-disable-next-line no-empty-blocks
     {
-
+        (initData);
+        if (_fromBuild < 3) {
+            PermissionLib.MultiTargetPermission[]
+                memory permissions = new PermissionLib.MultiTargetPermission[](1);
+
+            permissions[0] = PermissionLib.MultiTargetPermission({
+                operation: PermissionLib.Operation.Revoke,
+                where: _payload.plugin,
+                who: _dao,
+                condition: PermissionLib.NO_CONDITION,
+                permissionId: Multisig(IMPLEMENTATION).UPGRADE_PLUGIN_PERMISSION_ID()
+            });
+
+            preparedSetupData.permissions = permissions;
+        }
     }
 
     /// @inheritdoc IPluginSetup
@@ -100,7 +105,7 @@ contract MultisigSetup is PluginUpgradeableSetup {
         SetupPayload calldata _payload
     ) external view returns (PermissionLib.MultiTargetPermission[] memory permissions) {
         // Prepare permissions
-        permissions = new PermissionLib.MultiTargetPermission[](3);
+        permissions = new PermissionLib.MultiTargetPermission[](2);
 
         // Set permissions to be Revoked.
         permissions[0] = PermissionLib.MultiTargetPermission({
@@ -112,14 +117,6 @@ contract MultisigSetup is PluginUpgradeableSetup {
         });
 
         permissions[1] = PermissionLib.MultiTargetPermission({
-            operation: PermissionLib.Operation.Revoke,
-            where: _payload.plugin,
-            who: _dao,
-            condition: PermissionLib.NO_CONDITION,
-            permissionId: Multisig(IMPLEMENTATION).UPGRADE_PLUGIN_PERMISSION_ID()
-        });
-
-        permissions[2] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Revoke,
             where: _dao,
             who: _payload.plugin,

packages/contracts/src/build-metadata.json

@@ -1,6 +1,6 @@
 {
   "ui": {},
-  "change": "v1.3\n - TODO TODO.",
+  "change": "v1.3\n - Removed an unneccessary permission that allowed the Dao to upgrade the plugin, because this is supposed to happens as part of the update itself. The unnecessary permission, which was granted on installation of previous versions, will be automatically removed with the update to this version.\n",
   "pluginSetup": {
     "prepareInstallation": {
       "description": "The information required for the installation.",

packages/contracts/test/10_unit-testing/12_plugin-setup.ts

@@ -1,5 +1,5 @@
 import {createDaoProxy} from '../20_integration-testing/test-helpers';
-import {METADATA, VERSION} from '../../plugin-settings';
+import {METADATA} from '../../plugin-settings';
 import {MultisigSetup, MultisigSetup__factory} from '../../typechain';
 import {
   MULTISIG_INTERFACE,
@@ -274,7 +274,7 @@ describe('MultisigSetup', function () {
       // Check the return data.
       expect(plugin).to.be.equal(anticipatedPluginAddress);
       expect(helpers.length).to.be.equal(0);
-      expect(permissions.length).to.be.equal(3);
+      expect(permissions.length).to.be.equal(2);
       expect(permissions).to.deep.equal([
         [
           Operation.Grant,
@@ -283,13 +283,6 @@ describe('MultisigSetup', function () {
           AddressZero,
           UPDATE_MULTISIG_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Grant,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Grant,
           dao.address,
@@ -340,26 +333,70 @@ describe('MultisigSetup', function () {
   });
 
   describe('prepareUpdate', async () => {
-    it('should return nothing', async () => {
+    it('returns the permissions expected for the update from build 1', async () => {
       const {pluginSetup, dao} = await loadFixture(fixture);
+      const plugin = ethers.Wallet.createRandom().address;
 
       // Make a static call to check that the plugin update data being returned is correct.
-      const prepareUpdateData = await pluginSetup.callStatic.prepareUpdate(
-        dao.address,
-        VERSION.build,
-        {
-          currentHelpers: [
-            ethers.Wallet.createRandom().address,
-            ethers.Wallet.createRandom().address,
-          ],
-          data: [],
-          plugin: ethers.Wallet.createRandom().address,
-        }
-      );
+      const {
+        initData: initData,
+        preparedSetupData: {helpers, permissions},
+      } = await pluginSetup.callStatic.prepareUpdate(dao.address, 1, {
+        currentHelpers: [
+          ethers.Wallet.createRandom().address,
+          ethers.Wallet.createRandom().address,
+        ],
+        data: [],
+        plugin,
+      });
+
+      // Check the return data.
+      expect(initData).to.be.eq('0x');
+      expect(permissions.length).to.be.eql(1);
+      expect(helpers).to.be.eql([]);
+      // check correct permission is revoked
+      expect(permissions).to.deep.equal([
+        [
+          Operation.Revoke,
+          plugin,
+          dao.address,
+          AddressZero,
+          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
+        ],
+      ]);
+    });
+
+    it('returns the permissions expected for the update from build 2', async () => {
+      const {pluginSetup, dao} = await loadFixture(fixture);
+      const plugin = ethers.Wallet.createRandom().address;
+
+      // Make a static call to check that the plugin update data being returned is correct.
+      const {
+        initData: initData,
+        preparedSetupData: {helpers, permissions},
+      } = await pluginSetup.callStatic.prepareUpdate(dao.address, 2, {
+        currentHelpers: [
+          ethers.Wallet.createRandom().address,
+          ethers.Wallet.createRandom().address,
+        ],
+        data: [],
+        plugin,
+      });
+
       // Check the return data.
-      expect(prepareUpdateData.initData).to.be.eq('0x');
-      expect(prepareUpdateData.preparedSetupData.permissions).to.be.eql([]);
-      expect(prepareUpdateData.preparedSetupData.helpers).to.be.eql([]);
+      expect(initData).to.be.eq('0x');
+      expect(permissions.length).to.be.eql(1);
+      expect(helpers).to.be.eql([]);
+      // check correct permission is revoked
+      expect(permissions).to.deep.equal([
+        [
+          Operation.Revoke,
+          plugin,
+          dao.address,
+          AddressZero,
+          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
+        ],
+      ]);
     });
   });
 
@@ -384,7 +421,7 @@ describe('MultisigSetup', function () {
       );
 
       // Check the return data.
-      expect(permissions.length).to.be.equal(3);
+      expect(permissions.length).to.be.equal(2);
       expect(permissions).to.deep.equal([
         [
           Operation.Revoke,
@@ -393,13 +430,6 @@ describe('MultisigSetup', function () {
           AddressZero,
           UPDATE_MULTISIG_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Revoke,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Revoke,
           dao.address,

packages/contracts/test/20_integration-testing/test-helpers.ts

@@ -264,8 +264,8 @@ export async function updateFromBuildTest(
     .connect(deployer)
     .grant(dao.address, psp.address, DAO_PERMISSIONS.ROOT_PERMISSION_ID);
 
-  // Install build 1.
-  const pluginSetupRefBuild1 = {
+  // Install a previous build with build number `build`
+  const pluginSetupRefPreviousBuild = {
     versionTag: {
       release: VERSION.release,
       build: build,
@@ -276,7 +276,7 @@ export async function updateFromBuildTest(
     deployer,
     psp,
     dao,
-    pluginSetupRefBuild1,
+    pluginSetupRefPreviousBuild,
     ethers.utils.defaultAbiCoder.encode(
       getNamedTypesFromMetadata(
         METADATA.build.pluginSetup.prepareInstallation.inputs
@@ -292,15 +292,16 @@ export async function updateFromBuildTest(
   );
 
   // Check that the implementation of the plugin proxy matches the latest build
-  const implementationBuild1 = await PluginUpgradeableSetup__factory.connect(
-    (
-      await pluginRepo['getVersion((uint8,uint16))'](
-        pluginSetupRefBuild1.versionTag
-      )
-    ).pluginSetup,
-    deployer
-  ).implementation();
-  expect(await plugin.implementation()).to.equal(implementationBuild1);
+  const implementationPreviousBuild =
+    await PluginUpgradeableSetup__factory.connect(
+      (
+        await pluginRepo['getVersion((uint8,uint16))'](
+          pluginSetupRefPreviousBuild.versionTag
+        )
+      ).pluginSetup,
+      deployer
+    ).implementation();
+  expect(await plugin.implementation()).to.equal(implementationPreviousBuild);
 
   // Grant the PSP the permission to upgrade the plugin implementation.
   await dao
@@ -311,15 +312,15 @@ export async function updateFromBuildTest(
       PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID
     );
 
-  // Update build 1 to the latest build
+  // Update from the previous build to the latest build
   await expect(
     updatePlugin(
       deployer,
       psp,
       dao,
       plugin,
       installationResults.preparedEvent.args.preparedSetupData.helpers,
-      pluginSetupRefBuild1,
+      pluginSetupRefPreviousBuild,
       pluginSetupRefLatestBuild,
       ethers.utils.defaultAbiCoder.encode(
         getNamedTypesFromMetadata(