From a746bb3f6924d3cba79fa680a7cd76216add3f22 Mon Sep 17 00:00:00 2001 From: josemarinas <36479864+josemarinas@users.noreply.github.com> Date: Fri, 24 Nov 2023 16:12:54 +0100 Subject: [PATCH] Fix: Add missing check (#306) * add missing check * update package and changelog * fix comments * Update modules/client/CHANGELOG.md Co-authored-by: Michael Heuer <20623991+Michael-A-Heuer@users.noreply.github.com> --------- Co-authored-by: Michael Heuer <20623991+Michael-A-Heuer@users.noreply.github.com> --- modules/client/CHANGELOG.md | 5 ++ modules/client/package.json | 2 +- modules/client/src/internal/utils.ts | 29 ++++++++ modules/client/src/types.ts | 10 +++ modules/client/test/unit/client/utils.test.ts | 74 ++++++++++++++++++- 5 files changed, 117 insertions(+), 3 deletions(-) diff --git a/modules/client/CHANGELOG.md b/modules/client/CHANGELOG.md index 9db08abf2..ca4bfede4 100644 --- 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 index 75d707e51..c31aaf689 100644 --- 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 index 41a274b92..b4bed2fdd 100644 --- 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 index dcd87321b..5c1db5791 100644 --- 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 index f778d210b..1874561b7 100644 --- 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,