Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C4 Audit Suggestions #360

Merged
merged 4 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/contracts/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added the `Multisig` plugin and setup contract.
- Added a `VotingMode` enumeration to specify if the vote should be conducted in `Standard`, `EarlyExecution`, or `VoteReplacement` mode.
- Added the `Admin` plugin and setup contract.
- Added NFT compatibility by using OpenZepplin's `IVotesUpgradeable` interface in `ERC20Voting` and renaming the contract to `TokenVoting`.
- Added NFT compatibility by using OpenZeppelin's `IVotesUpgradeable` interface in `ERC20Voting` and renaming the contract to `TokenVoting`.
- Added extra check in `PermissionManager` to disallow giving DAO specific permissions to ANY_ADDR + giving any other permissions
to ANY_ADDR unless oracle is passed. Also, freeze can only be used when where is not ANY_ADDR.
- Added `resolver` check in initialize function and `setDefaultResolver` of `ENSSubdomainRegistrar.sol`.
Expand Down Expand Up @@ -152,7 +152,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added `AllowlistVotingSetup` and `ERC20VotingSetup`.
- Added utility functions (`deployPluginRepoRegistry`, `deployPluginSetupProcessor`, `deployPluginRepoFactory`, and `filterEvents`) to the test suite.
- Added `DaoAuthorizableBase` class.
- Added `DaoAuthorizableClonable` using OpenZepplin initialization.
- Added `DaoAuthorizableClonable` using OpenZeppelin initialization.
- Added mocks and tests for the `Plugin` and `PluginSetup` classes.
- Added `PluginSetupProcessor` to be the main class processing `PluginSetup` contracts and applying permissions in the installing DAO.
- Added `DaoAuthorizableUpgradeable` and a free `_auth` function to provide an `auth` modifier to the different plugin types and prevent code duplication.
Expand All @@ -162,7 +162,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added helper function `test/test-utils/ens.ts` deploying the `ENSSubdomainRegistrar` and `ENS`-related contracts.
- Added Multi Target Bulk Permission object for `PermissionManager` with the oracle option as well.
- Added Abstract `PluginSetup` for the devs to inherit from for their concrete plugin manager implementation.
- Added the `solidity-docgen` hardhat plugin by OpenZepplin to automatically generate documentation via `yarn docgen`.
- Added the `solidity-docgen` hardhat plugin by OpenZeppelin to automatically generate documentation via `yarn docgen`.
- Added deployment script for `ENSSubdomainRegistrar`.
- Added `ENSSubdomainRegistrar` `Component` to register subdomains at the ENS.
- Added `IPluginRepo` interface for plugin PluginRepo contract.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ This contract must implement the `IPermissionCondition` interface.
interface IPermissionCondition {
/// @notice This method is used to check if a call is permitted.
/// @param _where The address of the target contract.
/// @param _who The address (EOA or contract) for which the permission are checked.
/// @param _who The address (EOA or contract) for which the permissions are checked.
/// @param _permissionId The permission identifier.
/// @param _data Optional data passed to the `PermissionCondition` implementation.
/// @return allowed Returns true if the call is permitted.
Expand Down Expand Up @@ -170,10 +170,10 @@ Here, the permission condition will only allow the call if the PoH registry conf
In another use-case, we might want to make sure that the `sendCoins` function can only be called if the ETH price in USD is above a certain threshold:

<!-- prettier-ignore -->
```solidity title="PriceOracle.sol"
```solidity title="PriceOracleCondition.sol"
import '@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol';

contract PriceOracle is IPermissionCondition {
contract PriceOracleCondition is IPermissionCondition {
AggregatorV3Interface internal priceFeed;

// Network: Goerli
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ This concatenated information is then stored as `keccak256` hashes inside a mapp
mapping(bytes32 => address) internal permissionsHashed;
```

Here, the `bytes32` keys are the permission hashes and the `address` values are either zero-address flags, such as `ALLOW_FLAG = address(0)` and `UNSET_FLAG = address(2)` indicating if the permission is set, or an actual address pointing to a `PermissionCondition` contract, which is discussed in the next section of this guide.
Here, the `bytes32` keys are the permission hashes and the `address` values are either zero-address flags, such as `ALLOW_FLAG = address(2)` and `UNSET_FLAG = address(0)` indicating if the permission is set, or an actual address pointing to a `PermissionCondition` contract, which is discussed in the next section of this guide.

### Authorization Modifiers

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion packages/contracts/docs/osx/01-how-it-works/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ The Aragon OSx DAO framework is structured as follows:
The foundation of the Aragon OSx protocol is the **code layer** constituted by the core and framework related contracts.
The [core contracts](./01-core/index.md) provide the core primitives intended to be used by users and implemented by developers of the DAO framework.
The [framework contracts](./02-framework/index.md) provide the infrastructure to easily create and manage your DAOs and plugins easy.
Both are built on top of external dependencies, most notably the [OpenZepplin](https://www.openzeppelin.com/contracts) and the [Ethereum Name Service (ENS)](https://docs.ens.domains/) contracts.
Both are built on top of external dependencies, most notably the [OpenZeppelin](https://www.openzeppelin.com/contracts) and the [Ethereum Name Service (ENS)](https://docs.ens.domains/) contracts.

The core and framework contracts are free to use, and no additional fees are charged.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Upgradeable plugin contracts (i.e., `PluginUUPSUpgradeable` implementations) mus

### Upgradeability & Deployment

Upgradeability and the deployment method of a plugin contract go hand in hand. The motivation behind upgrading smart contracts is nicely summarized by OpenZepplin:
Upgradeability and the deployment method of a plugin contract go hand in hand. The motivation behind upgrading smart contracts is nicely summarized by OpenZeppelin:

> Smart contracts in Ethereum are immutable by default. Once you create them there is no way to alter them, effectively acting as an unbreakable contract among participants.
>
Expand All @@ -37,9 +37,9 @@ Upgradeability and the deployment method of a plugin contract go hand in hand. T
> 3. Update all contracts that interacted with the old contract to use the address of the new one
> 4. Reach out to all your users and convince them to start using the new deployment (and handle both contracts being used simultaneously, as users are slow to migrate
>
> _source: [OpenZepplin: What's in an upgrade](https://docs.openzeppelin.com/learn/upgrading-smart-contracts#whats-in-an-upgrade)_
> _source: [OpenZeppelin: What's in an upgrade](https://docs.openzeppelin.com/learn/upgrading-smart-contracts#whats-in-an-upgrade)_

With upgradeable smart contracts, you can modify their code while keep using or even extending the storage (see the guide [Writing Upgradeable Contracts](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable) by OpenZepplin).
With upgradeable smart contracts, you can modify their code while keep using or even extending the storage (see the guide [Writing Upgradeable Contracts](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable) by OpenZeppelin).

To enable upgradeable smart contracts (as well as cheap contract clones), the proxy pattern is used.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ contract SimpleAdmin is PluginCloneable {

</details>

We must protect it from being called multiple times by using [OpenZepplin's `initializer` modifier made available through `Initalizable`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable) and call the internal function `__PluginCloneable_init(IDAO _dao)` available through the `PluginCloneable` base contract to store the `IDAO _dao` reference in the right place.
We must protect it from being called multiple times by using [OpenZeppelin's `initializer` modifier made available through `Initalizable`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable) and call the internal function `__PluginCloneable_init(IDAO _dao)` available through the `PluginCloneable` base contract to store the `IDAO _dao` reference in the right place.

:::caution
If you forget calling `__PluginCloneable_init(_dao)` inside your `initialize` function, your plugin won't be associated with a DAO and cannot use the DAO's `PermissionManager`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ function prepareInstallation(
address _dao,
bytes calldata _data
) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
// Decode `_data` to extract the params needed for cloning and initializing `Admin` plugin.
// Decode `_data` to extract the params needed for cloning and initializing the `Admin` plugin.
address admin = abi.decode(_data, (address));

if (admin == address(0)) {
Expand All @@ -134,7 +134,7 @@ function prepareInstallation(
PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](2);

// Grant the `ADMIN_EXECUTE_PERMISSION` of the Plugin to the admin.
// Grant the `ADMIN_EXECUTE_PERMISSION` of the plugin to the admin.
permissions[0] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Grant,
where: plugin,
Expand All @@ -158,7 +158,7 @@ function prepareInstallation(

</details>

At the top of the function, we first decode the `admin` address from the `_data` provided and check, that it is not accidentally set to `address(0)`. In the next step, we clone the plugin contract using [OpenZepplin's `Clones` library](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Clones) and initialize it with the `admin` address. For this to work, we had to include the following three code lines at the top of the file:
At the top of the function, we first decode the `admin` address from the `_data` provided and check, that it is not accidentally set to `address(0)`. In the next step, we clone the plugin contract using [OpenZeppelin's `Clones` library](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Clones) and initialize it with the `admin` address. For this to work, we had to include the following three code lines at the top of the file:

<details>
<summary><code>SimpleAdminSetup</code>: Adding the <code>Clones</code> library and the <code>error AdminAddressInvalid</code></summary>
Expand All @@ -181,7 +181,7 @@ contract SimpleAdminSetup is PluginSetup {

Finally, we construct an array of permission operations requesting two permissions to be granted that is returned by the function. First, we request the `ADMIN_EXECUTE_PERMISSION_ID` permission for the `admin` (`who`) address on the `plugin` (`where`). Second, we request the `EXECUTE_PERMISSION_ID` permission for the freshly deployed `plugin` (`who`) on the `_dao` (`where`). We don't add conditions to the permissions, so we use the `NO_CONDITION` constant provided by `PermissionLib`.

The first line, `using Clones for address;`, allows us to call OpenZepplin `Clones` library to clone contracts deployed at an address.
The first line, `using Clones for address;`, allows us to call OpenZeppelin `Clones` library to clone contracts deployed at an address.
The second line introduces a custom error being thrown if the admin address specified is the zero address.

### Implementing the `prepareUninstallation` function
Expand Down Expand Up @@ -262,7 +262,7 @@ contract SimpleAdminSetup is PluginSetup {
address _dao,
bytes calldata _data
) external returns (address plugin, PreparedSetupData memory preparedSetupData) {
// Decode `_data` to extract the params needed for cloning and initializing `Admin` plugin.
// Decode `_data` to extract the params needed for cloning and initializing the `Admin` plugin.
address admin = abi.decode(_data, (address));

if (admin == address(0)) {
Expand All @@ -279,7 +279,7 @@ contract SimpleAdminSetup is PluginSetup {
PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](2);

// Grant the `ADMIN_EXECUTE_PERMISSION` of the Plugin to the admin.
// Grant the `ADMIN_EXECUTE_PERMISSION` of the plugin to the admin.
permissions[0] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Grant,
where: plugin,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ contract SimpleStorageBuild1 is PluginUUPSUpgradeable {

</details>

To discriminate the initialize functions of different builds, we name it `initializeBuild1` this time. Again, you must protect it from being used multiple times by using [OpenZepplin's `initializer` modifier made available through `Initalizable`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable) and call the internal function `__PluginUUPSUpgradeable_init(IDAO _dao)` available through the `PluginUUPSUpgradeable` base contract storing the `IDAO _dao` reference in the right place.
To discriminate the initialize functions of different builds, we name it `initializeBuild1` this time. Again, you must protect it from being used multiple times by using [OpenZeppelin's `initializer` modifier made available through `Initalizable`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable) and call the internal function `__PluginUUPSUpgradeable_init(IDAO _dao)` available through the `PluginUUPSUpgradeable` base contract storing the `IDAO _dao` reference in the right place.

This becomes more demanding for subsequent builds of your plugin.

Expand Down Expand Up @@ -77,4 +77,4 @@ contract SimpleStorageBuild2 is PluginUUPSUpgradeable {
In general, for each version for which you want to support updates from, you have to provide a separate `initializeFromBuildX` function taking care of initializing the storage and transferring the `helpers` and `permissions` of the previous version into the same state as if it had been freshly installed.
Each `initializeBuildX` must be protected with a modifier that allows it to be only called once.

In contrast to build 1, we now must use [OpenZepplin's `modifier reinitializer(uint8 build)`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-reinitializer-uint8-) for build 2 instead of `modifier initializer` because it allows us to execute 255 subsequent initializations. More specifically, we used `reinitializer(2)` here for our build 2. Note that we could also have used `function initializeBuild1(IDAO _dao, uint256 _number) external reinitializer(1)` for build 1 because `initializer` and `reinitializer(1)` are equivalent statements. For build 3, we must use `reinitializer(3)`, for build 4 `reinitializer(4)` and so on.
In contrast to build 1, we now must use [OpenZeppelin's `modifier reinitializer(uint8 build)`](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable-reinitializer-uint8-) for build 2 instead of `modifier initializer` because it allows us to execute 255 subsequent initializations. More specifically, we used `reinitializer(2)` here for our build 2. Note that we could also have used `function initializeBuild1(IDAO _dao, uint256 _number) external reinitializer(1)` for build 1 because `initializer` and `reinitializer(1)` are equivalent statements. For build 3, we must use `reinitializer(3)`, for build 4 `reinitializer(4)` and so on.
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ contract SimpleStorageBuild2 is PluginUUPSUpgradeable {

</details>

Builds that you publish don't necessarily need to introduce new storage varaibles of your contracts don't necessarily need to change the storage.
Builds that you publish don't necessarily need to introduce new storage variables of your contracts don't necessarily need to change the storage.

#### Build 3: Changes Affecting the Bytecode

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ title: Meta Transactions
## Supporting Meta Transactions

Our plugins are compatible with the [ERC-2771 (Meta Transaction)](https://eips.ethereum.org/EIPS/eip-2771) standard, which allows users to send gasless transactions, also known as meta transactions.
This is possible because we use `_msgSender` and `_msgData` context from OpenZepplin's `Context` and `ContextUpgradeable` in our `Plugin`, `PluginCloneable`, and `PluginUUPSUpgradeable` classes.
This is possible because we use `_msgSender` and `_msgData` context from OpenZeppelin's `Context` and `ContextUpgradeable` in our `Plugin`, `PluginCloneable`, and `PluginUUPSUpgradeable` classes.

To support meta transactions, your implementation contract must inherit and override the `Context` implementation with the `_msgSender` and `_msgData` functions provided in OpenGSN's `BaseRelayRecipient`, and use the [DAO's trusted forwarder](../../01-how-it-works/01-core/01-dao/index.md#6-meta-transaction-compatibility).

Expand Down
Loading