Fix: Add missing check (#306)

* add missing check * update package and changelog * fix comments * Update modules/client/CHANGELOG.md Co-authored-by: Michael Heuer <[email protected]> --------- Co-authored-by: Michael Heuer <[email protected]>
aragon · Nov 24, 2023 · a746bb3 · a746bb3
1 parent d56a1f3
commit a746bb3
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 3 deletions.
diff --git a/modules/client/CHANGELOG.md b/modules/client/CHANGELOG.md
@@ -19,6 +19,11 @@ TEMPLATE:
 
 ## [UPCOMING]
 
+### Fixed
+- Added missing security check that checks that the `to` address in the permission actions is the DAO address
+
+## [1.19.2]
+### Fixed
 - Fixed proposal id not being transformed
 
 ## [1.19.1]

diff --git a/modules/client/package.json b/modules/client/package.json
@@ -1,7 +1,7 @@
 {
   "name": "@aragon/sdk-client",
   "author": "Aragon Association",
-  "version": "1.19.2",
+  "version": "1.19.3",
   "license": "MIT",
   "main": "dist/index.js",
   "module": "dist/sdk-client.esm.js",

diff --git a/modules/client/src/internal/utils.ts b/modules/client/src/internal/utils.ts
@@ -927,6 +927,14 @@ export async function validateGrantUpgradePluginPermissionAction(
         .NON_ZERO_GRANT_UPGRADE_PLUGIN_PERMISSION_CALL_VALUE,
     );
   }
+  // The action should be sent to the DAO
+  if (action.to !== daoAddress) {
+    causes.push(
+      PluginUpdateProposalInValidityCause
+        .INVALID_GRANT_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS,
+    );
+  }
+
   // The permission should be granted to the PSP
   if (decodedPermission.who !== pspAddress) {
     causes.push(
@@ -975,6 +983,13 @@ export async function validateRevokeUpgradePluginPermissionAction(
         .PLUGIN_NOT_INSTALLED,
     );
   }
+  // The action should be sent to the DAO
+  if (action.to !== daoAddress) {
+    causes.push(
+      PluginUpdateProposalInValidityCause
+        .INVALID_REVOKE_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS,
+    );
+  }
   if (action.value.toString() !== "0") {
     causes.push(
       PluginUpdateProposalInValidityCause
@@ -1019,6 +1034,13 @@ export function validateGrantRootPermissionAction(
         .NON_ZERO_GRANT_ROOT_PERMISSION_CALL_VALUE,
     );
   }
+  // The action should be sent to the DAO
+  if (action.to !== daoAddress) {
+    causes.push(
+      PluginUpdateProposalInValidityCause
+        .INVALID_GRANT_ROOT_PERMISSION_TO_ADDRESS,
+    );
+  }
   if (decodedPermission.where !== daoAddress) {
     causes.push(
       PluginUpdateProposalInValidityCause
@@ -1063,6 +1085,13 @@ export function validateRevokeRootPermissionAction(
         .NON_ZERO_REVOKE_ROOT_PERMISSION_CALL_VALUE,
     );
   }
+  // The action should be sent to the DAO
+  if (action.to !== daoAddress) {
+    causes.push(
+      PluginUpdateProposalInValidityCause
+        .INVALID_REVOKE_ROOT_PERMISSION_TO_ADDRESS,
+    );
+  }
   if (decodedPermission.where !== daoAddress) {
     causes.push(
       PluginUpdateProposalInValidityCause

diff --git a/modules/client/src/types.ts b/modules/client/src/types.ts
@@ -440,6 +440,9 @@ export enum PluginUpdateProposalInValidityCause {
     "nonZeroGrantUpgradePluginPermissionCallValue",
   INVALID_GRANT_UPGRADE_PLUGIN_PERMISSION_PERMISSION_ID =
     "invalidGrantUpgradePluginPermissionPermissionId",
+  INVALID_GRANT_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS =
+    "invalidGrantUpgradePluginPermissionToAddress",
+
   // Revoke UPDATE_PLUGIN_PERMISSION action
   INVALID_REVOKE_UPGRADE_PLUGIN_PERMISSION_WHO_ADDRESS =
     "invalidRevokeUpgradePluginPermissionWhoAddress",
@@ -451,6 +454,9 @@ export enum PluginUpdateProposalInValidityCause {
     "nonZeroRevokeUpgradePluginPermissionCallValue",
   INVALID_REVOKE_UPGRADE_PLUGIN_PERMISSION_PERMISSION_ID =
     "invalidRevokeUpgradePluginPermissionPermissionId",
+  INVALID_REVOKE_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS = 
+    "invalidRevokeUpdatePluginPermissionToAddress",
+
   // Grant ROOT_PERMISSION action
   INVALID_GRANT_ROOT_PERMISSION_WHO_ADDRESS =
     "invalidGrantRootPermissionWhoAddress",
@@ -462,6 +468,8 @@ export enum PluginUpdateProposalInValidityCause {
     "nonZeroGrantRootPermissionCallValue",
   INVALID_GRANT_ROOT_PERMISSION_PERMISSION_ID =
     "invalidGrantRootPermissionPermissionId",
+  INVALID_GRANT_ROOT_PERMISSION_TO_ADDRESS = 
+    "invalidGrantRootPermissionToAddress",
   // Revoke ROOT_PERMISSION action
   INVALID_REVOKE_ROOT_PERMISSION_WHO_ADDRESS =
     "invalidRevokeRootPermissionWhoAddress",
@@ -473,6 +481,8 @@ export enum PluginUpdateProposalInValidityCause {
     "nonZeroRevokeRootPermissionCallValue",
   INVALID_REVOKE_ROOT_PERMISSION_PERMISSION_ID =
     "invalidRevokeRootPermissionPermissionId",
+  INVALID_REVOKE_ROOT_PERMISSION_TO_ADDRESS = 
+    "invalidRevokeRootPermissionToAddress",
   // applyUpdate action
   NON_ZERO_APPLY_UPDATE_CALL_VALUE = "nonZeroApplyUpdateCallValue",
   PLUGIN_NOT_INSTALLED = "pluginNotInstalled",

diff --git a/modules/client/test/unit/client/utils.test.ts b/modules/client/test/unit/client/utils.test.ts
@@ -117,6 +117,24 @@ describe("Test client utils", () => {
           .NON_ZERO_GRANT_UPGRADE_PLUGIN_PERMISSION_CALL_VALUE,
       ]);
     });
+    it("should return an error if `to` is not the DAO address", async () => {
+      const grantAction = client.encoding.grantAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.UPGRADE_PLUGIN_PERMISSION,
+      });
+      grantAction.to = ADDRESS_ONE;
+      const result = await validateGrantUpgradePluginPermissionAction(
+        grantAction,
+        pspAddress,
+        daoAddress,
+        client.graphql,
+      );
+      expect(result).toEqual([
+        PluginUpdateProposalInValidityCause
+          .INVALID_GRANT_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS,
+      ]);
+    });
     it("should return an error if the plugin does not exist", async () => {
       const grantAction = client.encoding.grantAction(daoAddress, {
         where: daoAddress,
@@ -251,6 +269,24 @@ describe("Test client utils", () => {
           .NON_ZERO_REVOKE_UPGRADE_PLUGIN_PERMISSION_CALL_VALUE,
       ]);
     });
+    it("should return an error if `to` is not the DAO address", async () => {
+      const revokeAction = client.encoding.revokeAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.UPGRADE_PLUGIN_PERMISSION,
+      });
+      revokeAction.to = ADDRESS_ONE;
+      const result = await validateRevokeUpgradePluginPermissionAction(
+        revokeAction,
+        pspAddress,
+        daoAddress,
+        client.graphql,
+      );
+      expect(result).toEqual([
+        PluginUpdateProposalInValidityCause
+          .INVALID_REVOKE_UPGRADE_PLUGIN_PERMISSION_TO_ADDRESS,
+      ]);
+    });
     it("should return an error if the installation does not exist", async () => {
       const revokeAction = client.encoding.revokeAction(daoAddress, {
         where: daoAddress,
@@ -377,6 +413,23 @@ describe("Test client utils", () => {
           .NON_ZERO_GRANT_ROOT_PERMISSION_CALL_VALUE,
       ]);
     });
+    it("should return an error if `to` is not the DAO address", () => {
+      const grantAction = client.encoding.grantAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.ROOT_PERMISSION,
+      });
+      grantAction.to = ADDRESS_ONE;
+      const result = validateGrantRootPermissionAction(
+        grantAction,
+        daoAddress,
+        pspAddress,
+      );
+      expect(result).toEqual([
+        PluginUpdateProposalInValidityCause
+          .INVALID_GRANT_ROOT_PERMISSION_TO_ADDRESS,
+      ]);
+    });
     it("should return an error if the permission is not granted in the DAO", () => {
       const grantAction = client.encoding.grantAction(daoAddress, {
         where: pluginAddress,
@@ -452,13 +505,13 @@ describe("Test client utils", () => {
     });
     it("should return an empty array for a valid action", () => {
       const revokeAction = client.encoding.revokeAction(daoAddress, {
-        where: pluginAddress,
+        where: daoAddress,
         who: pspAddress,
         permission: Permissions.ROOT_PERMISSION,
       });
       const result = validateRevokeRootPermissionAction(
         revokeAction,
-        pluginAddress,
+        daoAddress,
         pspAddress,
       );
       expect(result).toEqual([]);
@@ -493,6 +546,23 @@ describe("Test client utils", () => {
           .NON_ZERO_REVOKE_ROOT_PERMISSION_CALL_VALUE,
       ]);
     });
+    it("should return an error if `to` is not the DAO address", () => {
+      const revokeAction = client.encoding.revokeAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.ROOT_PERMISSION,
+      });
+      revokeAction.to = ADDRESS_ONE;
+      const result = validateRevokeRootPermissionAction(
+        revokeAction,
+        daoAddress,
+        pspAddress,
+      );
+      expect(result).toEqual([
+        PluginUpdateProposalInValidityCause
+          .INVALID_REVOKE_ROOT_PERMISSION_TO_ADDRESS,
+      ]);
+    });
     it("should return an error if the permission is not revoked in the DAO", () => {
       const revokeAction = client.encoding.revokeAction(daoAddress, {
         where: pluginAddress,