feature: targets and standardized proposal (#99)

* proposal and plugins interfaces changes with backwards compatibility * add artifacts folder in gitignore * fix calldata to memory * add tests * add operation type for target * feat: add tests and refactor (#100) * feat: add test for checking the ExecuteFailed is properly emitted * feat: add new case in mock executor to simulate the tests * feat: refactor code to avoid duplications * ci: fix solhint warnings * ci: fix lint issues * remove previous target from targetset event * add get proposalid in the interface * rename function * from calldata to memory * improved target config and virtual functions * add params byte approach (#102) * add params byte approach * add simple executor (#103) * add simple executor * action as free level export * executor added with reentrancy guard * call initialization only * add comments * fallback to dao as a target if no target set (#104) * metadata contracts (#105) * metadata contracts * rename * diamond storage for metadata * remove revert * Update contracts/src/executors/IExecutor.sol Co-authored-by: Jør∂¡ <[email protected]> * review changes * unused import remove * remove unusued import * from memory to calldata in metadataextension * create proposal id not enforced (#106) * create proposal id not enforced * new changes * remove unusued function * add condition (#108) * add condition * rename * fix linter * fix: executor 63/64 rule tests for coverage * feat: add tests for powerful condition contract * ci: undo changes * ci: rmv not used import * fix: prettier * feat: add tests for conditions that checks block number and timestamp * ci: prettier * ci: lint fix * Feat/rule condition clean up (#109) * cd: rename powerfulCondtion to RuledCondition * add natspecs * remove redandant imports * extension alwaystruecondition * extension folder * rename, fix and add tests * add rules updated event * Feat: add missing tests (#111) * feat: add test for always true condition * feat: modify mock code to allow sending the compare list on the data * feat: add tests for rule condition * feat: simplify ruled condition tests * fix lint * add supportsinterface on executor * change const name --------- Co-authored-by: Claudia <[email protected]> Co-authored-by: Rekard0 <[email protected]> * remove always true condition * add hasSucceeded instead of (#112) * add hasSucceeded instead of * remove .only --------- Co-authored-by: Claudia Barcelo <[email protected]> Co-authored-by: Jør∂¡ <[email protected]> Co-authored-by: Rekard0 <[email protected]>
aragon · Nov 5, 2024 · 7c9546c · 7c9546c
1 parent 0fd5cb0
commit 7c9546c
Show file tree

Hide file tree

Showing 36 changed files with 3,722 additions and 448 deletions.
diff --git a/.gitignore b/.gitignore
@@ -19,6 +19,10 @@ dist-ssr
 .cache
 typechain
 
+contracts/artifacts
+contracts/cache 
+
+
 # misc
 .DS_Store
 *.pem

diff --git a/contracts/CHANGELOG.md b/contracts/CHANGELOG.md
@@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - Use [OpenZepplin v4.9.6](https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.9.6).
-- Updated hardhat configuration to use the `commons- config` networks.
+- Updated hardhat configuration to use the `commons-config` networks.
 
 ## v1.4.0
 

diff --git a/contracts/src/dao/IDAO.sol b/contracts/src/dao/IDAO.sol
@@ -7,16 +7,6 @@ pragma solidity ^0.8.8;
 /// @notice The interface required for DAOs within the Aragon App DAO framework.
 /// @custom:security-contact [email protected]
 interface IDAO {
-    /// @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;
-    }
-
     /// @notice Checks if an address has permission on a contract via a permission identifier and considers if `ANY_ADDRESS` was used in the granting process.
     /// @param _where The address of the contract.
     /// @param _who The address of a EOA or contract to give the permissions.
@@ -38,35 +28,6 @@ interface IDAO {
     /// @param metadata The IPFS hash of the new metadata object.
     event MetadataSet(bytes metadata);
 
-    /// @notice Executes a list of actions. If a zero allow-failure map is provided, a failing action reverts the entire execution. If a non-zero allow-failure map is provided, allowed actions can fail without the entire call being reverted.
-    /// @param _callId The ID of the call. The definition of the value of `callId` is up to the calling contract and can be used, e.g., as a nonce.
-    /// @param _actions The array of actions.
-    /// @param _allowFailureMap A bitmap allowing execution to succeed, even if individual actions might revert. If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
-    /// @return The array of results obtained from the executed actions in `bytes`.
-    /// @return The resulting failure map containing the actions have actually failed.
-    function execute(
-        bytes32 _callId,
-        Action[] memory _actions,
-        uint256 _allowFailureMap
-    ) external returns (bytes[] memory, uint256);
-
-    /// @notice Emitted when a proposal is executed.
-    /// @param actor The address of the caller.
-    /// @param callId The ID of the call.
-    /// @param actions The array of actions executed.
-    /// @param allowFailureMap The allow failure map encoding which actions are allowed to fail.
-    /// @param failureMap The failure map encoding which actions have failed.
-    /// @param execResults The array with the results of the executed actions.
-    /// @dev The value of `callId` is defined by the component/contract calling the execute function. A `Plugin` implementation can use it, for example, as a nonce.
-    event Executed(
-        address indexed actor,
-        bytes32 callId,
-        Action[] actions,
-        uint256 allowFailureMap,
-        uint256 failureMap,
-        bytes[] execResults
-    );
-
     /// @notice Emitted when a standard callback is registered.
     /// @param interfaceId The ID of the interface.
     /// @param callbackSelector The selector of the callback function.

diff --git a/contracts/src/executors/Executor.sol b/contracts/src/executors/Executor.sol
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
+
+import {IExecutor, Action} from "./IExecutor.sol";
+import {flipBit, hasBit} from "../utils/math/BitMap.sol";
+
+/// @notice Simple Executor that loops through the actions and executes them.
+/// @dev This doesn't use any type of permission for execution and can be called by anyone.
+/// Most useful use-case is to deploy as non-upgradeable and call from another contract via delegatecall.
+/// If used with delegatecall, DO NOT add state variables in sequential slots, otherwise this will overwrite
+/// the storage of the calling contract.
+contract Executor is IExecutor, ERC165 {
+    /// @notice The internal constant storing the maximal action array length.
+    uint256 internal constant MAX_ACTIONS = 256;
+
+    // keccak256("osx-commons.storage.Executor")
+    bytes32 private constant REENTRANCY_GUARD_STORAGE_LOCATION =
+        0x4d6542319dfb3f7c8adbb488d7b4d7cf849381f14faf4b64de3ac05d08c0bdec;
+
+    /// @notice The first out of two values to which the `_reentrancyStatus` state variable (used by the `nonReentrant` modifier) can be set indicating that a function was not entered.
+    uint256 private constant _NOT_ENTERED = 1;
+
+    /// @notice The second out of two values to which the `_reentrancyStatus` state variable (used by the `nonReentrant` modifier) can be set indicating that a function was entered.
+    uint256 private constant _ENTERED = 2;
+
+    /// @notice Thrown if the action array length is larger than `MAX_ACTIONS`.
+    error TooManyActions();
+
+    /// @notice Thrown if an action has insufficient gas left.
+    error InsufficientGas();
+
+    /// @notice Thrown if action execution has failed.
+    /// @param index The index of the action in the action array that failed.
+    error ActionFailed(uint256 index);
+
+    /// @notice Thrown if a call is reentrant.
+    error ReentrantCall();
+
+    constructor() {
+        _storeReentrancyStatus(_NOT_ENTERED);
+    }
+
+    modifier nonReentrant() {
+        if (_getReentrancyStatus() == _ENTERED) {
+            revert ReentrantCall();
+        }
+
+        _storeReentrancyStatus(_ENTERED);
+
+        _;
+
+        _storeReentrancyStatus(_NOT_ENTERED);
+    }
+
+    /// @notice Checks if this or the parent contract supports an interface by its ID.
+    /// @param _interfaceId The ID of the interface.
+    /// @return Returns `true` if the interface is supported.
+    function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
+        return _interfaceId == type(IExecutor).interfaceId || super.supportsInterface(_interfaceId);
+    }
+
+    /// @inheritdoc IExecutor
+    function execute(
+        bytes32 _callId,
+        Action[] memory _actions,
+        uint256 _allowFailureMap
+    )
+        public
+        virtual
+        override
+        nonReentrant
+        returns (bytes[] memory execResults, uint256 failureMap)
+    {
+        // Check that the action array length is within bounds.
+        if (_actions.length > MAX_ACTIONS) {
+            revert TooManyActions();
+        }
+
+        execResults = new bytes[](_actions.length);
+
+        uint256 gasBefore;
+        uint256 gasAfter;
+
+        for (uint256 i = 0; i < _actions.length; ) {
+            gasBefore = gasleft();
+
+            (bool success, bytes memory data) = _actions[i].to.call{value: _actions[i].value}(
+                _actions[i].data
+            );
+
+            gasAfter = gasleft();
+
+            // Check if failure is allowed
+            if (!success) {
+                if (!hasBit(_allowFailureMap, uint8(i))) {
+                    revert ActionFailed(i);
+                }
+
+                // Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)).
+                // In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit
+                // where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call.
+                if (gasAfter < gasBefore / 64) {
+                    revert InsufficientGas();
+                }
+                // Store that this action failed.
+                failureMap = flipBit(failureMap, uint8(i));
+            }
+
+            execResults[i] = data;
+
+            unchecked {
+                ++i;
+            }
+        }
+
+        emit Executed({
+            actor: msg.sender,
+            callId: _callId,
+            actions: _actions,
+            allowFailureMap: _allowFailureMap,
+            failureMap: failureMap,
+            execResults: execResults
+        });
+    }
+
+    /// @notice Gets the current reentrancy status.
+    /// @return status This returns the current reentrancy status.
+    function _getReentrancyStatus() private view returns (uint256 status) {
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            status := sload(REENTRANCY_GUARD_STORAGE_LOCATION)
+        }
+    }
+
+    /// @notice Stores the reentrancy status on a specific slot.
+    function _storeReentrancyStatus(uint256 _status) private {
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            sstore(REENTRANCY_GUARD_STORAGE_LOCATION, _status)
+        }
+    }
+}
diff --git a/contracts/src/executors/IExecutor.sol b/contracts/src/executors/IExecutor.sol
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+/// @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;
+}
+
+/// @title IExecutor
+/// @author Aragon X - 2022-2024
+/// @notice The interface required for Executors within the Aragon App DAO framework.
+/// @custom:security-contact [email protected]
+interface IExecutor {
+    /// @notice Emitted when a proposal is executed.
+    /// @param actor The address of the caller.
+    /// @param callId The ID of the call.
+    /// @param actions The array of actions executed.
+    /// @param allowFailureMap The allow failure map encoding which actions are allowed to fail.
+    /// @param failureMap The failure map encoding which actions have failed.
+    /// @param execResults The array with the results of the executed actions.
+    /// @dev The value of `callId` is defined by the component/contract calling the execute function. A `Plugin` implementation can use it, for example, as a nonce.
+    event Executed(
+        address indexed actor,
+        bytes32 callId,
+        Action[] actions,
+        uint256 allowFailureMap,
+        uint256 failureMap,
+        bytes[] execResults
+    );
+
+    /// @notice Executes a list of actions. If a zero allow-failure map is provided, a failing action reverts the entire execution. If a non-zero allow-failure map is provided, allowed actions can fail without the entire call being reverted.
+    /// @param _callId The ID of the call. The definition of the value of `callId` is up to the calling contract and can be used, e.g., as a nonce.
+    /// @param _actions The array of actions.
+    /// @param _allowFailureMap A bitmap allowing execution to succeed, even if individual actions might revert. If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert.
+    /// @return The array of results obtained from the executed actions in `bytes`.
+    /// @return The resulting failure map containing the actions have actually failed.
+    function execute(
+        bytes32 _callId,
+        Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory, uint256);
+}
diff --git a/contracts/src/mocks/dao/DAOMock.sol b/contracts/src/mocks/dao/DAOMock.sol
@@ -3,10 +3,11 @@
 pragma solidity ^0.8.8;
 
 import {IDAO} from "../../dao/IDAO.sol";
+import {IExecutor, Action} from "../../executors/IExecutor.sol";
 
 /// @notice A mock DAO that anyone can set permissions in.
 /// @dev DO NOT USE IN PRODUCTION!
-contract DAOMock is IDAO {
+contract DAOMock is IDAO, IExecutor {
     bool public hasPermissionReturnValueMock;
 
     function setHasPermissionReturnValueMock(bool _hasPermissionReturnValueMock) external {

diff --git a/contracts/src/mocks/executors/ActionExecute.sol b/contracts/src/mocks/executors/ActionExecute.sol
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {IExecutor, Action} from "../../executors/Executor.sol";
+
+/// @notice A dummy contract to test if Executor can successfully execute an action.
+contract ActionExecute {
+    uint256 internal _num = 10;
+
+    function setTest(uint256 newNum) public returns (uint256) {
+        _num = newNum;
+        return _num;
+    }
+
+    function fail() public pure {
+        // solhint-disable-next-line reason-string, custom-errors
+        revert("ActionExecute:Revert");
+    }
+
+    // Used to test custom reentrancy guard
+    // that is implemented on Executor contract's
+    // execute function.
+    function callBackCaller() public {
+        IExecutor(msg.sender).execute(bytes32(0), new Action[](0), 0);
+    }
+}
diff --git a/contracts/src/mocks/executors/GasConsumer.sol b/contracts/src/mocks/executors/GasConsumer.sol
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+/// @notice This contract is used for testing to consume gas.
+contract GasConsumer {
+    // solhint-disable-next-line named-parameters-mapping
+    mapping(uint256 => uint256) public store;
+
+    function consumeGas(uint256 count) external {
+        for (uint256 i = 0; i < count; i++) {
+            store[i] = 1;
+        }
+    }
+}
diff --git a/contracts/src/mocks/permission/condition/extensions/RuledConditionMock.sol b/contracts/src/mocks/permission/condition/extensions/RuledConditionMock.sol
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+import {RuledCondition} from "../../../../permission/condition/extensions/RuledCondition.sol";
+import {DaoAuthorizableUpgradeable} from "../../../../permission/auth/DaoAuthorizableUpgradeable.sol";
+
+/// @notice A mock powerful condition to expose internal functions
+/// @dev DO NOT USE IN PRODUCTION!
+contract RuledConditionMock is DaoAuthorizableUpgradeable, RuledCondition {
+    function updateRules(Rule[] memory _rules) public virtual {
+        _updateRules(_rules);
+    }
+
+    function isGranted(
+        address _where,
+        address _who,
+        bytes32 _permissionId,
+        bytes calldata data
+    ) external view override returns (bool isPermitted) {
+        uint256[] memory _compareList = data.length == 0
+            ? new uint256[](0)
+            : abi.decode(data, (uint256[]));
+        return _evalRule(0, _where, _who, _permissionId, _compareList);
+    }
+}
diff --git a/contracts/src/mocks/plugin/CustomExecutorMock.sol b/contracts/src/mocks/plugin/CustomExecutorMock.sol
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {IExecutor, Action} from "../../executors/IExecutor.sol";
+
+/// @notice A mock DAO that anyone can set permissions in.
+/// @dev DO NOT USE IN PRODUCTION!
+contract CustomExecutorMock is IExecutor {
+    error Failed();
+
+    function execute(
+        bytes32 callId,
+        Action[] memory _actions,
+        uint256 allowFailureMap
+    ) external override returns (bytes[] memory execResults, uint256 failureMap) {
+        if (callId == bytes32(0)) {
+            revert Failed();
+        } else if (callId == bytes32(uint256(123))) {
+            // solhint-disable-next-line reason-string, custom-errors
+            revert();
+        } else {
+            emit Executed(msg.sender, callId, _actions, allowFailureMap, failureMap, execResults);
+        }
+    }
+}