refactor: Plugin Update Security Check (#301)

* add fix for plugin upgrade execution

* fix tests

* fix tests

* remove unused query

* Update modules/client/src/internal/client/encoding.ts

Co-authored-by: Michael Heuer <[email protected]>

* Update modules/client/CHANGELOG.md

Co-authored-by: Michael Heuer <[email protected]>

* update changelogg

* wip

* refactor and update tests

* add tests

* add constants

* refactor checks

* fix tests

* fix code smells and comment validation function

* update changelogs

* refactor update plugin output

* refactor compare arrays

* refactor conditions in validateApplyUpdateFunction

* refactor in classifyProposalActions

* refactor isPluginUpdateActionWithRootPermission and isPluginUpdateAction

* refactor validateUpdateDaoProposalActions function

* refactor conditions in validateUpdatePluginProposalActions

* error causes renaming

* add minor function and variable renaming

* add tests to utils

* rename functions

* rename isDaoUpdateValid and isDaoUpdate functions

* rename isPluginUpdateValid and isPluginUpdate functions

* rename isPluginUpdateAction function

* fix build

* remove unused types

* fix unit tests

* fix integration tests

* fix comments

* fix tests

* docs: added security check documentation

* update error messages and docs

* fix typo

* docs: fix typos

Co-authored-by: Mathias Scherer <[email protected]>

* fix naming

---------

Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Mathias Scherer <[email protected]>

docs/sdk/03-update-security-check/index.md

@@ -0,0 +1,121 @@
+---
+title: Update Security Check
+---
+
+## General Update Proposal Checks
+
+The security check is composed of two functions: `isDaoUpdateProposalValid` and `isPluginUpdateProposalValid` that both receive a proposal ID as an input.
+If the proposal cannot be found via the ID or the subgraph is down, we return `"proposalNotFound"` in both cases.
+
+A proposal contains an `Action[]` array (see [our docs](https://devs.aragon.org/docs/osx/how-it-works/core/dao/actions#actions)).
+
+The update proposal MUST only contain actions related to the update and no other actions.
+If an unexpected action is present, we return `"invalidActions"`.
+
+DAO proposals contain an `_allowFailureMap` value indicating actions in the action array that are allowed to fail (see [our docs](https://devs.aragon.org/docs/osx/how-it-works/core/dao/actions#the-allowfailuremap-input-argument)).
+For updates, `_allowFailureMap` MUST be zero (no failure is allowed).
+If it is non-zero, we return `"nonZeroAllowFailureMapValue"`.
+
+## Specific Action Checks
+
+Every `Action` in the actions array has three parameters:
+
+```solidity title="@aragon/osx/core/dao/IDAO.sol"
+/// @notice The action struct to be consumed by the DAO's `execute` function resulting in an external call.
+/// @param to The address to call.
+/// @param value The native token value to be sent with the call.
+/// @param data The bytes-encoded function selector and calldata for the call.
+struct Action {
+  address to;
+  uint256 value;
+  bytes data;
+}
+```
+
+In the following, we distinguish between actions related to the DAO.
+
+### DAO Update
+
+For DAO updates, we expect single action at position 0 of the action array being characterized as follows:
+
+- `to` MUST be the DAO address. If not, we return `"invalidToAddress"`.
+
+- `value` MUST be zero. If not, we return `"nonZeroCallValue"`.
+
+- `data` MUST contain the `upgradeTo(address newImplementation)` OR `upgradeToAndCall(address newImplementation, bytes memory data)` function selector depending on the nature of the update
+  - The `upgradeToAndCall` call
+    - MUST go to the right implementation address. This can be either the latest version or any other, newer version. If not, we return `"invalidUpgradeToImplementationAddress"`.
+    - MUST go to the `initializeFrom` function
+    - the additional data passed to `initializeFrom` MUST be empty. If not, we return `"invalidUpgradeToAndCallData"`.
+      - the semantic version number of the previous DAO must be as expected. If not, we return `"invalidUpgradeToAndCallVersion"`.
+  - `upgradeTo` can be called instead, if no reinitalization of the DAO is required. The call
+    - MUST go to the right implementation address. This can be either the latest version or any other, newer version. If not, we return `"invalidUpgradeToAndCallImplementationAddress"`.
+    - MUST have empty subsequent calldata. If not, we return `"invalidUpgradeToAndCallData"`.
+
+### Plugin Updates
+
+For each plugin update, we expect a block of associated actions. There can be multiple, independent plugin updates happening in one update proposal.
+We expect two types of blocks:
+
+```
+[
+  grant({_where: plugin, _who: pluginSetupProcessor, _permissionId: UPGRADE_PLUGIN_PERMISSION_ID}),
+  applyUpdate({_dao: dao, _params: applyUpdateParams}),
+  revoke({_where: plugin, _who: pluginSetupProcessor, _permissionId: UPGRADE_PLUGIN_PERMISSION_ID})
+]
+```
+
+or
+
+```
+[
+  grant({_where: plugin, _who: pluginSetupProcessor, _permissionId: UPGRADE_PLUGIN_PERMISSION_ID}),
+  grant({_where: dao, _who: pluginSetupProcessor, _permissionId: ROOT_PERMISSION_ID}),
+  applyUpdate({_dao: dao, _params: applyUpdateParams}),
+  revoke({_where: dao, _who: pluginSetupProcessor, _permissionId: ROOT_PERMISSION_ID}),
+  revoke({_where: plugin, _who: pluginSetupProcessor, _permissionId: UPGRADE_PLUGIN_PERMISSION_ID})
+]
+```
+
+#### Mandatory `applyUpdate` Call
+
+Each block being related to a plugin update MUST contain an `applyUpdate` action exactly once. This action is composed as follows:
+
+- `to` MUST be the `PluginSetupProcessor` address
+- `value` MUST be zero. If not, we return `"nonZeroApplyUpdateCallValue"`.
+- `data` MUST contain the `applyUpdate` function selector in the first 4 bytes. The following bytes MUST be encoded according to the [the `build-metadata.json` specifications](https://devs.aragon.org/docs/osx/how-to-guides/plugin-development/publication/metadata).
+  If we cannot decode the action, we return `"invalidData"`.
+  If we cannot obtain the metadata, we return `"invalidPluginRepoMetadata"`.
+  Furthermore, the data MUST
+  - update a plugin that is currently installed to the DAO. If not, we return `"pluginNotInstalled"`.
+  - update a plugin from an Aragon plugin repo. If it is not an Aragon repo, we return `"notAragonPluginRepo"`. If it does not exist, we return `"missingPluginRepo"`.
+  - reference an update preparation (resulting from an `prepareUpdate` call). If the update is not prepared, we return `"missingPluginPreparation"`.
+  - update to a new build and not
+    - to the same or an older build. If not, we return `"updateToOlderOrSameBuild"`.
+    - to a different release. If not, we return `"updateToIncompatibleRelease"`.
+
+#### Mandatory `grant`/`revoke` `UPGRADE_PLUGIN_PERMISSION` Calls
+
+The `applyUpdate` action MUST be wrapped by `grant` and `revoke` actions on the same DAO where
+
+- `to` MUST be the DAO address
+- `value` MUST be zero. If not, we return `"nonZeroGrantUpgradePluginPermissionCallValue"` / `"nonZeroRevokeUpgradePluginPermissionCallValue"`.
+- `data` MUST contain the `grant` / `revoke` function selector in the first 4 bytes. The subsequent bytes MUST be as follows:
+  - `where` MUST be the plugin proxy address. If not, we return `"invalidGrantUpgradePluginPermissionWhereAddress"` / `"invalidRevokeUpgradePluginPermissionWhereAddress"`.
+  - `who` MUST be the `PluginSetupProcessor` address. If not, we return `"invalidGrantUpgradePluginPermissionWhoAddress"` / `"invalidRevokeUpgradePluginPermissionWhoAddress"`.
+  - `permissionId` MUST be `bytes32 UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION")`. If not, we return `"invalidGrantUpgradePluginPermissionPermissionId"` / `"invalidRevokeUpgradePluginPermissionPermissionId"`.
+  - `permissionName` MUST be `UPGRADE_PLUGIN_PERMISSION`. If not, we return `"invalidGrantUpgradePluginPermissionPermissionName"` / `"invalidRevokeUpgradePluginPermissionPermissionName"`
+
+#### Optional `grant`/`revoke` `ROOT_PERMISSION` Calls
+
+The `applyUpdate` action CAN be wrapped by `grant` and `revoke` actions on the same DAO where
+
+- `to` MUST be the DAO address
+- `value` MUST be zero. If not, we return `"nonZeroGrantRootPermissionCallValue"` / `"nonZeroRevokeRootPermissionCallValue"`.
+- `data` MUST contain the `grant` / `revoke` function selector in the first 4 bytes. The subsequent bytes MUST be as follows:
+  - `where` MUST be the DAO proxy address. If not, we return `"invalidGrantRootPermissionWhereAddress"` / `"invalidRevokeRootPermissionWhereAddress"`.
+  - `who` MUST be the `PluginSetupProcessor` address. If not, we return `"invalidGrantRootPermissionWhoAddress"` / `"invalidRevokeRootPermissionWhoAddress"`.
+  - `permissionId` MUST be `bytes32 ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION")`. If not, we return `"invalidGrantRootPermissionPermissionId"` / `"invalidRevokeRootPermissionPermissionId"`.
+  - `permissionName` MUST be `ROOT_PERMISSION`. If not, we return `"invalidGrantRootPermissionPermissionName"` / `"invalidRevokeRootPermissionPermissionName"`
+
+

modules/client-common/CHANGELOG.md

@@ -21,6 +21,13 @@ TEMPLATE:
 
 ### Added
 
+- Added `UPGRADE_PLUGIN_PERMISSION` to `PermissionType` enum
+- Fix chain id for sepolia network
+
+## [1.11.0]
+
+### Added
+
 - Added `actions` to `SubgraphListItem` type
 
 ## [1.10.0]

modules/client-common/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@aragon/sdk-client-common",
   "author": "Aragon Association",
-  "version": "1.11.0",
+  "version": "1.11.1",
   "license": "MIT",
   "main": "dist/index.js",
   "module": "dist/sdk-client-common.esm.js",

modules/client-common/src/constants.ts

@@ -456,7 +456,7 @@ export const ADDITIONAL_NETWORKS: Network[] = [
   },
   {
     name: "sepolia",
-    chainId: 58008,
+    chainId: 11155111,
   },
   {
     name: "local",
@@ -466,6 +466,7 @@ export const ADDITIONAL_NETWORKS: Network[] = [
 
 const Permissions = {
   UPGRADE_PERMISSION: "UPGRADE_PERMISSION",
+  UPGRADE_PLUGIN_PERMISSION: "UPGRADE_PLUGIN_PERMISSION",
   SET_METADATA_PERMISSION: "SET_METADATA_PERMISSION",
   EXECUTE_PERMISSION: "EXECUTE_PERMISSION",
   WITHDRAW_PERMISSION: "WITHDRAW_PERMISSION",

modules/client/CHANGELOG.md

@@ -19,6 +19,26 @@ TEMPLATE:
 
 ## [UPCOMING]
 
+### Changed
+
+- Add grant and revoke for permission `UPGRADE_PLUGIN_PERMISSION` in `applyUpdate` encoder
+- Refactor DAO update proposal validation function
+- Refactor plugin update proposal validation function
+- Refactor boolean check for checking if a proposal is a DAO update proposal
+- Refactor boolean check for checking if a proposal is a plugin update proposal
+- Rename `applyUpdateAction` to `applyUpdateActionAndPermissionsBlock`
+- Rename `isDaoUpdateAction`to `containsDaoUpdateAction`
+- Rename `isDaoUpdate` to `isDaoUpdateProposal`
+- Rename `isDaoUpdateValid` to `isDaoUpdateProposalValid` 
+- Rename `isPluginUpdate` to `isPluginUpdateProposal`
+- Rename `isPluginUpdateValid` to `isPluginUpdateProposalValid` 
+- Rename `isPluginUpdateAction` to `containsPluginUpdateAction`
+- Rename `isPluginUpdateActionBlockWithRootPermission` to `containsPluginUpdateActionBlockWithRootPermission`
+
+
+
+## [1.19.0]
+
 ### Added
 
 - `isMember` function to `TokenVotingClient`

modules/client/examples/01-client/16-is-plugin-update-valid.ts

@@ -21,7 +21,7 @@ const actions: DaoAction[] = [
 ];
 
 // check if a plugin update proposal is valid
-const isValid = client.methods.isPluginUpdateValid({
+const isValid = client.methods.isPluginUpdateProposalValid({
   daoAddress: "0x1234567890123456789012345678901234567890",
   actions,
 });

modules/client/examples/01-client/18-is-dao-update-valid.ts

@@ -21,7 +21,7 @@ const actions: DaoAction[] = [
 ];
 
 // check if a dap update proposal is valid
-const isValid = client.methods.isDaoUpdateValid({
+const isValid = client.methods.isDaoUpdateProposalValid({
   daoAddress: "0x1234567890123456789012345678901234567890",
   actions,
 });

modules/client/examples/01-client/19-is-dao-update-proposal.ts

@@ -24,7 +24,7 @@ const actions: DaoAction[] = [
 ];
 
 // check if a plugin update proposal is valid
-const isValid = client.methods.isDaoUpdateProposal(actions);
+const isValid = client.methods.isDaoUpdateProposalProposal(actions);
 
 console.log(isValid);
 

modules/client/examples/05-encoders-decoders/15-apply-update.ts

@@ -42,7 +42,7 @@ const applyUpdateParams: ApplyUpdateParams = {
 
 const daoAddressOrEns: string = "0x123123123123123123123123123123123123"; // "my-dao.eth"
 
-const actions: DaoAction[] = client.encoding.applyUpdateAction(
+const actions: DaoAction[] = client.encoding.applyUpdateAndPermissionsActionBlock(
   daoAddressOrEns,
   applyUpdateParams,
 );

modules/client/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@aragon/sdk-client",
   "author": "Aragon Association",
-  "version": "1.19.0",
+  "version": "1.19.1",
   "license": "MIT",
   "main": "dist/index.js",
   "module": "dist/sdk-client.esm.js",
@@ -62,7 +62,7 @@
   },
   "dependencies": {
     "@aragon/osx-ethers": "1.3.0-rc0.4",
-    "@aragon/sdk-client-common": "^1.11.0",
+    "@aragon/sdk-client-common": "^1.11.1",
     "@aragon/sdk-ipfs": "^1.1.0",
     "@ethersproject/abstract-signer": "^5.5.0",
     "@ethersproject/bignumber": "^5.6.0",

modules/client/src/internal/client/decoding.ts

@@ -20,6 +20,7 @@ import {
   decodeApplyUpdateAction,
   decodeGrantAction,
   decodeInitializeFromAction,
+  decodeUpgradeToAction,
   decodeUpgradeToAndCallAction,
   findInterface,
   permissionParamsFromContract,
@@ -308,13 +309,7 @@ export class ClientDecoding extends ClientCore implements IClientDecoding {
     return result[0];
   }
   public upgradeToAction(data: Uint8Array): string {
-    const daoInterface = DAO__factory.createInterface();
-    const hexBytes = bytesToHex(data);
-    const expectedFunction = daoInterface.getFunction(
-      "upgradeTo",
-    );
-    const result = daoInterface.decodeFunctionData(expectedFunction, hexBytes);
-    return result[0];
+    return decodeUpgradeToAction(data);
   }
   /**
    * Decodes upgradeToAndCallback params from an upgradeToAndCallAction

modules/client/src/internal/client/encoding.ts

@@ -152,7 +152,7 @@ export class ClientEncoding extends ClientCore implements IClientEncoding {
    * @return {*}  {DaoAction[]}
    * @memberof ClientEncoding
    */
-  public applyUpdateAction(
+  public applyUpdateAndPermissionsActionBlock(
     daoAddress: string,
     params: ApplyUpdateParams,
   ): DaoAction[] {
@@ -164,26 +164,52 @@ export class ClientEncoding extends ClientCore implements IClientEncoding {
       args,
     ]);
     const pspAddress = this.web3.getAddress("pluginSetupProcessorAddress");
-    // Grant ROOT_PERMISION in the DAO to the PSP
-    const grantAction = this.grantAction(daoAddress, {
-      where: daoAddress,
+
+    // Grant UPGRADE_PLUGIN_PERMISSION in the plugin to the PSP
+    const grantUpgradeAction = this.grantAction(daoAddress, {
+      where: params.pluginAddress,
       who: pspAddress,
-      permission: Permissions.ROOT_PERMISSION,
+      permission: Permissions.UPGRADE_PLUGIN_PERMISSION,
     });
-    // Revoke ROOT_PERMISION in the DAO to the PSP
-    const revokeAction = this.revokeAction(daoAddress, {
-      where: daoAddress,
+    // Revoke UPGRADE_PLUGIN_PERMISSION in the plugin to the PSP
+    const revokeUpgradeAction = this.revokeAction(daoAddress, {
+      where: params.pluginAddress,
       who: pspAddress,
-      permission: Permissions.ROOT_PERMISSION,
+      permission: Permissions.UPGRADE_PLUGIN_PERMISSION,
     });
+    // If the update requests permissions to be granted or revoked, the PSP needs temporary `ROOT_PERMISSION_ID` permission
+    if (params.permissions.length > 0) {
+      const grantRootAction = this.grantAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.ROOT_PERMISSION,
+      });
+      // Revoke ROOT_PERMISSION in the DAO to the PSP
+      const revokeRootAction = this.revokeAction(daoAddress, {
+        where: daoAddress,
+        who: pspAddress,
+        permission: Permissions.ROOT_PERMISSION,
+      });
+      return [
+        grantUpgradeAction,
+        grantRootAction,
+        {
+          to: pspAddress,
+          value: BigInt(0),
+          data: hexToBytes(hexBytes),
+        },
+        revokeRootAction,
+        revokeUpgradeAction,
+      ];
+    }
     return [
-      grantAction,
+      grantUpgradeAction,
       {
         to: pspAddress,
         value: BigInt(0),
         data: hexToBytes(hexBytes),
       },
-      revokeAction,
+      revokeUpgradeAction,
     ];
   }