Skip to content

Commit

Permalink
Merge branch 'develop' into feat/OS-816-remove-tsdx-in-all-packages
Browse files Browse the repository at this point in the history
  • Loading branch information
mathewmeconry authored Nov 24, 2023
2 parents 0aadbf6 + 2588415 commit 3d1ead0
Show file tree
Hide file tree
Showing 25 changed files with 3,129 additions and 1,124 deletions.
121 changes: 121 additions & 0 deletions docs/sdk/03-update-security-check/index.md
Original file line number Diff line number Diff line change
@@ -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"`


7 changes: 7 additions & 0 deletions modules/client-common/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

### Changed
Expand Down
2 changes: 1 addition & 1 deletion modules/client-common/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
3 changes: 2 additions & 1 deletion modules/client-common/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ export const ADDITIONAL_NETWORKS: Network[] = [
},
{
name: "sepolia",
chainId: 58008,
chainId: 11155111,
},
{
name: "local",
Expand All @@ -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",
Expand Down
20 changes: 20 additions & 0 deletions modules/client/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
4 changes: 2 additions & 2 deletions modules/client/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down Expand Up @@ -68,7 +68,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",
Expand Down
9 changes: 2 additions & 7 deletions modules/client/src/internal/client/decoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
decodeApplyUpdateAction,
decodeGrantAction,
decodeInitializeFromAction,
decodeUpgradeToAction,
decodeUpgradeToAndCallAction,
findInterface,
permissionParamsFromContract,
Expand Down Expand Up @@ -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
Expand Down
48 changes: 37 additions & 11 deletions modules/client/src/internal/client/encoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export class ClientEncoding extends ClientCore implements IClientEncoding {
* @return {*} {DaoAction[]}
* @memberof ClientEncoding
*/
public applyUpdateAction(
public applyUpdateAndPermissionsActionBlock(
daoAddress: string,
params: ApplyUpdateParams,
): DaoAction[] {
Expand All @@ -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,
];
}

Expand Down
Loading

0 comments on commit 3d1ead0

Please sign in to comment.