fix: stop granting upgrade plugin permission to Dao (#23)

* feat: stop grating the upgrade plugin permission on the Dao during the installation process, and revoke that permission to the previous builds during the plugin update * feat: update the unit tests * fix: remove not used code * ci: rename vars and comments on test helpers * feat: update metadata * feat: update changelog * ci: enhance the changelog description * ci: improve the metadata changes description * Update packages/contracts/CHANGELOG.md Co-authored-by: Michael Heuer <[email protected]> * Update packages/contracts/src/build-metadata.json Co-authored-by: Michael Heuer <[email protected]> * Update packages/contracts/test/10_unit-testing/12_plugin-setup.ts Co-authored-by: Michael Heuer <[email protected]> * ci: pr comments * ci: pr review comment * ci: pr review comment * ci: undo changes since it is already fixed in another pr * Update packages/contracts/test/20_integration-testing/test-helpers.ts Co-authored-by: Michael Heuer <[email protected]> * Update packages/contracts/test/20_integration-testing/test-helpers.ts Co-authored-by: Michael Heuer <[email protected]> --------- Co-authored-by: Michael Heuer <[email protected]>
aragon · May 10, 2024 · 47e427f · 47e427f
1 parent 64bc612
commit 47e427f
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 92 deletions.
diff --git a/packages/contracts/CHANGELOG.md b/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 `TokenVotingSetup`.
 - Hard-coded the `bytes32 internal constant EXECUTE_PERMISSION_ID` constant in `TokenVotingSetup` until it is available in `PermissionLib`.
+- Changed the `prepareInstallation` function in `TokenVotingSetup` to not unnecessarily grant the `UPGRADE_PLUGIN_PERMISSION_ID` permission to the installing DAO.
+- Changed the `prepareUpdate` function in `TokenVotingSetup` to revoke the previously unnecessarily granted `UPGRADE_PLUGIN_PERMISSION_ID` permission from the installing DAO.
diff --git a/packages/contracts/src/TokenVotingSetup.sol b/packages/contracts/src/TokenVotingSetup.sol
@@ -103,11 +103,12 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
             );
 
         address token = tokenSettings.addr;
+        bool tokenAddressNotZero = token != address(0);
 
         // Prepare helpers.
         address[] memory helpers = new address[](1);
 
-        if (token != address(0)) {
+        if (tokenAddressNotZero) {
             if (!token.isContract()) {
                 revert TokenNotContract(token);
             }
@@ -162,7 +163,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
         // Prepare permissions
         PermissionLib.MultiTargetPermission[]
             memory permissions = new PermissionLib.MultiTargetPermission[](
-                tokenSettings.addr != address(0) ? 3 : 4
+                tokenAddressNotZero ? 2 : 3
             );
 
         // Set plugin permissions to be granted.
@@ -175,27 +176,19 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
             permissionId: tokenVotingBase.UPDATE_VOTING_SETTINGS_PERMISSION_ID()
         });
 
-        permissions[1] = PermissionLib.MultiTargetPermission({
-            operation: PermissionLib.Operation.Grant,
-            where: plugin,
-            who: _dao,
-            condition: PermissionLib.NO_CONDITION,
-            permissionId: tokenVotingBase.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,
             condition: PermissionLib.NO_CONDITION,
             permissionId: EXECUTE_PERMISSION_ID
         });
 
-        if (tokenSettings.addr == address(0)) {
+        if (!tokenAddressNotZero) {
             bytes32 tokenMintPermission = GovernanceERC20(token).MINT_PERMISSION_ID();
 
-            permissions[3] = PermissionLib.MultiTargetPermission({
+            permissions[2] = PermissionLib.MultiTargetPermission({
                 operation: PermissionLib.Operation.Grant,
                 where: token,
                 who: _dao,
@@ -209,19 +202,32 @@ contract TokenVotingSetup 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: tokenVotingBase.UPGRADE_PLUGIN_PERMISSION_ID()
+            });
 
+            preparedSetupData.permissions = permissions;
+        }
     }
 
     /// @inheritdoc IPluginSetup
@@ -243,7 +249,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
 
         bool isGovernanceERC20 = supportedIds[0] && supportedIds[1] && !supportedIds[2];
 
-        permissions = new PermissionLib.MultiTargetPermission[](isGovernanceERC20 ? 4 : 3);
+        permissions = new PermissionLib.MultiTargetPermission[](isGovernanceERC20 ? 3 : 2);
 
         // Set permissions to be Revoked.
         permissions[0] = PermissionLib.MultiTargetPermission({
@@ -255,14 +261,6 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
         });
 
         permissions[1] = PermissionLib.MultiTargetPermission({
-            operation: PermissionLib.Operation.Revoke,
-            where: _payload.plugin,
-            who: _dao,
-            condition: PermissionLib.NO_CONDITION,
-            permissionId: tokenVotingBase.UPGRADE_PLUGIN_PERMISSION_ID()
-        });
-
-        permissions[2] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Revoke,
             where: _dao,
             who: _payload.plugin,
@@ -274,7 +272,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
         // as GovernanceWrapped does not possess this permission. Only return the following
         // if it's type of GovernanceERC20, otherwise revoking this permission wouldn't have any effect.
         if (isGovernanceERC20) {
-            permissions[3] = PermissionLib.MultiTargetPermission({
+            permissions[2] = PermissionLib.MultiTargetPermission({
                 operation: PermissionLib.Operation.Revoke,
                 where: token,
                 who: _dao,

diff --git a/packages/contracts/src/build-metadata.json b/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.",

diff --git a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts b/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 {
   ERC20,
   ERC20__factory,
@@ -310,7 +310,7 @@ describe('TokenVotingSetup', function () {
       expect(plugin).to.be.equal(anticipatedPluginAddress);
       expect(helpers.length).to.be.equal(1);
       expect(helpers).to.be.deep.equal([anticipatedWrappedTokenAddress]);
-      expect(permissions.length).to.be.equal(3);
+      expect(permissions.length).to.be.equal(2);
       expect(permissions).to.deep.equal([
         [
           Operation.Grant,
@@ -319,13 +319,6 @@ describe('TokenVotingSetup', function () {
           AddressZero,
           UPDATE_VOTING_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Grant,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Grant,
           dao.address,
@@ -423,7 +416,7 @@ describe('TokenVotingSetup', function () {
       expect(plugin).to.be.equal(anticipatedPluginAddress);
       expect(helpers.length).to.be.equal(1);
       expect(helpers).to.be.deep.equal([governanceERC20.address]);
-      expect(permissions.length).to.be.equal(3);
+      expect(permissions.length).to.be.equal(2);
       expect(permissions).to.deep.equal([
         [
           Operation.Grant,
@@ -432,13 +425,6 @@ describe('TokenVotingSetup', function () {
           AddressZero,
           UPDATE_VOTING_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Grant,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Grant,
           dao.address,
@@ -490,7 +476,7 @@ describe('TokenVotingSetup', function () {
       expect(plugin).to.be.equal(anticipatedPluginAddress);
       expect(helpers.length).to.be.equal(1);
       expect(helpers).to.be.deep.equal([anticipatedTokenAddress]);
-      expect(permissions.length).to.be.equal(4);
+      expect(permissions.length).to.be.equal(3);
       expect(permissions).to.deep.equal([
         [
           Operation.Grant,
@@ -499,13 +485,6 @@ describe('TokenVotingSetup', function () {
           AddressZero,
           UPDATE_VOTING_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Grant,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Grant,
           dao.address,
@@ -595,26 +574,68 @@ describe('TokenVotingSetup', 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(helpers).to.be.eql([]);
+      expect(permissions.length).to.be.eql(1);
+      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(helpers).to.be.eql([]);
+      expect(permissions.length).to.be.eql(1);
+      expect(permissions).to.deep.equal([
+        [
+          Operation.Revoke,
+          plugin,
+          dao.address,
+          AddressZero,
+          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
+        ],
+      ]);
     });
   });
 
@@ -695,13 +716,6 @@ describe('TokenVotingSetup', function () {
           AddressZero,
           UPDATE_VOTING_SETTINGS_PERMISSION_ID,
         ],
-        [
-          Operation.Revoke,
-          plugin,
-          dao.address,
-          AddressZero,
-          PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID,
-        ],
         [
           Operation.Revoke,
           dao.address,
@@ -711,7 +725,7 @@ describe('TokenVotingSetup', function () {
         ],
       ];
 
-      expect(permissions1.length).to.be.equal(3);
+      expect(permissions1.length).to.be.equal(2);
       expect(permissions1).to.deep.equal(essentialPermissions);
 
       const permissions2 = await pluginSetup.callStatic.prepareUninstallation(
@@ -723,7 +737,7 @@ describe('TokenVotingSetup', function () {
         }
       );
 
-      expect(permissions2.length).to.be.equal(4);
+      expect(permissions2.length).to.be.equal(3);
       expect(permissions2).to.deep.equal([
         ...essentialPermissions,
         [

diff --git a/packages/contracts/test/20_integration-testing/test-helpers.ts b/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(