diff --git a/.gitignore b/.gitignore index b9ff0afd..2c3cd5ab 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,10 @@ dist-ssr .cache typechain +contracts/artifacts +contracts/cache + + # misc .DS_Store *.pem diff --git a/contracts/src/dao/IDAO.sol b/contracts/src/dao/IDAO.sol index ba1151eb..133970a8 100644 --- 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 sirt@aragon.org 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 new file mode 100644 index 00000000..d86338bc --- /dev/null +++ b/contracts/src/executors/Executor.sol @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +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 { + /// @notice The internal constant storing the maximal action array length. + uint256 internal constant MAX_ACTIONS = 256; + + // keccak256("osx-commons.storage.Executor") + bytes32 private constant ReentrancyGuardStorageLocation = + 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); + } + + /// @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 (!hasBit(_allowFailureMap, uint8(i))) { + // Check if the call failed. + if (!success) { + revert ActionFailed(i); + } + } else { + // Check if the call failed. + if (!success) { + // 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) { + assembly { + status := sload(ReentrancyGuardStorageLocation) + } + } + + /// @notice Stores the reentrancy status on a specific slot. + function _storeReentrancyStatus(uint256 _status) private { + assembly { + sstore(ReentrancyGuardStorageLocation, _status) + } + } +} diff --git a/contracts/src/executors/IExecutor.sol b/contracts/src/executors/IExecutor.sol new file mode 100644 index 00000000..2961b87e --- /dev/null +++ b/contracts/src/executors/IExecutor.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {IDAO} from "../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; +} + +/// @title IDAO +/// @author Aragon X - 2022-2023 +/// @notice The interface required for Executors within the Aragon App DAO framework. +/// @custom:security-contact sirt@aragon.org +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 index dbd49e1a..ff8ffc21 100644 --- 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 new file mode 100644 index 00000000..36fcd754 --- /dev/null +++ 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"; +import "hardhat/console.sol"; + +/// @notice A dummy contract to test if Executor can successfully execute an action. +contract ActionExecute { + uint num = 10; + + function setTest(uint newNum) public returns (uint) { + num = newNum; + return num; + } + + function fail() public pure { + 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 new file mode 100644 index 00000000..1eb2db57 --- /dev/null +++ b/contracts/src/mocks/executors/GasConsumer.sol @@ -0,0 +1,14 @@ +// 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 { + 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/plugin/CustomExecutorMock.sol b/contracts/src/mocks/plugin/CustomExecutorMock.sol new file mode 100644 index 00000000..acab86e9 --- /dev/null +++ b/contracts/src/mocks/plugin/CustomExecutorMock.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +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 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); + } + } +} diff --git a/contracts/src/mocks/plugin/PluginCloneableMock.sol b/contracts/src/mocks/plugin/PluginCloneableMock.sol index 71dfd8bf..2a5a4ecd 100644 --- a/contracts/src/mocks/plugin/PluginCloneableMock.sol +++ b/contracts/src/mocks/plugin/PluginCloneableMock.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.8; import {PluginCloneable} from "../../plugin/PluginCloneable.sol"; import {IDAO} from "../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../executors/IExecutor.sol"; /// @notice A mock cloneable plugin to be deployed via the minimal proxy pattern. /// v1.1 (Release 1, Build 1) @@ -16,6 +17,30 @@ contract PluginCloneableMockBuild1 is PluginCloneable { __PluginCloneable_init(_dao); state1 = 1; } + + function execute( + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute( + _target, + bytes32(_callId), + _actions, + _allowFailureMap, + _op + ); + } } /// @notice A mock cloneable plugin to be deployed via the minimal proxy pattern. diff --git a/contracts/src/mocks/plugin/PluginMock.sol b/contracts/src/mocks/plugin/PluginMock.sol index 4fdaa558..22fa5cc5 100644 --- a/contracts/src/mocks/plugin/PluginMock.sol +++ b/contracts/src/mocks/plugin/PluginMock.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.8; import {Plugin} from "../../plugin/Plugin.sol"; import {IDAO} from "../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../executors/IExecutor.sol"; /// @notice A mock plugin to be deployed via the `new` keyword. /// v1.1 (Release 1, Build 1) @@ -14,4 +15,28 @@ contract PluginMockBuild1 is Plugin { constructor(IDAO _dao) Plugin(_dao) { state1 = 1; } + + function execute( + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute( + _target, + bytes32(_callId), + _actions, + _allowFailureMap, + _op + ); + } } diff --git a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol index ea9d5306..a792458c 100644 --- a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol +++ b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.8; import {PluginUUPSUpgradeable} from "../../plugin/PluginUUPSUpgradeable.sol"; import {IDAO} from "../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../executors/IExecutor.sol"; /// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern. /// v1.1 (Release 1, Build 1) @@ -16,6 +17,30 @@ contract PluginUUPSUpgradeableMockBuild1 is PluginUUPSUpgradeable { __PluginUUPSUpgradeable_init(_dao); state1 = 1; } + + function execute( + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap); + } + + function execute( + address _target, + uint256 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) external returns (bytes[] memory execResults, uint256 failureMap) { + (execResults, failureMap) = _execute( + _target, + bytes32(_callId), + _actions, + _allowFailureMap, + _op + ); + } } /// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern. diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol index ffa12da5..1bf07368 100644 --- a/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol +++ b/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol @@ -3,39 +3,29 @@ pragma solidity ^0.8.8; import {Proposal} from "../../../../plugin/extensions/proposal/Proposal.sol"; -import {IDAO} from "../../../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../../../executors/IExecutor.sol"; -/// @notice A mock contract containing functions to create and execute proposals. +/// @notice A mock contract. /// @dev DO NOT USE IN PRODUCTION! contract ProposalMock is Proposal { - function createProposalId() external returns (uint256 proposalId) { - proposalId = _createProposalId(); - } + // We don't need to test these below functions as they will be tested in the actual plugins. + // This mock contract is only used to test `supportsInterface` function. + // solhint-disable no-empty-blocks function createProposal( - address _creator, - bytes calldata _metadata, - uint64 _startDate, - uint64 _endDate, - IDAO.Action[] calldata _actions, - uint256 _allowFailureMap - ) external returns (uint256 proposalId) { - proposalId = _createProposal( - _creator, - _metadata, - _startDate, - _endDate, - _actions, - _allowFailureMap - ); - } + bytes memory data, + Action[] memory actions, + uint64 startDate, + uint64 endDate, + bytes memory + ) external returns (uint256 proposalId) {} - function executeProposal( - IDAO _dao, - uint256 _proposalId, - IDAO.Action[] memory _actions, - uint256 _allowFailureMap - ) external returns (bytes[] memory execResults, uint256 failureMap) { - (execResults, failureMap) = _executeProposal(_dao, _proposalId, _actions, _allowFailureMap); - } + function canExecute(uint256 proposalId) external view returns (bool) {} + + function createProposalId( + Action[] memory actions, + bytes memory metadata + ) external view returns (uint256) {} + + function createProposalParamsABI() external view returns (string memory) {} } diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol index fab1019c..ff06644a 100644 --- a/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol +++ b/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol @@ -3,39 +3,29 @@ pragma solidity ^0.8.8; import {ProposalUpgradeable} from "../../../../plugin/extensions/proposal/ProposalUpgradeable.sol"; -import {IDAO} from "../../../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../../../executors/IExecutor.sol"; -/// @notice A mock contract containing functions to create and execute proposals as well as storage gaps in inherited contracts to be used in UUPS upgradeable contracts. +/// @notice A mock contract. /// @dev DO NOT USE IN PRODUCTION! contract ProposalUpgradeableMock is ProposalUpgradeable { - function createProposalId() external returns (uint256 proposalId) { - proposalId = _createProposalId(); - } + // We don't need to test these below functions as they will be tested in the actual plugins. + // This mock contract is only used to test `supportsInterface` function. + // solhint-disable no-empty-blocks function createProposal( - address _creator, - bytes calldata _metadata, - uint64 _startDate, - uint64 _endDate, - IDAO.Action[] calldata _actions, - uint256 _allowFailureMap - ) external returns (uint256 proposalId) { - proposalId = _createProposal( - _creator, - _metadata, - _startDate, - _endDate, - _actions, - _allowFailureMap - ); - } + bytes memory data, + Action[] memory actions, + uint64 startDate, + uint64 endDate, + bytes memory + ) external returns (uint256 proposalId) {} - function executeProposal( - IDAO _dao, - uint256 _proposalId, - IDAO.Action[] memory _actions, - uint256 _allowFailureMap - ) external returns (bytes[] memory execResults, uint256 failureMap) { - (execResults, failureMap) = _executeProposal(_dao, _proposalId, _actions, _allowFailureMap); - } + function canExecute(uint256 proposalId) external view returns (bool) {} + + function createProposalId( + Action[] memory actions, + bytes memory metadata + ) external view returns (uint256) {} + + function createProposalParamsABI() external view returns (string memory) {} } diff --git a/contracts/src/plugin/Plugin.sol b/contracts/src/plugin/Plugin.sol index 66ffaf23..11b33e1f 100644 --- a/contracts/src/plugin/Plugin.sol +++ b/contracts/src/plugin/Plugin.sol @@ -3,18 +3,47 @@ pragma solidity ^0.8.8; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; +import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import {IProtocolVersion} from "../utils/versioning/IProtocolVersion.sol"; import {ProtocolVersion} from "../utils/versioning/ProtocolVersion.sol"; import {DaoAuthorizable} from "../permission/auth/DaoAuthorizable.sol"; import {IDAO} from "../dao/IDAO.sol"; import {IPlugin} from "./IPlugin.sol"; +import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title Plugin /// @author Aragon X - 2022-2023 /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the `new` keyword. /// @custom:security-contact sirt@aragon.org abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { + using ERC165Checker for address; + + enum Operation { + Call, + DelegateCall + } + + struct TargetConfig { + address target; + Operation operation; + } + + TargetConfig private currentTargetConfig; + + /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. + /// @param targetConfig The target config to update it to. + error InvalidTargetConfig(TargetConfig targetConfig); + + /// @dev Emitted each time the TargetConfig is set. + event TargetSet(TargetConfig newTargetConfig); + + /// @notice Thrown when `delegatecall` fails. + error ExecuteFailed(); + + /// @notice The ID of the permission required to call the `setTarget` function. + bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION"); + /// @notice Constructs the plugin by storing the associated DAO. /// @param _dao The DAO contract. constructor(IDAO _dao) DaoAuthorizable(_dao) {} @@ -24,13 +53,125 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion { return PluginType.Constructable; } - /// @notice Checks if this or the parent contract supports an interface by its ID. + /// @notice Returns the currently set target contract. + /// @return TargetConfig The currently set target. + function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { + return currentTargetConfig; + } + + /// @notice A convenient function to get current target config only if its target is not address(0), otherwise dao(). + /// @return TargetConfig The current target config if its target is not address(0), otherwise returns dao()." + function getTargetConfig() public view virtual returns (TargetConfig memory) { + TargetConfig memory targetConfig = currentTargetConfig; + + if (targetConfig.target == address(0)) { + targetConfig = TargetConfig({target: address(dao()), operation: Operation.Call}); + } + + return targetConfig; + } + + /// @dev Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function setTargetConfig( + TargetConfig calldata _targetConfig + ) public auth(SET_TARGET_PERMISSION_ID) { + _setTargetConfig(_targetConfig); + } + + /// @notice Checks if an interface is supported by this or its parent contract. /// @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(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || + _interfaceId == + this.setTargetConfig.selector ^ + this.getTargetConfig.selector ^ + this.getCurrentTargetConfig.selector || super.supportsInterface(_interfaceId); } + + /// @notice Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function _setTargetConfig(TargetConfig memory _targetConfig) internal virtual { + // safety check to avoid setting dao as `target` with `delegatecall` operation + // as this would not work and cause the plugin to be bricked. + if ( + _targetConfig.target.supportsInterface(type(IDAO).interfaceId) && + _targetConfig.operation == Operation.DelegateCall + ) { + revert InvalidTargetConfig(_targetConfig); + } + + currentTargetConfig = _targetConfig; + + emit TargetSet(_targetConfig); + } + + /// @notice Forwards the actions to the currently set `target` for the execution. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + return + _execute( + currentTargetConfig.target, + _callId, + _actions, + _allowFailureMap, + currentTargetConfig.operation + ); + } + + /// @notice Forwards the actions to the `target` for the execution. + /// @param _target Forwards the actions to the specific target. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + address _target, + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + if (_op == Operation.DelegateCall) { + bool success; + bytes memory data; + + // solhint-disable-next-line avoid-low-level-calls + (success, data) = _target.delegatecall( + abi.encodeCall(IExecutor.execute, (_callId, _actions, _allowFailureMap)) + ); + + if (!success) { + if (data.length > 0) { + // solhint-disable-next-line no-inline-assembly + assembly { + let returndata_size := mload(data) + revert(add(32, data), returndata_size) + } + } else { + revert ExecuteFailed(); + } + } + (execResults, failureMap) = abi.decode(data, (bytes[], uint256)); + } else { + (execResults, failureMap) = IExecutor(_target).execute( + _callId, + _actions, + _allowFailureMap + ); + } + } } diff --git a/contracts/src/plugin/PluginCloneable.sol b/contracts/src/plugin/PluginCloneable.sol index 08025c2c..cb1704c8 100644 --- a/contracts/src/plugin/PluginCloneable.sol +++ b/contracts/src/plugin/PluginCloneable.sol @@ -3,12 +3,14 @@ pragma solidity ^0.8.8; import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; +import {ERC165CheckerUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165CheckerUpgradeable.sol"; import {IProtocolVersion} from "../utils/versioning/IProtocolVersion.sol"; import {ProtocolVersion} from "../utils/versioning/ProtocolVersion.sol"; import {DaoAuthorizableUpgradeable} from "../permission/auth/DaoAuthorizableUpgradeable.sol"; import {IDAO} from "../dao/IDAO.sol"; import {IPlugin} from "./IPlugin.sol"; +import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title PluginCloneable /// @author Aragon X - 2022-2023 @@ -20,6 +22,34 @@ abstract contract PluginCloneable is DaoAuthorizableUpgradeable, ProtocolVersion { + using ERC165CheckerUpgradeable for address; + + enum Operation { + Call, + DelegateCall + } + + struct TargetConfig { + address target; + Operation operation; + } + + TargetConfig private currentTargetConfig; + + /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. + /// @param targetConfig The target config to update it to. + error InvalidTargetConfig(TargetConfig targetConfig); + + /// @notice Thrown when `delegatecall` fails. + error ExecuteFailed(); + + /// @dev Emitted each time the TargetConfig is set. + event TargetSet(TargetConfig newTargetConfig); + + /// @notice The ID of the permission required to call the `setTargetConfig` function. + bytes32 public constant SET_TARGET_CONFIG_PERMISSION_ID = + keccak256("SET_TARGET_CONFIG_PERMISSION"); + /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -33,11 +63,37 @@ abstract contract PluginCloneable is __DaoAuthorizableUpgradeable_init(_dao); } + /// @dev Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function setTargetConfig( + TargetConfig calldata _targetConfig + ) public auth(SET_TARGET_CONFIG_PERMISSION_ID) { + _setTargetConfig(_targetConfig); + } + /// @inheritdoc IPlugin function pluginType() public pure override returns (PluginType) { return PluginType.Cloneable; } + /// @notice Returns the currently set target contract. + /// @return TargetConfig The currently set target. + function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { + return currentTargetConfig; + } + + /// @notice A convenient function to get current target config only if its target is not address(0), otherwise dao(). + /// @return TargetConfig The current target config if its target is not address(0), otherwise returns dao()." + function getTargetConfig() public view virtual returns (TargetConfig memory) { + TargetConfig memory targetConfig = currentTargetConfig; + + if (targetConfig.target == address(0)) { + targetConfig = TargetConfig({target: address(dao()), operation: Operation.Call}); + } + + return targetConfig; + } + /// @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. @@ -45,6 +101,92 @@ abstract contract PluginCloneable is return _interfaceId == type(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || + _interfaceId == + this.setTargetConfig.selector ^ + this.getTargetConfig.selector ^ + this.getCurrentTargetConfig.selector || super.supportsInterface(_interfaceId); } + + /// @notice Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function _setTargetConfig(TargetConfig memory _targetConfig) internal virtual { + // safety check to avoid setting dao as `target` with `delegatecall` operation + // as this would not work and cause the plugin to be bricked. + if ( + _targetConfig.target.supportsInterface(type(IDAO).interfaceId) && + _targetConfig.operation == Operation.DelegateCall + ) { + revert InvalidTargetConfig(_targetConfig); + } + + currentTargetConfig = _targetConfig; + + emit TargetSet(_targetConfig); + } + + /// @notice Forwards the actions to the currently set `target` for the execution. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + return + _execute( + currentTargetConfig.target, + _callId, + _actions, + _allowFailureMap, + currentTargetConfig.operation + ); + } + + /// @notice Forwards the actions to the `target` for the execution. + /// @param _target Forwards the actions to the specific target. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + address _target, + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + if (_op == Operation.DelegateCall) { + bool success; + bytes memory data; + + // solhint-disable-next-line avoid-low-level-calls + (success, data) = _target.delegatecall( + abi.encodeCall(IExecutor.execute, (_callId, _actions, _allowFailureMap)) + ); + + if (!success) { + if (data.length > 0) { + // solhint-disable-next-line no-inline-assembly + assembly { + let returndata_size := mload(data) + revert(add(32, data), returndata_size) + } + } else { + revert ExecuteFailed(); + } + } + (execResults, failureMap) = abi.decode(data, (bytes[], uint256)); + } else { + (execResults, failureMap) = IExecutor(_target).execute( + _callId, + _actions, + _allowFailureMap + ); + } + } } diff --git a/contracts/src/plugin/PluginUUPSUpgradeable.sol b/contracts/src/plugin/PluginUUPSUpgradeable.sol index 32380f0d..fa7c7c02 100644 --- a/contracts/src/plugin/PluginUUPSUpgradeable.sol +++ b/contracts/src/plugin/PluginUUPSUpgradeable.sol @@ -5,12 +5,14 @@ pragma solidity ^0.8.8; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {IERC1822ProxiableUpgradeable} from "@openzeppelin/contracts-upgradeable/interfaces/draft-IERC1822Upgradeable.sol"; import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; +import {ERC165CheckerUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165CheckerUpgradeable.sol"; import {IProtocolVersion} from "../utils/versioning/IProtocolVersion.sol"; import {ProtocolVersion} from "../utils/versioning/ProtocolVersion.sol"; import {DaoAuthorizableUpgradeable} from "../permission/auth/DaoAuthorizableUpgradeable.sol"; -import {IDAO} from "../dao/IDAO.sol"; import {IPlugin} from "./IPlugin.sol"; +import {IDAO} from "../dao/IDAO.sol"; +import {IExecutor, Action} from "../executors/IExecutor.sol"; /// @title PluginUUPSUpgradeable /// @author Aragon X - 2022-2023 @@ -23,21 +25,79 @@ abstract contract PluginUUPSUpgradeable is DaoAuthorizableUpgradeable, ProtocolVersion { + using ERC165CheckerUpgradeable for address; + // NOTE: When adding new state variables to the contract, the size of `_gap` has to be adapted below as well. + enum Operation { + Call, + DelegateCall + } + + struct TargetConfig { + address target; + Operation operation; + } + + TargetConfig private currentTargetConfig; + + /// @notice Thrown when target is of type 'IDAO', but operation is `delegateCall`. + /// @param targetConfig The target config to update it to. + error InvalidTargetConfig(TargetConfig targetConfig); + + /// @notice Thrown when `delegatecall` fails. + error ExecuteFailed(); + + /// @notice Thrown when initialize is called after it has already been executed. + error AlreadyInitialized(); + + /// @dev Emitted each time the TargetConfig is set. + event TargetSet(TargetConfig newTargetConfig); + + /// @notice The ID of the permission required to call the `setTargetConfig` function. + bytes32 public constant SET_TARGET_CONFIG_PERMISSION_ID = + keccak256("SET_TARGET_CONFIG_PERMISSION"); + + /// @notice The ID of the permission required to call the `_authorizeUpgrade` function. + bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); + /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); } + /// @notice This ensures that the initialize function cannot be called during the upgrade process. + modifier onlyCallAtInitialization() { + if (_getInitializedVersion() != 0) { + revert AlreadyInitialized(); + } + + _; + } + /// @inheritdoc IPlugin function pluginType() public pure override returns (PluginType) { return PluginType.UUPS; } - /// @notice The ID of the permission required to call the `_authorizeUpgrade` function. - bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION"); + /// @notice Returns the currently set target contract. + /// @return TargetConfig The currently set target. + function getCurrentTargetConfig() public view virtual returns (TargetConfig memory) { + return currentTargetConfig; + } + + /// @notice A convenient function to get current target config only if its target is not address(0), otherwise dao(). + /// @return TargetConfig The current target config if its target is not address(0), otherwise returns dao()." + function getTargetConfig() public view virtual returns (TargetConfig memory) { + TargetConfig memory targetConfig = currentTargetConfig; + + if (targetConfig.target == address(0)) { + targetConfig = TargetConfig({target: address(dao()), operation: Operation.Call}); + } + + return targetConfig; + } /// @notice Initializes the plugin by storing the associated DAO. /// @param _dao The DAO contract. @@ -46,6 +106,14 @@ abstract contract PluginUUPSUpgradeable is __DaoAuthorizableUpgradeable_init(_dao); } + /// @dev Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function setTargetConfig( + TargetConfig calldata _targetConfig + ) public auth(SET_TARGET_CONFIG_PERMISSION_ID) { + _setTargetConfig(_targetConfig); + } + /// @notice Checks if an interface is supported by this or its parent contract. /// @param _interfaceId The ID of the interface. /// @return Returns `true` if the interface is supported. @@ -54,6 +122,10 @@ abstract contract PluginUUPSUpgradeable is _interfaceId == type(IPlugin).interfaceId || _interfaceId == type(IProtocolVersion).interfaceId || _interfaceId == type(IERC1822ProxiableUpgradeable).interfaceId || + _interfaceId == + this.setTargetConfig.selector ^ + this.getTargetConfig.selector ^ + this.getCurrentTargetConfig.selector || super.supportsInterface(_interfaceId); } @@ -63,6 +135,88 @@ abstract contract PluginUUPSUpgradeable is return _getImplementation(); } + /// @notice Sets the target to a new target (`newTarget`). + /// @param _targetConfig The target Config containing the address and operation type. + function _setTargetConfig(TargetConfig memory _targetConfig) internal virtual { + // safety check to avoid setting dao as `target` with `delegatecall` operation + // as this would not work and cause the plugin to be bricked. + if ( + _targetConfig.target.supportsInterface(type(IDAO).interfaceId) && + _targetConfig.operation == Operation.DelegateCall + ) { + revert InvalidTargetConfig(_targetConfig); + } + + currentTargetConfig = _targetConfig; + + emit TargetSet(_targetConfig); + } + + /// @notice Forwards the actions to the currently set `target` for the execution. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + return + _execute( + currentTargetConfig.target, + _callId, + _actions, + _allowFailureMap, + currentTargetConfig.operation + ); + } + + /// @notice Forwards the actions to the `target` for the execution. + /// @param _target Forwards the actions to the specific target. + /// @param _callId Identifier for this execution. + /// @param _actions actions that will be eventually called. + /// @param _allowFailureMap Bitmap-encoded number. TODO: + /// @return execResults address of the implementation contract. + /// @return failureMap address of the implementation contract. + function _execute( + address _target, + bytes32 _callId, + Action[] memory _actions, + uint256 _allowFailureMap, + Operation _op + ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { + if (_op == Operation.DelegateCall) { + bool success; + bytes memory data; + + // solhint-disable-next-line avoid-low-level-calls + (success, data) = _target.delegatecall( + abi.encodeCall(IExecutor.execute, (_callId, _actions, _allowFailureMap)) + ); + + if (!success) { + if (data.length > 0) { + // solhint-disable-next-line no-inline-assembly + assembly { + let returndata_size := mload(data) + revert(add(32, data), returndata_size) + } + } else { + revert ExecuteFailed(); + } + } + (execResults, failureMap) = abi.decode(data, (bytes[], uint256)); + } else { + (execResults, failureMap) = IExecutor(_target).execute( + _callId, + _actions, + _allowFailureMap + ); + } + } + /// @notice Internal method authorizing the upgrade of the contract via the [upgradeability mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)). /// @dev The caller must have the `UPGRADE_PLUGIN_PERMISSION_ID` permission. function _authorizeUpgrade( @@ -78,5 +232,5 @@ abstract contract PluginUUPSUpgradeable is } /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). - uint256[50] private __gap; + uint256[49] private __gap; } diff --git a/contracts/src/plugin/extensions/proposal/IProposal.sol b/contracts/src/plugin/extensions/proposal/IProposal.sol index 889583a2..5c325366 100644 --- a/contracts/src/plugin/extensions/proposal/IProposal.sol +++ b/contracts/src/plugin/extensions/proposal/IProposal.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.8; import {IDAO} from "../../../dao/IDAO.sol"; +import {IExecutor, Action} from "../../../executors/IExecutor.sol"; /// @title IProposal /// @author Aragon X - 2022-2023 @@ -23,7 +24,7 @@ interface IProposal { uint64 startDate, uint64 endDate, bytes metadata, - IDAO.Action[] actions, + Action[] actions, uint256 allowFailureMap ); @@ -31,7 +32,43 @@ interface IProposal { /// @param proposalId The ID of the proposal. event ProposalExecuted(uint256 indexed proposalId); + /// @notice Creates a new proposal. + /// @param metadata The metadata of the proposal. + /// @param actions The actions that will be executed after the proposal passes. + /// @param startDate The start date of the proposal. + /// @param endDate The end date of the proposal. + /// @param data The additional abi-encoded data to include more necessary fields. + /// @return proposalId The id of the proposal. + function createProposal( + bytes memory metadata, + Action[] memory actions, + uint64 startDate, + uint64 endDate, + bytes memory data + ) external returns (uint256 proposalId); + + /// @notice Whether proposal can be executed or not. + /// @param proposalId The id of the proposal. + /// @return bool Returns if proposal can be executed or not. + function canExecute(uint256 proposalId) external view returns (bool); + + /// @notice Creates a proposal Id. + /// @param actions The actions that will be executed after the proposal passes. + /// @param metadata The custom metadata that is passed when creating a proposal. + /// @return proposalId The id of the proposal. + function createProposalId( + Action[] memory actions, + bytes memory metadata + ) external view returns (uint256); + + /// @notice The human-readable abi format for extra params included in `data` of `createProposal`. + /// @dev Used for UI to easily detect what extra params the contract expects. + /// @return abi ABI of params in `data` of `createProposal`. + function createProposalParamsABI() external view returns (string memory abi); + /// @notice Returns the proposal count determining the next proposal ID. + /// @dev This function has been deprecated but due to backwards compatibility, it still stays in the interface + /// but returns maximum value of uint256 to let consumers know not to depend on it anymore. /// @return The proposal count. function proposalCount() external view returns (uint256); } diff --git a/contracts/src/plugin/extensions/proposal/Proposal.sol b/contracts/src/plugin/extensions/proposal/Proposal.sol index 6df0a6a2..13dbd18c 100644 --- a/contracts/src/plugin/extensions/proposal/Proposal.sol +++ b/contracts/src/plugin/extensions/proposal/Proposal.sol @@ -2,10 +2,8 @@ pragma solidity ^0.8.8; -import {Counters} from "@openzeppelin/contracts/utils/Counters.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; -import {IDAO} from "../../../dao/IDAO.sol"; import {IProposal} from "./IProposal.sol"; /// @title Proposal @@ -13,71 +11,26 @@ import {IProposal} from "./IProposal.sol"; /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by non-upgradeable DAO plugins. /// @custom:security-contact sirt@aragon.org abstract contract Proposal is IProposal, ERC165 { - using Counters for Counters.Counter; - - /// @notice The incremental ID for proposals and executions. - Counters.Counter private proposalCounter; - /// @inheritdoc IProposal - function proposalCount() public view override returns (uint256) { - return proposalCounter.current(); + function proposalCount() public pure override returns (uint256) { + return type(uint256).max; } /// @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(IProposal).interfaceId || super.supportsInterface(_interfaceId); - } - - /// @notice Creates a proposal ID. - /// @return proposalId The proposal ID. - function _createProposalId() internal returns (uint256 proposalId) { - proposalId = proposalCount(); - proposalCounter.increment(); - } - - /// @notice Internal function to create a proposal. - /// @param _metadata The proposal metadata. - /// @param _startDate The start date of the proposal in seconds. - /// @param _endDate The end date of the proposal in seconds. - /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. - /// @param _actions The actions that will be executed after the proposal passes. - /// @return proposalId The ID of the proposal. - function _createProposal( - address _creator, - bytes calldata _metadata, - uint64 _startDate, - uint64 _endDate, - IDAO.Action[] calldata _actions, - uint256 _allowFailureMap - ) internal virtual returns (uint256 proposalId) { - proposalId = _createProposalId(); - - emit ProposalCreated({ - proposalId: proposalId, - creator: _creator, - metadata: _metadata, - startDate: _startDate, - endDate: _endDate, - actions: _actions, - allowFailureMap: _allowFailureMap - }); - } - - /// @notice Internal function to execute a proposal. - /// @param _proposalId The ID of the proposal to be executed. - /// @param _actions The array of actions to be executed. - /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. - /// @return execResults The array with the results of the executed actions. - /// @return failureMap The failure map encoding which actions have failed. - function _executeProposal( - IDAO _dao, - uint256 _proposalId, - IDAO.Action[] memory _actions, - uint256 _allowFailureMap - ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { - (execResults, failureMap) = _dao.execute(bytes32(_proposalId), _actions, _allowFailureMap); - emit ProposalExecuted({proposalId: _proposalId}); + // In addition to the current interfaceId, also support previous version of the interfaceId + // that did not include the following functions: + // `createProposal, canExecute, createProposalId, createProposalParamsABI`. + return + _interfaceId == + type(IProposal).interfaceId ^ + IProposal.createProposal.selector ^ + IProposal.canExecute.selector ^ + IProposal.createProposalId.selector ^ + IProposal.createProposalParamsABI.selector || + _interfaceId == type(IProposal).interfaceId || + super.supportsInterface(_interfaceId); } } diff --git a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol index 864ace63..bbec294b 100644 --- a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol +++ b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.8; import {CountersUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol"; import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; -import {IDAO} from "../../../dao/IDAO.sol"; import {IProposal} from "./IProposal.sol"; /// @title ProposalUpgradeable @@ -19,66 +18,26 @@ abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable { CountersUpgradeable.Counter private proposalCounter; /// @inheritdoc IProposal - function proposalCount() public view override returns (uint256) { - return proposalCounter.current(); + function proposalCount() public pure override returns (uint256) { + return type(uint256).max; } /// @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(IProposal).interfaceId || super.supportsInterface(_interfaceId); - } - - /// @notice Creates a proposal ID. - /// @return proposalId The proposal ID. - function _createProposalId() internal returns (uint256 proposalId) { - proposalId = proposalCount(); - proposalCounter.increment(); - } - - /// @notice Internal function to create a proposal. - /// @param _metadata The proposal metadata. - /// @param _startDate The start date of the proposal in seconds. - /// @param _endDate The end date of the proposal in seconds. - /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. - /// @param _actions The actions that will be executed after the proposal passes. - /// @return proposalId The ID of the proposal. - function _createProposal( - address _creator, - bytes calldata _metadata, - uint64 _startDate, - uint64 _endDate, - IDAO.Action[] calldata _actions, - uint256 _allowFailureMap - ) internal virtual returns (uint256 proposalId) { - proposalId = _createProposalId(); - - emit ProposalCreated({ - proposalId: proposalId, - creator: _creator, - metadata: _metadata, - startDate: _startDate, - endDate: _endDate, - actions: _actions, - allowFailureMap: _allowFailureMap - }); - } - - /// @notice Internal function to execute a proposal. - /// @param _proposalId The ID of the proposal to be executed. - /// @param _actions The array of actions to be executed. - /// @param _allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. - /// @return execResults The array with the results of the executed actions. - /// @return failureMap The failure map encoding which actions have failed. - function _executeProposal( - IDAO _dao, - uint256 _proposalId, - IDAO.Action[] memory _actions, - uint256 _allowFailureMap - ) internal virtual returns (bytes[] memory execResults, uint256 failureMap) { - (execResults, failureMap) = _dao.execute(bytes32(_proposalId), _actions, _allowFailureMap); - emit ProposalExecuted({proposalId: _proposalId}); + // In addition to the current interfaceId, also support previous version of the interfaceId + // that did not include the following functions: + // `createProposal, canExecute, createProposalId, createProposalParamsABI`. + return + _interfaceId == + type(IProposal).interfaceId ^ + IProposal.createProposal.selector ^ + IProposal.canExecute.selector ^ + IProposal.createProposalId.selector ^ + IProposal.createProposalParamsABI.selector || + _interfaceId == type(IProposal).interfaceId || + super.supportsInterface(_interfaceId); } /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). diff --git a/contracts/test/dao/dao.ts b/contracts/test/dao/dao.ts deleted file mode 100644 index a6c1af75..00000000 --- a/contracts/test/dao/dao.ts +++ /dev/null @@ -1,12 +0,0 @@ -import {IDAO__factory} from '../../typechain'; -import {getInterfaceId} from '@aragon/osx-commons-sdk'; -import {IDAO__factory as IDAO_V1_0_0__factory} from '@aragon/osx-ethers-v1.0.0'; -import {expect} from 'chai'; - -describe('IDAO', function () { - it('has the same interface ID as its initial version introduced in v1.0.0', async () => { - const current = getInterfaceId(IDAO__factory.createInterface()); - const initial = getInterfaceId(IDAO_V1_0_0__factory.createInterface()); - expect(current).to.equal(initial); - }); -}); diff --git a/contracts/test/executors/executor.ts b/contracts/test/executors/executor.ts new file mode 100644 index 00000000..1fa95c05 --- /dev/null +++ b/contracts/test/executors/executor.ts @@ -0,0 +1,268 @@ +import { + ActionExecute__factory, + Executor, + Executor__factory, + GasConsumer__factory, +} from '../../typechain'; +import {ExecutedEvent} from '../../typechain/src/executors/Executor'; +import {findEvent, flipBit} from '@aragon/osx-commons-sdk'; +import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; +import {expect} from 'chai'; +import {ethers} from 'hardhat'; + +export const ZERO_BYTES32 = + '0x0000000000000000000000000000000000000000000000000000000000000000'; + +const MAX_ACTIONS = 256; +const errorSignature = '0x08c379a0'; // first 4 bytes of Error(string) + +const EventExecuted = 'Executed'; + +export async function getActions() { + const signers = await ethers.getSigners(); + const ActionExecuteFactory = new ActionExecute__factory(signers[0]); + let ActionExecute = await ActionExecuteFactory.deploy(); + const iface = new ethers.utils.Interface(ActionExecute__factory.abi); + + const num = 20; + return { + failAction: { + to: ActionExecute.address, + data: iface.encodeFunctionData('fail'), + value: 0, + }, + succeedAction: { + to: ActionExecute.address, + data: iface.encodeFunctionData('setTest', [num]), + value: 0, + }, + reentrancyAction: { + to: ActionExecute.address, + data: iface.encodeFunctionData('callBackCaller'), + value: 0, + }, + failActionMessage: ethers.utils + .hexlify(ethers.utils.toUtf8Bytes('ActionExecute:Revert')) + .substring(2), + successActionResult: ethers.utils.hexZeroPad(ethers.utils.hexlify(num), 32), + }; +} + +describe('Executor', async () => { + let data: any; + let executor: Executor; + let ownerAddress: string; + let signers: SignerWithAddress[]; + + before(async () => { + data = await getActions(); + signers = await ethers.getSigners(); + ownerAddress = await signers[0].getAddress(); + + executor = await new Executor__factory(signers[0]).deploy(); + }); + + it('reverts if array of actions is too big', async () => { + let actions = []; + for (let i = 0; i < MAX_ACTIONS; i++) { + actions[i] = data.succeedAction; + } + + await expect(executor.execute(ZERO_BYTES32, actions, 0)).not.to.be.reverted; + + // add one more to make sure it fails + actions[MAX_ACTIONS] = data.failAction; + + await expect( + executor.execute(ZERO_BYTES32, actions, 0) + ).to.be.revertedWithCustomError(executor, 'TooManyActions'); + }); + + it("reverts if action is failable and allowFailureMap doesn't include it", async () => { + await expect(executor.execute(ZERO_BYTES32, [data.failAction], 0)) + .to.be.revertedWithCustomError(executor, 'ActionFailed') + .withArgs(0); + }); + + it('reverts if one of the action called back the Executor with reentrancy', async () => { + // Allow the call to fail so we can get the error message + // in the `execResults`, otherwise with allowFailureMap = 0, + // it fails with `ActionFailed` even though reentrancy worked correctly. + let allowFailureMap = flipBit(0, ethers.BigNumber.from(0)); + + let tx = await executor.execute( + ZERO_BYTES32, + [data.reentrancyAction], + allowFailureMap + ); + const event = findEvent(await tx.wait(), EventExecuted); + const execResult = event.args.execResults[0]; + + const reentrancyErrorSignature = + Executor__factory.createInterface().encodeErrorResult('ReentrantCall'); + + expect(reentrancyErrorSignature).to.equal(execResult); + + // If called with allowFailureMap = 0, it must fail with `ActionFailed`. + await expect( + executor.execute(ZERO_BYTES32, [data.reentrancyAction], 0) + ).to.be.revertedWithCustomError(executor, 'ActionFailed'); + }); + + it('succeeds if action is failable but allowFailureMap allows it', async () => { + let num = ethers.BigNumber.from(0); + num = flipBit(0, num); + + const tx = await executor.execute(ZERO_BYTES32, [data.failAction], num); + const event = findEvent(await tx.wait(), EventExecuted); + + // Check that failAction's revertMessage was correctly stored in the executor's execResults + expect(event.args.execResults[0]).to.includes(data.failActionMessage); + expect(event.args.execResults[0]).to.includes(errorSignature); + }); + + it('returns the correct result if action succeeds', async () => { + const tx = await executor.execute(ZERO_BYTES32, [data.succeedAction], 0); + const event = findEvent(await tx.wait(), EventExecuted); + expect(event.args.execResults[0]).to.equal(data.successActionResult); + }); + + it('succeeds and correctly constructs failureMap results ', async () => { + let allowFailureMap = ethers.BigNumber.from(0); + let actions = []; + + // First 3 actions will fail + actions[0] = data.failAction; + actions[1] = data.failAction; + actions[2] = data.failAction; + + // The next 3 actions will succeed + actions[3] = data.succeedAction; + actions[4] = data.succeedAction; + actions[5] = data.succeedAction; + + // add first 3 actions in the allowFailureMap + // to make sure tx succeeds. + for (let i = 0; i < 3; i++) { + allowFailureMap = flipBit(i, allowFailureMap); + } + + // If the below call not fails, means allowFailureMap is correct. + let tx = await executor.execute(ZERO_BYTES32, actions, allowFailureMap); + let event = findEvent(await tx.wait(), EventExecuted); + + expect(event.args.actor).to.equal(ownerAddress); + expect(event.args.callId).to.equal(ZERO_BYTES32); + expect(event.args.allowFailureMap).to.equal(allowFailureMap); + + // construct the failureMap which only has those + // bits set at indexes where actions failed + let failureMap = ethers.BigNumber.from(0); + for (let i = 0; i < 3; i++) { + failureMap = flipBit(i, failureMap); + } + // Check that executor correctly generated failureMap + expect(event.args.failureMap).to.equal(failureMap); + + // Check that execResult emitted correctly stores action results. + for (let i = 0; i < 3; i++) { + expect(event.args.execResults[i]).to.includes(data.failActionMessage); + expect(event.args.execResults[i]).to.includes(errorSignature); + } + for (let i = 3; i < 6; i++) { + expect(event.args.execResults[i]).to.equal(data.successActionResult); + } + + // lets remove one of the action from allowFailureMap + // to see tx will actually revert. + allowFailureMap = flipBit(2, allowFailureMap); + await expect(executor.execute(ZERO_BYTES32, actions, allowFailureMap)) + .to.be.revertedWithCustomError(executor, 'ActionFailed') + .withArgs(2); // Since we unset the 2th action from failureMap, it should fail with that index. + }); + + it('emits an event afterwards', async () => { + const tx = await executor.execute(ZERO_BYTES32, [data.succeedAction], 0); + const rc = await tx.wait(); + + const event = findEvent(rc, 'Executed'); + expect(event.args.actor).to.equal(ownerAddress); + expect(event.args.callId).to.equal(ZERO_BYTES32); + expect(event.args.actions.length).to.equal(1); + expect(event.args.actions[0].to).to.equal(data.succeedAction.to); + expect(event.args.actions[0].value).to.equal(data.succeedAction.value); + expect(event.args.actions[0].data).to.equal(data.succeedAction.data); + expect(event.args.execResults[0]).to.equal(data.successActionResult); + expect(event.args.allowFailureMap).to.equal(0); + }); + + it('reverts if failure is allowed but not enough gas is provided (many actions)', async () => { + const GasConsumer = new GasConsumer__factory(signers[0]); + const gasConsumer = await GasConsumer.deploy(); + + // Prepare an action array calling `consumeGas` twenty times. + const gasConsumingAction = { + to: gasConsumer.address, + data: GasConsumer.interface.encodeFunctionData('consumeGas', [20]), + value: 0, + }; + + let allowFailureMap = ethers.BigNumber.from(0); + allowFailureMap = flipBit(0, allowFailureMap); // allow the action to fail + + const expectedGas = await executor.estimateGas.execute( + ZERO_BYTES32, + [gasConsumingAction], + allowFailureMap + ); + + // Provide too little gas so that the last `to.call` fails, but the remaining gas is enough to finish the subsequent operations. + await expect( + executor.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas.sub(3200), + }) + ).to.be.revertedWithCustomError(executor, 'InsufficientGas'); + + // Provide enough gas so that the entire call passes. + await expect( + executor.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas, + }) + ).to.not.be.reverted; + }); + + it('reverts if failure is allowed but not enough gas is provided (one action)', async () => { + const GasConsumer = new GasConsumer__factory(signers[0]); + const gasConsumer = await GasConsumer.deploy(); + + // Prepare an action array calling `consumeGas` one times. + const gasConsumingAction = { + to: gasConsumer.address, + data: GasConsumer.interface.encodeFunctionData('consumeGas', [2]), + value: 0, + }; + + let allowFailureMap = ethers.BigNumber.from(0); + allowFailureMap = flipBit(0, allowFailureMap); // allow the action to fail + + const expectedGas = await executor.estimateGas.execute( + ZERO_BYTES32, + [gasConsumingAction], + allowFailureMap + ); + + // Provide too little gas so that the last `to.call` fails, but the remaining gas is enough to finish the subsequent operations. + await expect( + executor.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas.sub(10000), + }) + ).to.be.revertedWithCustomError(executor, 'InsufficientGas'); + + // Provide enough gas so that the entire call passes. + await expect( + executor.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas, + }) + ).to.not.be.reverted; + }); +}); diff --git a/contracts/test/plugin/extensions/proposal.ts b/contracts/test/plugin/extensions/proposal.ts index d43d2bb1..4bea8272 100644 --- a/contracts/test/plugin/extensions/proposal.ts +++ b/contracts/test/plugin/extensions/proposal.ts @@ -1,40 +1,19 @@ import { DAOMock, DAOMock__factory, - IDAO, - IDAO__factory, IProposal__factory, ProposalMock, ProposalUpgradeableMock, ProposalMock__factory, ProposalUpgradeableMock__factory, } from '../../../typechain'; -import {ExecutedEvent} from '../../../typechain/src/dao/IDAO'; import {erc165ComplianceTests} from '../../helpers'; -import {findEventTopicLog, getInterfaceId} from '@aragon/osx-commons-sdk'; +import {getInterfaceId} from '@aragon/osx-commons-sdk'; import {IProposal__factory as IProposal_V1_0_0__factory} from '@aragon/osx-ethers-v1.0.0'; -import {loadFixture, time} from '@nomicfoundation/hardhat-network-helpers'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; +import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; import {expect} from 'chai'; -import {BigNumberish, BytesLike} from 'ethers'; import {ethers} from 'hardhat'; -export type ProposalData = { - metadata: BytesLike; - startDate: BigNumberish; - endDate: BigNumberish; - actions: IDAO.ActionStruct[]; - allowFailureMap: BigNumberish; -}; - -describe('IProposal', function () { - it('has the same interface ID as its initial version introduced in v1.0.0', async () => { - const current = getInterfaceId(IProposal__factory.createInterface()); - const initial = getInterfaceId(IProposal_V1_0_0__factory.createInterface()); - expect(current).to.equal(initial); - }); -}); - describe('Proposal', async () => { proposalBaseTests(proposalFixture); }); @@ -45,231 +24,65 @@ describe('ProposalUpgradeable', async () => { // Contains tests for functionality common for `ProposalMock` and `ProposalUpgradeableMock` to avoid duplication. function proposalBaseTests(fixture: () => Promise) { - it('counts proposals', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - expect(await proposalMock.proposalCount()).to.equal(0); - - const creator = bob; - - await proposalMock - .connect(alice) - .createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(await proposalMock.proposalCount()).to.equal(1); - }); - - it('creates proposalIds', async () => { + it('returns max value of integer for backwards-compatibility', async () => { const {proposalMock} = await loadFixture(fixture); - const expectedProposalId = 0; - const proposalId = await proposalMock.callStatic.createProposalId(); - - expect(proposalId).to.equal(expectedProposalId); - }); - - it('creates proposals', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - const expectedProposalId = 0; - const creator = bob; - - const proposalId = await proposalMock - .connect(alice) - .callStatic.createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(proposalId).to.equal(expectedProposalId); - }); - - it('emits the `ProposalCreated` event', async () => { - const {alice, bob, proposalMock, exampleData} = await loadFixture(fixture); - - const expectedProposalId = 0; - const creator = bob; - - await expect( - proposalMock - .connect(alice) - .createProposal( - creator.address, - exampleData.metadata, - exampleData.startDate, - exampleData.endDate, - exampleData.actions, - exampleData.allowFailureMap - ) - ) - .to.emit(proposalMock, 'ProposalCreated') - .withArgs( - expectedProposalId, - creator.address, - exampleData.startDate, - exampleData.endDate, - exampleData.metadata, - exampleData.actions, - exampleData.allowFailureMap - ); - }); - it('executes proposals', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture + expect(await proposalMock.proposalCount()).to.equal( + ethers.constants.MaxUint256 ); - - const expectedExecResults: string[] = []; - const expectedFailureMap: BigNumberish = 0; - - const proposalId = 0; - const [execResults, failureMap] = await proposalMock - .connect(alice) - .callStatic.executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ); - - expect(execResults).to.deep.equal(expectedExecResults); - expect(failureMap).to.equal(expectedFailureMap); - }); - - it('emits the `ProposalExecuted` event', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture - ); - - const proposalId = 0; - - await expect( - proposalMock - .connect(alice) - .executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ) - ) - .to.emit(proposalMock, 'ProposalExecuted') - .withArgs(proposalId); - }); - - it('emits the `Executed` event on the executing DAO', async () => { - const {alice, daoMock, proposalMock, exampleData} = await loadFixture( - fixture - ); - - const proposalId = 0; - const proposalIdAsBytes32 = ethers.utils.hexZeroPad( - ethers.utils.hexlify(proposalId), - 32 - ); - - const expectedActor = proposalMock.address; - const expectedExecResults: string[] = []; - const expectedFailureMap: BigNumberish = 0; - - const tx = await proposalMock - .connect(alice) - .executeProposal( - daoMock.address, - proposalId, - exampleData.actions, - exampleData.allowFailureMap - ); - const event = findEventTopicLog( - await tx.wait(), - IDAO__factory.createInterface(), - 'Executed' - ); - - expect(event.args.actor).to.equal(expectedActor); - expect(event.args.callId).to.equal(proposalIdAsBytes32); - expect(event.args.actions).to.deep.equal(exampleData.actions); - expect(event.args.allowFailureMap).to.equal(exampleData.allowFailureMap); - expect(event.args.failureMap).to.equal(expectedFailureMap); - expect(event.args.execResults).to.deep.equal(expectedExecResults); }); describe('ERC-165', async () => { it('supports the `ERC-165` standard', async () => { - const {alice, proposalMock} = await loadFixture(fixture); - await erc165ComplianceTests(proposalMock, alice); + const {proposalMock} = await loadFixture(fixture); + const signers = await ethers.getSigners(); + await erc165ComplianceTests(proposalMock, signers[0]); }); - it('supports the `IProposal` interface', async () => { + it('supports the `IProposal` interface for the new as well as old versions', async () => { const {proposalMock} = await loadFixture(fixture); - const iface = IProposal__factory.createInterface(); - expect(await proposalMock.supportsInterface(getInterfaceId(iface))).to.be - .true; + const newIface = IProposal__factory.createInterface(); + expect(await proposalMock.supportsInterface(getInterfaceId(newIface))).to + .be.true; + + const oldIface = IProposal_V1_0_0__factory.createInterface(); + expect(await proposalMock.supportsInterface(getInterfaceId(oldIface))).to + .be.true; }); }); } type BaseFixtureResult = { - alice: SignerWithAddress; - bob: SignerWithAddress; daoMock: DAOMock; - exampleData: ProposalData; }; async function baseFixture(): Promise { - const [alice, bob] = await ethers.getSigners(); - const daoMock = await new DAOMock__factory(alice).deploy(); - - const uri = ethers.utils.hexlify( - ethers.utils.toUtf8Bytes( - 'ipfs://QmTMcfhxgYA3ziwpnhEg1K3aFn7ixMH9dBxWXs5YTJdZwR' - ) - ); - const current = await time.latest(); + const signers = await ethers.getSigners(); + const daoMock = await new DAOMock__factory(signers[0]).deploy(); - const exampleData = { - metadata: uri, - startDate: current, - endDate: current + 12, - actions: [], - allowFailureMap: 0, - }; - - return {alice, bob, daoMock, exampleData}; + return {daoMock}; } type FixtureResult = { proposalMock: ProposalMock | ProposalUpgradeableMock; - alice: SignerWithAddress; - bob: SignerWithAddress; daoMock: DAOMock; - exampleData: ProposalData; }; async function proposalFixture(): Promise { - const {alice, bob, daoMock, exampleData} = await baseFixture(); - - const proposalMock = await new ProposalMock__factory(alice).deploy(); + const {daoMock} = await baseFixture(); + const signers = await ethers.getSigners(); + const proposalMock = await new ProposalMock__factory(signers[0]).deploy(); - return {alice, bob, proposalMock, daoMock, exampleData}; + return {proposalMock, daoMock}; } async function proposalUpgradeableFixture(): Promise { - const {alice, bob, daoMock, exampleData} = await baseFixture(); + const {daoMock} = await baseFixture(); + const signers = await ethers.getSigners(); const proposalMock = await new ProposalUpgradeableMock__factory( - alice + signers[0] ).deploy(); - return {alice, bob, proposalMock, daoMock, exampleData}; + return {proposalMock, daoMock}; } diff --git a/contracts/test/plugin/plugin-clonable.ts b/contracts/test/plugin/plugin-clonable.ts index 4166216e..6914d46a 100644 --- a/contracts/test/plugin/plugin-clonable.ts +++ b/contracts/test/plugin/plugin-clonable.ts @@ -1,4 +1,6 @@ import { + CustomExecutorMock, + CustomExecutorMock__factory, DAOMock, DAOMock__factory, IPlugin__factory, @@ -20,6 +22,11 @@ import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {ethers} from 'hardhat'; +enum Operation { + call, + delegatecall, +} + describe('PluginCloneable', function () { describe('Initializable', async () => { it('initialize', async () => { @@ -112,6 +119,20 @@ describe('PluginCloneable', function () { expect(await implementation.supportsInterface(getInterfaceId(iface))).to .be.true; }); + + it('supports the `setTarget^getTarget` interface', async () => { + const {implementation} = await loadFixture(fixture); + const iface = PluginCloneableMockBuild1__factory.createInterface(); + + const interfaceId = ethers.BigNumber.from( + iface.getSighash('setTargetConfig') + ) + .xor(ethers.BigNumber.from(iface.getSighash('getTargetConfig'))) + .xor(ethers.BigNumber.from(iface.getSighash('getCurrentTargetConfig'))) + .toHexString(); + + expect(await implementation.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -122,12 +143,278 @@ describe('PluginCloneable', function () { ); }); }); + + describe('setTargetConfig/getTargetConfig/getCurrentTargetConfig', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, proxy, daoMock} = await loadFixture(fixture); + + const newTarget = proxy.address; + + await expect(proxy.setTargetConfig({target: newTarget, operation: 0})) + .to.be.revertedWithCustomError(proxy, 'DaoUnauthorized') + .withArgs( + daoMock.address, + proxy.address, + deployer.address, + ethers.utils.id('SET_TARGET_CONFIG_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + const newTarget = proxy.address; + + const targetConfig = {target: newTarget, operation: 0}; + + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + await expect(proxy.setTargetConfig(targetConfig)) + .to.emit(proxy, 'TargetSet') + .withArgs((val: any) => + expect(val).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]) + ); + + expect(await proxy.getTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + }); + + it('returns default target config if no target config is set', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + const dao = await proxy.dao(); + + // Current Config must return zeroes. + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + // Since no target is set, it must return default - i.e dao target. + expect(await proxy.getTargetConfig()).to.deep.equal([dao, 0]); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + const newTargetConfig = {target: proxy.address, operation: 1}; + + await proxy.setTargetConfig(newTargetConfig); + + // Current config must return the newly set one. + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // new config was set, so it must return the new one. + expect(await proxy.getTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // set the zero target which should then return default again - i.e dao. + await proxy.setTargetConfig({ + target: ethers.constants.AddressZero, + operation: 0, + }); + + expect(await proxy.getTargetConfig()).to.deep.equal([dao, 0]); + }); + }); + + describe('Executions', async () => { + let executor: CustomExecutorMock; + let proxy: PluginCloneableMockBuild1; + let daoMock: DAOMock; + let mergedABI: any; + before(async () => { + const [deployer] = await ethers.getSigners(); + + const executorFactory = new CustomExecutorMock__factory(deployer); + executor = await executorFactory.deploy(); + + const abiA = CustomExecutorMock__factory.abi; + const abiB = PluginCloneableMockBuild1__factory.abi; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + mergedABI = abiA.concat(abiB); + }); + + beforeEach(async () => { + const data = await fixture(); + const [deployer] = await ethers.getSigners(); + + proxy = new ethers.Contract( + data.proxy.address, + mergedABI, + deployer + ) as PluginCloneableMockBuild1; + daoMock = data.daoMock; + + await daoMock.setHasPermissionReturnValueMock(true); + }); + + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and reverts from target', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and emits event from target', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(proxy, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(proxy, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 123, + [], + 0 + ) + ).to.be.revertedWithCustomError(proxy, 'ExecuteFailed'); + }); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](ethers.constants.AddressZero, 1, [], 0, 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and emits event from target', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, 0) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and reverts from target', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, 0) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(proxy, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, Operation.delegatecall) + ).to.emit(proxy, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 123, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(proxy, 'ExecuteFailed'); + }); + }); + }); + }); }); type FixtureResult = { deployer: SignerWithAddress; implementation: PluginCloneableMockBuild1; proxyFactory: ProxyFactory; + proxy: PluginCloneableMockBuild1; daoMock: DAOMock; initCalldata: string; Build1Factory: PluginCloneableMockBuild1__factory; @@ -150,10 +437,15 @@ async function fixture(): Promise { [daoMock.address] ); + const tx = await proxyFactory.deployUUPSProxy(initCalldata); + const event = findEvent(await tx.wait(), 'ProxyCreated'); + const proxy = Build1Factory.attach(event.args.proxy); + return { deployer, implementation, proxyFactory, + proxy, daoMock, initCalldata, Build1Factory, diff --git a/contracts/test/plugin/plugin-uups-upgradeable.ts b/contracts/test/plugin/plugin-uups-upgradeable.ts index 03501e8a..36c70d04 100644 --- a/contracts/test/plugin/plugin-uups-upgradeable.ts +++ b/contracts/test/plugin/plugin-uups-upgradeable.ts @@ -9,6 +9,8 @@ import { PluginUUPSUpgradeableMockBuild2__factory, PluginUUPSUpgradeableMockBad__factory, ProxyFactory__factory, + CustomExecutorMock__factory, + CustomExecutorMock, } from '../../typechain'; import { ProxyCreatedEvent, @@ -27,6 +29,11 @@ import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {ethers} from 'hardhat'; +enum Operation { + call, + delegatecall, +} + describe('PluginUUPSUpgradeable', function () { describe('Initializable', async () => { it('initialize', async () => { @@ -125,6 +132,20 @@ describe('PluginUUPSUpgradeable', function () { expect(await implementation.supportsInterface(getInterfaceId(iface))).to .be.true; }); + + it('supports the `setTargetConfig^getTargetConfig` interface', async () => { + const {implementation} = await loadFixture(fixture); + const iface = PluginUUPSUpgradeableMockBuild1__factory.createInterface(); + + const interfaceId = ethers.BigNumber.from( + iface.getSighash('setTargetConfig') + ) + .xor(ethers.BigNumber.from(iface.getSighash('getTargetConfig'))) + .xor(ethers.BigNumber.from(iface.getSighash('getCurrentTargetConfig'))) + .toHexString(); + + expect(await implementation.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -136,6 +157,271 @@ describe('PluginUUPSUpgradeable', function () { }); }); + describe('setTargetConfig/getTargetConfig/getCurrentTargetConfig', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, proxy, daoMock} = await loadFixture(fixture); + + const newTarget = proxy.address; + + await expect(proxy.setTargetConfig({target: newTarget, operation: 0})) + .to.be.revertedWithCustomError(proxy, 'DaoUnauthorized') + .withArgs( + daoMock.address, + proxy.address, + deployer.address, + ethers.utils.id('SET_TARGET_CONFIG_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + const newTarget = proxy.address; + + const targetConfig = {target: newTarget, operation: 0}; + + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + await expect(proxy.setTargetConfig(targetConfig)) + .to.emit(proxy, 'TargetSet') + .withArgs((val: any) => + expect(val).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]) + ); + + expect(await proxy.getTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + }); + + it('returns default target config if no target config is set', async () => { + const {proxy, daoMock} = await loadFixture(fixture); + + const dao = await proxy.dao(); + + // Current Config must return zeroes. + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + // Since no target is set, it must return default - i.e dao target. + expect(await proxy.getTargetConfig()).to.deep.equal([dao, 0]); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + const newTargetConfig = {target: proxy.address, operation: 1}; + + await proxy.setTargetConfig(newTargetConfig); + + // Current config must return the newly set one. + expect(await proxy.getCurrentTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // new config was set, so it must return the new one. + expect(await proxy.getTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // set the zero target which should then return default again - i.e dao. + await proxy.setTargetConfig({ + target: ethers.constants.AddressZero, + operation: 0, + }); + + expect(await proxy.getTargetConfig()).to.deep.equal([dao, 0]); + }); + }); + + describe('Executions', async () => { + let executor: CustomExecutorMock; + let proxy: PluginUUPSUpgradeableMockBuild1; + let daoMock: DAOMock; + let mergedABI: any; + before(async () => { + const [deployer] = await ethers.getSigners(); + + const executorFactory = new CustomExecutorMock__factory(deployer); + executor = await executorFactory.deploy(); + + const abiA = CustomExecutorMock__factory.abi; + const abiB = PluginUUPSUpgradeableMockBuild1__factory.abi; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + mergedABI = abiA.concat(abiB); + }); + + beforeEach(async () => { + const data = await fixture(); + const [deployer] = await ethers.getSigners(); + + proxy = new ethers.Contract( + data.proxy.address, + mergedABI, + deployer + ) as PluginUUPSUpgradeableMockBuild1; + daoMock = data.daoMock; + + await daoMock.setHasPermissionReturnValueMock(true); + }); + + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and reverts from target', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and emits event from target', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(proxy, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(proxy, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await proxy.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + proxy['execute(uint256,(address,uint256,bytes)[],uint256)']( + 123, + [], + 0 + ) + ).to.be.revertedWithCustomError(proxy, 'ExecuteFailed'); + }); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](ethers.constants.AddressZero, 1, [], 0, 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and emits event from target', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, 0) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and reverts from target', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, 0) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(proxy, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, Operation.delegatecall) + ).to.emit(proxy, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await expect( + proxy[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 123, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(proxy, 'ExecuteFailed'); + }); + }); + }); + }); + describe('Upgradeability', async () => { it('returns the implementation', async () => { const {proxyFactory, Build1Factory, implementation} = await loadFixture( @@ -273,6 +559,7 @@ type FixtureResult = { deployer: SignerWithAddress; implementation: PluginUUPSUpgradeableMockBuild1; proxyFactory: ProxyFactory; + proxy: PluginUUPSUpgradeableMockBuild1; daoMock: DAOMock; initCalldata: string; Build1Factory: PluginUUPSUpgradeableMockBuild1__factory; @@ -297,10 +584,15 @@ async function fixture(): Promise { [daoMock.address] ); + const tx = await proxyFactory.deployUUPSProxy(initCalldata); + const event = findEvent(await tx.wait(), 'ProxyCreated'); + const proxy = Build1Factory.attach(event.args.proxy); + return { deployer, implementation, proxyFactory, + proxy, initCalldata, daoMock, Build1Factory, diff --git a/contracts/test/plugin/plugin.ts b/contracts/test/plugin/plugin.ts index d5d8342f..1de5f99d 100644 --- a/contracts/test/plugin/plugin.ts +++ b/contracts/test/plugin/plugin.ts @@ -1,4 +1,6 @@ import { + CustomExecutorMock, + CustomExecutorMock__factory, DAOMock, DAOMock__factory, IPlugin__factory, @@ -21,6 +23,11 @@ type FixtureResult = { Build1Factory: PluginMockBuild1__factory; }; +enum Operation { + call, + delegatecall, +} + describe('Plugin', function () { describe('PluginType', async () => { it('returns the current protocol version', async () => { @@ -55,6 +62,20 @@ describe('Plugin', function () { const iface = IProtocolVersion__factory.createInterface(); expect(await plugin.supportsInterface(getInterfaceId(iface))).to.be.true; }); + + it('supports the `setTarget^getTarget` interface', async () => { + const {plugin} = await loadFixture(fixture); + const iface = PluginMockBuild1__factory.createInterface(); + + const interfaceId = ethers.BigNumber.from( + iface.getSighash('setTargetConfig') + ) + .xor(ethers.BigNumber.from(iface.getSighash('getTargetConfig'))) + .xor(ethers.BigNumber.from(iface.getSighash('getCurrentTargetConfig'))) + .toHexString(); + + expect(await plugin.supportsInterface(interfaceId)).to.be.true; + }); }); describe('Protocol version', async () => { @@ -65,6 +86,271 @@ describe('Plugin', function () { ); }); }); + + describe('setTargetConfig/getTargetConfig/getCurrentTargetConfig', async () => { + it('reverts if caller does not have the permission', async () => { + const {deployer, plugin, daoMock} = await loadFixture(fixture); + + const newTarget = plugin.address; + + await expect(plugin.setTargetConfig({target: newTarget, operation: 0})) + .to.be.revertedWithCustomError(plugin, 'DaoUnauthorized') + .withArgs( + daoMock.address, + plugin.address, + deployer.address, + ethers.utils.id('SET_TARGET_PERMISSION') + ); + }); + + it('updates the target and emits an appropriate event', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + const newTarget = plugin.address; + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + expect(await plugin.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + const targetConfig = {target: newTarget, operation: 0}; + + await expect(plugin.setTargetConfig(targetConfig)) + .to.emit(plugin, 'TargetSet') + .withArgs((val: any) => + expect(val).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]) + ); + + expect(await plugin.getTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + + expect(await plugin.getCurrentTargetConfig()).to.deep.equal([ + targetConfig.target, + targetConfig.operation, + ]); + }); + + it('returns default target config if no target config is set', async () => { + const {plugin, daoMock} = await loadFixture(fixture); + + const dao = await plugin.dao(); + + // Current Config must return zeroes. + expect(await plugin.getCurrentTargetConfig()).to.deep.equal([ + ethers.constants.AddressZero, + 0, + ]); + + // Since no target is set, it must return default - i.e dao target. + expect(await plugin.getTargetConfig()).to.deep.equal([dao, 0]); + + // Set the `hasPermission` mock function to return `true`. + await daoMock.setHasPermissionReturnValueMock(true); // answer true for all permission requests + + const newTargetConfig = {target: plugin.address, operation: 1}; + + await plugin.setTargetConfig(newTargetConfig); + + // Current config must return the newly set one. + expect(await plugin.getCurrentTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // new config was set, so it must return the new one. + expect(await plugin.getTargetConfig()).to.deep.equal([ + newTargetConfig.target, + newTargetConfig.operation, + ]); + + // set the zero target which should then return default again - i.e dao. + await plugin.setTargetConfig({ + target: ethers.constants.AddressZero, + operation: 0, + }); + + expect(await plugin.getTargetConfig()).to.deep.equal([dao, 0]); + }); + }); + + describe('Executions', async () => { + let executor: CustomExecutorMock; + let plugin: PluginMockBuild1; + let daoMock: DAOMock; + let mergedABI: any; + before(async () => { + const [deployer] = await ethers.getSigners(); + + const executorFactory = new CustomExecutorMock__factory(deployer); + executor = await executorFactory.deploy(); + + const abiA = CustomExecutorMock__factory.abi; + const abiB = PluginMockBuild1__factory.abi; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + mergedABI = abiA.concat(abiB); + }); + + beforeEach(async () => { + const data = await fixture(); + const [deployer] = await ethers.getSigners(); + + plugin = new ethers.Contract( + data.plugin.address, + mergedABI, + deployer + ) as PluginMockBuild1; + daoMock = data.daoMock; + + await daoMock.setHasPermissionReturnValueMock(true); + }); + + describe('execute with current target', async () => { + it('reverts with ambiguity if target is not set', async () => { + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)'](1, [], 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and reverts from target', async () => { + await plugin.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and emits event from target', async () => { + await plugin.setTargetConfig({ + target: executor.address, + operation: Operation.call, + }); + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await plugin.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)']( + 0, + [], + 0 + ) + ).to.be.revertedWithCustomError(plugin, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + await plugin.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)']( + 1, + [], + 0 + ) + ).to.emit(plugin, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await plugin.setTargetConfig({ + target: executor.address, + operation: Operation.delegatecall, + }); + await expect( + plugin['execute(uint256,(address,uint256,bytes)[],uint256)']( + 123, + [], + 0 + ) + ).to.be.revertedWithCustomError(plugin, 'ExecuteFailed'); + }); + }); + }); + + describe('execute with the custom target', async () => { + it('reverts with ambiguity if target address(0) is passed', async () => { + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](ethers.constants.AddressZero, 1, [], 0, 0) + ).to.be.reverted; + }); + + describe('Execute with operation = `call`', async () => { + it('successfully forwards action to the currently set target and emits event from target', async () => { + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, 0) + ).to.be.revertedWithCustomError(executor, 'Failed'); + }); + + it('successfully forwards action to the currently set target and reverts from target', async () => { + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, 0) + ).to.emit(executor, 'Executed'); + }); + }); + + describe('Execute with operation = `delegatecall`', async () => { + it('bubbles up the revert message and reverts from the consumer', async () => { + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 0, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(plugin, 'Failed'); + }); + + it('successfully does `delegatecall` and emits the event from the consumer', async () => { + // Must emit the event from the consumer, not from the executor because of delegatecall + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 1, [], 0, Operation.delegatecall) + ).to.emit(plugin, 'Executed'); + }); + + it('reverts with `ExecuteFailed`error', async () => { + await expect( + plugin[ + 'execute(address,uint256,(address,uint256,bytes)[],uint256,uint8)' + ](executor.address, 123, [], 0, Operation.delegatecall) + ).to.be.revertedWithCustomError(plugin, 'ExecuteFailed'); + }); + }); + }); + }); }); async function fixture(): Promise {