From 758da27b553a6bc4309d71bb3ea118f089b82ef0 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Thu, 13 Jun 2024 19:25:25 +0900 Subject: [PATCH 1/6] refactor to simplify into 1 contract and interface --- README.md | 13 +- .../BridgeExecutorBase.sol => Executor.sol} | 171 +++++++++--------- src/executors/AuthBridgeExecutor.sol | 47 ----- src/interfaces/IAuthBridgeExecutor.sol | 31 ---- .../{IExecutorBase.sol => IExecutor.sol} | 29 ++- test/ArbitrumOneCrosschainTest.t.sol | 2 +- test/BaseChainCrosschainTest.t.sol | 2 +- test/CrosschainTestBase.sol | 23 ++- ...uthBridgeExecutor.t.sol => Executor.t.sol} | 50 ++--- test/GnosisCrosschainTest.t.sol | 2 +- test/OptimismCrosschainTest.t.sol | 2 +- test/mocks/ReconfigurationPayload.sol | 8 +- 12 files changed, 156 insertions(+), 224 deletions(-) rename src/{executors/BridgeExecutorBase.sol => Executor.sol} (86%) delete mode 100644 src/executors/AuthBridgeExecutor.sol delete mode 100644 src/interfaces/IAuthBridgeExecutor.sol rename src/interfaces/{IExecutorBase.sol => IExecutor.sol} (87%) rename test/{AuthBridgeExecutor.t.sol => Executor.t.sol} (94%) diff --git a/README.md b/README.md index 8ddcb63..3bcfea6 100644 --- a/README.md +++ b/README.md @@ -10,14 +10,11 @@ Executors serve as admins of the bridged domain instances of the protocol.
T ![Architecture Diagram](/diagram.png) ## 🤝 Contribution Guidelines In order to add governance relay infrastructure for a new domain, perform the following steps: -1. Go to [XChain Helpers](https://github.com/marsfoundation/xchain-helpers) repository and add: - 1. A domain helper abstracting away the process of passing messages between host domain and your bridged domain. - 2. A helper function to the XChainForwarders library abstracting away host domain interactions with the bridge. -2. Add `BridgeExecutor` to the `/src/executors` directory. Follow currently used naming convention. -3. Add a new test file for your domain to the `/test` directory. Inherit `CrosschainTestBase` and add tests specific to your domain to the test suite. All of the tests have to pass. Follow linting and naming convention used in other test files. -4. Use proper labeling for your open PR (always set adequate priority and status) -5. Get an approving review from at least one of three designated reviewers - **@hexonaut**, **@lucas-manuel** or **@barrutko** -6. Enjoy governance messages being passed through the bridge to your domain! 🎉 +1. Go to [XChain Helpers](https://github.com/marsfoundation/xchain-helpers) repository and complete the process for adding a new domain. +2. Add a new test file for your domain to the `/test` directory. Inherit `CrosschainTestBase` and add tests specific to your domain to the test suite. All of the tests have to pass. Follow linting and naming convention used in other test files. +3. Use proper labeling for your open PR (always set adequate priority and status) +4. Get an approving review from at least one of three designated reviewers - **@hexonaut**, **@lucas-manuel** or **@barrutko** +5. Enjoy governance messages being passed through the bridge to your domain! 🎉 *** *The IP in this repository was assigned to Mars SPC Limited in respect of the MarsOne SP* diff --git a/src/executors/BridgeExecutorBase.sol b/src/Executor.sol similarity index 86% rename from src/executors/BridgeExecutorBase.sol rename to src/Executor.sol index 90602ce..036e70d 100644 --- a/src/executors/BridgeExecutorBase.sol +++ b/src/Executor.sol @@ -1,18 +1,17 @@ // SPDX-License-Identifier: AGPL-3.0 pragma solidity ^0.8.10; -import { Address } from "lib/openzeppelin-contracts/contracts/utils/Address.sol"; +import { AccessControl } from "openzeppelin-contracts/contracts/access/AccessControl.sol"; +import { Address } from "openzeppelin-contracts/contracts/utils/Address.sol"; -import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol'; +import { IExecutor } from './interfaces/IExecutor.sol'; /** - * @title BridgeExecutorBase + * @title Executor * @author Aave - * @notice Abstract contract that implements basic executor functionality - * @dev It does not implement an external `queue` function. This should instead be done in the inheriting - * contract with proper access control + * @notice Executor which queues up message calls and executes them after an optional delay */ -abstract contract BridgeExecutorBase is IExecutorBase { +contract Executor is IExecutor, AccessControl { using Address for address; @@ -68,9 +67,71 @@ abstract contract BridgeExecutorBase is IExecutorBase { _updateDelay(delay); _updateGracePeriod(gracePeriod); _updateGuardian(guardian); + + _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor + function queue( + address[] memory targets, + uint256[] memory values, + string[] memory signatures, + bytes[] memory calldatas, + bool[] memory withDelegatecalls + ) external override onlyRole(DEFAULT_ADMIN_ROLE) { + if (targets.length == 0) revert EmptyTargets(); + uint256 targetsLength = targets.length; + if ( + targetsLength != values.length || + targetsLength != signatures.length || + targetsLength != calldatas.length || + targetsLength != withDelegatecalls.length + ) revert InconsistentParamsLength(); + + uint256 actionsSetId = _actionsSetCounter; + uint256 executionTime = block.timestamp + _delay; + unchecked { + ++_actionsSetCounter; + } + + for (uint256 i = 0; i < targetsLength; ) { + bytes32 actionHash = keccak256( + abi.encode( + targets[i], + values[i], + signatures[i], + calldatas[i], + executionTime, + withDelegatecalls[i] + ) + ); + if (isActionQueued(actionHash)) revert DuplicateAction(); + _queuedActions[actionHash] = true; + unchecked { + ++i; + } + } + + ActionsSet storage actionsSet = _actionsSets[actionsSetId]; + actionsSet.targets = targets; + actionsSet.values = values; + actionsSet.signatures = signatures; + actionsSet.calldatas = calldatas; + actionsSet.withDelegatecalls = withDelegatecalls; + actionsSet.executionTime = executionTime; + + emit ActionsSetQueued( + actionsSetId, + targets, + values, + signatures, + calldatas, + withDelegatecalls, + executionTime + ); + } + + /// @inheritdoc IExecutor function execute(uint256 actionsSetId) external payable override { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); @@ -98,7 +159,7 @@ abstract contract BridgeExecutorBase is IExecutorBase { emit ActionsSetExecuted(actionsSetId, msg.sender, returnedData); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function cancel(uint256 actionsSetId) external override onlyGuardian { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); @@ -123,23 +184,23 @@ abstract contract BridgeExecutorBase is IExecutorBase { emit ActionsSetCanceled(actionsSetId); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function updateGuardian(address guardian) external override onlyThis { _updateGuardian(guardian); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function updateDelay(uint256 delay) external override onlyThis { _updateDelay(delay); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function updateGracePeriod(uint256 gracePeriod) external override onlyThis { if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); _updateGracePeriod(gracePeriod); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function executeDelegateCall(address target, bytes calldata data) external payable @@ -150,30 +211,30 @@ abstract contract BridgeExecutorBase is IExecutorBase { return target.functionDelegateCall(data); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function receiveFunds() external payable override {} - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getDelay() external view override returns (uint256) { return _delay; } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getGracePeriod() external view override returns (uint256) { return _gracePeriod; } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getGuardian() external view override returns (address) { return _guardian; } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getActionsSetCount() external view override returns (uint256) { return _actionsSetCounter; } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getActionsSetById(uint256 actionsSetId) external view @@ -183,7 +244,7 @@ abstract contract BridgeExecutorBase is IExecutorBase { return _actionsSets[actionsSetId]; } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function getCurrentState(uint256 actionsSetId) public view override returns (ActionsSetState) { if (_actionsSetCounter <= actionsSetId) revert InvalidActionsSetId(); ActionsSet storage actionsSet = _actionsSets[actionsSetId]; @@ -198,7 +259,7 @@ abstract contract BridgeExecutorBase is IExecutorBase { } } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function isActionQueued(bytes32 actionHash) public view override returns (bool) { return _queuedActions[actionHash]; } @@ -218,74 +279,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { _gracePeriod = gracePeriod; } - /** - * @notice Queue an ActionsSet - * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise - * @param targets Array of targets to be called by the actions set - * @param values Array of values to pass in each call by the actions set - * @param signatures Array of function signatures to encode in each call (can be empty) - * @param calldatas Array of calldata to pass in each call (can be empty) - * @param withDelegatecalls Array of whether to delegatecall for each call - **/ - function _queue( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - bool[] memory withDelegatecalls - ) internal { - if (targets.length == 0) revert EmptyTargets(); - uint256 targetsLength = targets.length; - if ( - targetsLength != values.length || - targetsLength != signatures.length || - targetsLength != calldatas.length || - targetsLength != withDelegatecalls.length - ) revert InconsistentParamsLength(); - - uint256 actionsSetId = _actionsSetCounter; - uint256 executionTime = block.timestamp + _delay; - unchecked { - ++_actionsSetCounter; - } - - for (uint256 i = 0; i < targetsLength; ) { - bytes32 actionHash = keccak256( - abi.encode( - targets[i], - values[i], - signatures[i], - calldatas[i], - executionTime, - withDelegatecalls[i] - ) - ); - if (isActionQueued(actionHash)) revert DuplicateAction(); - _queuedActions[actionHash] = true; - unchecked { - ++i; - } - } - - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; - actionsSet.targets = targets; - actionsSet.values = values; - actionsSet.signatures = signatures; - actionsSet.calldatas = calldatas; - actionsSet.withDelegatecalls = withDelegatecalls; - actionsSet.executionTime = executionTime; - - emit ActionsSetQueued( - actionsSetId, - targets, - values, - signatures, - calldatas, - withDelegatecalls, - executionTime - ); - } - function _executeTransaction( address target, uint256 value, diff --git a/src/executors/AuthBridgeExecutor.sol b/src/executors/AuthBridgeExecutor.sol deleted file mode 100644 index 2772574..0000000 --- a/src/executors/AuthBridgeExecutor.sol +++ /dev/null @@ -1,47 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0 -pragma solidity ^0.8.10; - -import { AccessControl } from 'lib/openzeppelin-contracts/contracts/access/AccessControl.sol'; - -import { IAuthBridgeExecutor } from 'src/interfaces/IAuthBridgeExecutor.sol'; -import { BridgeExecutorBase } from './BridgeExecutorBase.sol'; - -/** - * @title AuthBridgeExecutor - * @notice Queue up proposals from an authorized bridge. - */ -contract AuthBridgeExecutor is IAuthBridgeExecutor, AccessControl, BridgeExecutorBase { - - /** - * @dev Constructor - * - * @param delay The delay before which an actions set can be executed - * @param gracePeriod The time period after a delay during which an actions set can be executed - * @param guardian The address of the guardian, which can cancel queued proposals (can be zero) - */ - constructor( - uint256 delay, - uint256 gracePeriod, - address guardian - ) - BridgeExecutorBase( - delay, - gracePeriod, - guardian - ) - { - _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - } - - /// @inheritdoc IAuthBridgeExecutor - function queue( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - bool[] memory withDelegatecalls - ) external onlyRole(DEFAULT_ADMIN_ROLE) { - _queue(targets, values, signatures, calldatas, withDelegatecalls); - } - -} diff --git a/src/interfaces/IAuthBridgeExecutor.sol b/src/interfaces/IAuthBridgeExecutor.sol deleted file mode 100644 index 9c8bbcb..0000000 --- a/src/interfaces/IAuthBridgeExecutor.sol +++ /dev/null @@ -1,31 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0 -pragma solidity ^0.8.10; - -import { IAccessControl } from 'lib/openzeppelin-contracts/contracts/access/IAccessControl.sol'; - -import { IExecutorBase } from './IExecutorBase.sol'; - -/** - * @title IAuthBridgeExecutor - * @notice Defines the basic interface for the AuthBridgeExecutor contract. - */ -interface IAuthBridgeExecutor is IAccessControl, IExecutorBase { - - /** - * @notice Queue an ActionsSet - * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise - * @param targets Array of targets to be called by the actions set - * @param values Array of values to pass in each call by the actions set - * @param signatures Array of function signatures to encode in each call by the actions (can be empty) - * @param calldatas Array of calldata to pass in each call by the actions set - * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set - **/ - function queue( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - bool[] memory withDelegatecalls - ) external; - -} diff --git a/src/interfaces/IExecutorBase.sol b/src/interfaces/IExecutor.sol similarity index 87% rename from src/interfaces/IExecutorBase.sol rename to src/interfaces/IExecutor.sol index d7290b0..707c256 100644 --- a/src/interfaces/IExecutorBase.sol +++ b/src/interfaces/IExecutor.sol @@ -1,12 +1,15 @@ // SPDX-License-Identifier: AGPL-3.0 -pragma solidity ^0.8.10; +pragma solidity >=0.8.0; + +import { IAccessControl } from 'openzeppelin-contracts/contracts/access/IAccessControl.sol'; /** - * @title IExecutorBase + * @title IExecutor * @author Aave - * @notice Defines the basic interface for the ExecutorBase abstract contract + * @notice Defines the interface for the Executor */ -interface IExecutorBase { +interface IExecutor is IAccessControl { + error InvalidInitParams(); error NotGuardian(); error OnlyCallableByThis(); @@ -115,6 +118,23 @@ interface IExecutorBase { **/ event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); + /** + * @notice Queue an ActionsSet + * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise + * @param targets Array of targets to be called by the actions set + * @param values Array of values to pass in each call by the actions set + * @param signatures Array of function signatures to encode in each call by the actions (can be empty) + * @param calldatas Array of calldata to pass in each call by the actions set + * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set + **/ + function queue( + address[] memory targets, + uint256[] memory values, + string[] memory signatures, + bytes[] memory calldatas, + bool[] memory withDelegatecalls + ) external; + /** * @notice Execute the ActionsSet * @param actionsSetId The id of the ActionsSet to execute @@ -208,4 +228,5 @@ interface IExecutorBase { * @return True if the underlying action of actionHash is queued, false otherwise **/ function isActionQueued(bytes32 actionHash) external view returns (bool); + } diff --git a/test/ArbitrumOneCrosschainTest.t.sol b/test/ArbitrumOneCrosschainTest.t.sol index 6cc3d06..4ed26e2 100644 --- a/test/ArbitrumOneCrosschainTest.t.sol +++ b/test/ArbitrumOneCrosschainTest.t.sol @@ -43,7 +43,7 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, defaultL2BridgeExecutorArgs.guardian diff --git a/test/BaseChainCrosschainTest.t.sol b/test/BaseChainCrosschainTest.t.sol index b87f747..51c6f50 100644 --- a/test/BaseChainCrosschainTest.t.sol +++ b/test/BaseChainCrosschainTest.t.sol @@ -41,7 +41,7 @@ contract BaseChainCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, defaultL2BridgeExecutorArgs.guardian diff --git a/test/CrosschainTestBase.sol b/test/CrosschainTestBase.sol index 7249ec5..99e55a5 100644 --- a/test/CrosschainTestBase.sol +++ b/test/CrosschainTestBase.sol @@ -6,9 +6,8 @@ import 'forge-std/Test.sol'; import { Bridge } from 'lib/xchain-helpers/src/testing/Bridge.sol'; import { DomainHelpers } from 'lib/xchain-helpers/src/testing/Domain.sol'; -import { IAuthBridgeExecutor } from 'src/interfaces/IAuthBridgeExecutor.sol'; -import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol'; -import { AuthBridgeExecutor } from 'src/executors/AuthBridgeExecutor.sol'; +import { IExecutor } from 'src/interfaces/IExecutor.sol'; +import { Executor } from 'src/Executor.sol'; import { IL1Executor } from './interfaces/IL1Executor.sol'; import { IPayload } from './interfaces/IPayload.sol'; @@ -48,7 +47,7 @@ abstract contract CrosschainPayload is IPayload { withDelegatecalls[0] = true; return abi.encodeWithSelector( - IAuthBridgeExecutor.queue.selector, + IExecutor.queue.selector, targets, values, signatures, @@ -78,8 +77,8 @@ abstract contract CrosschainTestBase is Test { Bridge public bridge; - AuthBridgeExecutor public bridgeExecutor; - address public bridgeReceiver; + Executor public bridgeExecutor; + address public bridgeReceiver; function deployCrosschainPayload(IPayload targetPayload, address bridgeReceiver) public virtual returns (IPayload); function relayMessagesAcrossBridge() internal virtual; @@ -132,7 +131,7 @@ abstract contract CrosschainTestBase is Test { skip(delay); - vm.expectRevert(IExecutorBase.OnlyQueuedActions.selector); + vm.expectRevert(IExecutor.OnlyQueuedActions.selector); bridgeExecutor.execute(0); } @@ -143,7 +142,7 @@ abstract contract CrosschainTestBase is Test { skip(delay); - vm.expectRevert(IExecutorBase.TimelockNotFinished.selector); + vm.expectRevert(IExecutor.TimelockNotFinished.selector); bridgeExecutor.execute(0); } @@ -197,7 +196,7 @@ abstract contract CrosschainTestBase is Test { vm.prank(defaultL2BridgeExecutorArgs.guardian); bridgeExecutor.cancel(0); - vm.expectRevert(IExecutorBase.OnlyQueuedActions.selector); + vm.expectRevert(IExecutor.OnlyQueuedActions.selector); vm.prank(defaultL2BridgeExecutorArgs.guardian); bridgeExecutor.cancel(0); } @@ -209,7 +208,7 @@ abstract contract CrosschainTestBase is Test { bridgeExecutor.execute(0); - vm.expectRevert(IExecutorBase.OnlyQueuedActions.selector); + vm.expectRevert(IExecutor.OnlyQueuedActions.selector); vm.prank(defaultL2BridgeExecutorArgs.guardian); bridgeExecutor.cancel(0); } @@ -219,7 +218,7 @@ abstract contract CrosschainTestBase is Test { skip(defaultL2BridgeExecutorArgs.delay + defaultL2BridgeExecutorArgs.gracePeriod + 1); - vm.expectRevert(IExecutorBase.OnlyQueuedActions.selector); + vm.expectRevert(IExecutor.OnlyQueuedActions.selector); vm.prank(defaultL2BridgeExecutorArgs.guardian); bridgeExecutor.cancel(0); } @@ -232,7 +231,7 @@ abstract contract CrosschainTestBase is Test { vm.prank(defaultL2BridgeExecutorArgs.guardian); bridgeExecutor.cancel(0); - vm.expectRevert(IExecutorBase.OnlyQueuedActions.selector); + vm.expectRevert(IExecutor.OnlyQueuedActions.selector); bridgeExecutor.execute(0); } diff --git a/test/AuthBridgeExecutor.t.sol b/test/Executor.t.sol similarity index 94% rename from test/AuthBridgeExecutor.t.sol rename to test/Executor.t.sol index d7fb049..93451aa 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/Executor.t.sol @@ -3,8 +3,8 @@ pragma solidity ^0.8.0; import 'forge-std/Test.sol'; -import { AuthBridgeExecutor } from 'src/executors/AuthBridgeExecutor.sol'; -import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol'; +import { IExecutor } from 'src/interfaces/IExecutor.sol'; +import { Executor } from 'src/Executor.sol'; contract DefaultPayload { event TestEvent(); @@ -26,7 +26,7 @@ contract RevertingPayload { } } -contract AuthBridgeExecutorTestBase is Test { +contract ExecutorTestBase is Test { struct Action { address[] targets; @@ -62,10 +62,10 @@ contract AuthBridgeExecutorTestBase is Test { address bridge = makeAddr("bridge"); address guardian = makeAddr("guardian"); - AuthBridgeExecutor executor; + Executor executor; function setUp() public { - executor = new AuthBridgeExecutor({ + executor = new Executor({ delay: DELAY, gracePeriod: GRACE_PERIOD, guardian: guardian @@ -137,7 +137,7 @@ contract AuthBridgeExecutorTestBase is Test { } function _assertActionSet(uint256 id, bool executed, bool canceled, uint256 executionTime, Action memory action) internal view { - IExecutorBase.ActionsSet memory actionsSet = executor.getActionsSetById(id); + IExecutor.ActionsSet memory actionsSet = executor.getActionsSetById(id); assertEq(actionsSet.targets, action.targets); assertEq(actionsSet.values, action.values); assertEq(actionsSet.signatures, action.signatures); @@ -150,17 +150,17 @@ contract AuthBridgeExecutorTestBase is Test { } -contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { +contract ExecutorConstructorTests is ExecutorTestBase { function test_constructor_invalidInitParams_boundary() public { vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); - executor = new AuthBridgeExecutor({ + executor = new Executor({ delay: DELAY, gracePeriod: 10 minutes - 1, guardian: guardian }); - executor = new AuthBridgeExecutor({ + executor = new Executor({ delay: DELAY, gracePeriod: 10 minutes, guardian: guardian @@ -174,7 +174,7 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { emit GracePeriodUpdate(0, GRACE_PERIOD); vm.expectEmit(); emit GuardianUpdate(address(0), guardian); - executor = new AuthBridgeExecutor({ + executor = new Executor({ delay: DELAY, gracePeriod: GRACE_PERIOD, guardian: guardian @@ -189,7 +189,7 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { } -contract AuthBridgeExecutorQueueTests is AuthBridgeExecutorTestBase { +contract ExecutorQueueTests is ExecutorTestBase { function test_queue_onlyDefaultAdmin() public { vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); @@ -292,7 +292,7 @@ contract AuthBridgeExecutorQueueTests is AuthBridgeExecutorTestBase { } -contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { +contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_actionsSetIdTooHigh_boundary() public { assertEq(executor.getActionsSetCount(), 0); @@ -439,7 +439,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); bytes[] memory returnedData = new bytes[](1); returnedData[0] = ""; @@ -451,7 +451,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } function test_execute_call() public { @@ -463,7 +463,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); bytes[] memory returnedData = new bytes[](1); returnedData[0] = ""; @@ -475,7 +475,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } function test_execute_delegateCallWithCalldata() public { @@ -488,7 +488,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); bytes[] memory returnedData = new bytes[](1); returnedData[0] = ""; @@ -500,7 +500,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } function test_execute_callWithCalldata() public { @@ -514,7 +514,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); bytes[] memory returnedData = new bytes[](1); returnedData[0] = ""; @@ -526,12 +526,12 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } } -contract AuthBridgeExecutorCancelTests is AuthBridgeExecutorTestBase { +contract ExecutorCancelTests is ExecutorTestBase { function test_cancel_notGuardian() public { _queueAction(); @@ -597,7 +597,7 @@ contract AuthBridgeExecutorCancelTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).canceled, false); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); vm.expectEmit(address(executor)); emit ActionsSetCanceled(0); @@ -606,12 +606,12 @@ contract AuthBridgeExecutorCancelTests is AuthBridgeExecutorTestBase { assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).canceled, true); - assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Canceled)); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Canceled)); } } -contract AuthBridgeExecutorUpdateTests is AuthBridgeExecutorTestBase { +contract ExecutorUpdateTests is ExecutorTestBase { function test_updateGuardian_notSelf() public { vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); @@ -674,7 +674,7 @@ contract AuthBridgeExecutorUpdateTests is AuthBridgeExecutorTestBase { } -contract AuthBridgeExecutorMiscTests is AuthBridgeExecutorTestBase { +contract ExecutorMiscTests is ExecutorTestBase { function test_executeDelegateCall_notSelf() public { vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); diff --git a/test/GnosisCrosschainTest.t.sol b/test/GnosisCrosschainTest.t.sol index 1707e8e..202a419 100644 --- a/test/GnosisCrosschainTest.t.sol +++ b/test/GnosisCrosschainTest.t.sol @@ -40,7 +40,7 @@ contract GnosisCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, defaultL2BridgeExecutorArgs.guardian diff --git a/test/OptimismCrosschainTest.t.sol b/test/OptimismCrosschainTest.t.sol index 6895e1e..eeb6a1d 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -41,7 +41,7 @@ contract OptimismCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, defaultL2BridgeExecutorArgs.guardian diff --git a/test/mocks/ReconfigurationPayload.sol b/test/mocks/ReconfigurationPayload.sol index 74f0100..107ba4c 100644 --- a/test/mocks/ReconfigurationPayload.sol +++ b/test/mocks/ReconfigurationPayload.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0 pragma solidity ^0.8.10; -import { IExecutorBase } from '../../src/interfaces/IExecutorBase.sol'; +import { IExecutor } from 'src/interfaces/IExecutor.sol'; import { IPayload } from '../interfaces/IPayload.sol'; @@ -25,9 +25,9 @@ contract ReconfigurationPayload is IPayload { } function execute() external override { - IExecutorBase(address(this)).updateDelay(getNewDelay()); - IExecutorBase(address(this)).updateGracePeriod(getNewGracePeriod()); - IExecutorBase(address(this)).updateGuardian(getNewGuardian()); + IExecutor(address(this)).updateDelay(getNewDelay()); + IExecutor(address(this)).updateGracePeriod(getNewGracePeriod()); + IExecutor(address(this)).updateGuardian(getNewGuardian()); } function getNewDelay() public view returns (uint256) { From 999d935ce903b4e5c31f4b721362f13038c2720b Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Thu, 13 Jun 2024 20:12:20 +0900 Subject: [PATCH 2/6] refactor guardian to be part of access control, also queue becomes a separate role --- src/Executor.sol | 55 +++++++-------------------- src/interfaces/IExecutor.sol | 36 +++++------------- test/ArbitrumOneCrosschainTest.t.sol | 7 ++-- test/BaseChainCrosschainTest.t.sol | 7 ++-- test/CrosschainTestBase.sol | 31 +++++++++------ test/Executor.t.sol | 52 +++++++------------------ test/GnosisCrosschainTest.t.sol | 7 ++-- test/OptimismCrosschainTest.t.sol | 7 ++-- test/mocks/ReconfigurationPayload.sol | 10 ++++- 9 files changed, 82 insertions(+), 130 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index 036e70d..f886466 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -15,6 +15,9 @@ contract Executor is IExecutor, AccessControl { using Address for address; + bytes32 public constant SUBMISSION_ROLE = keccak256('SUBMISSION_ROLE'); + bytes32 public constant GUARDIAN_ROLE = keccak256('GUARDIAN_ROLE'); + // Minimum allowed grace period, which reduces the risk of having an actions set expire due to network congestion uint256 constant MINIMUM_GRACE_PERIOD = 10 minutes; @@ -22,8 +25,6 @@ contract Executor is IExecutor, AccessControl { uint256 private _delay; // Time after the execution time during which the actions set can be executed uint256 private _gracePeriod; - // Address with the ability of canceling actions sets - address private _guardian; // Number of actions sets uint256 private _actionsSetCounter; @@ -32,33 +33,15 @@ contract Executor is IExecutor, AccessControl { // Map of queued actions (actionHash => isQueued) mapping(bytes32 => bool) private _queuedActions; - /** - * @dev Only guardian can call functions marked by this modifier. - **/ - modifier onlyGuardian() { - if (msg.sender != _guardian) revert NotGuardian(); - _; - } - - /** - * @dev Only this contract can call functions marked by this modifier. - **/ - modifier onlyThis() { - if (msg.sender != address(this)) revert OnlyCallableByThis(); - _; - } - /** * @dev Constructor * * @param delay The delay before which an actions set can be executed * @param gracePeriod The time period after a delay during which an actions set can be executed - * @param guardian The address of the guardian, which can cancel queued proposals (can be zero) */ constructor( uint256 delay, - uint256 gracePeriod, - address guardian + uint256 gracePeriod ) { if ( gracePeriod < MINIMUM_GRACE_PERIOD @@ -66,9 +49,12 @@ contract Executor is IExecutor, AccessControl { _updateDelay(delay); _updateGracePeriod(gracePeriod); - _updateGuardian(guardian); + + _setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE); + _setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE); _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); + _grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration } /// @inheritdoc IExecutor @@ -78,7 +64,7 @@ contract Executor is IExecutor, AccessControl { string[] memory signatures, bytes[] memory calldatas, bool[] memory withDelegatecalls - ) external override onlyRole(DEFAULT_ADMIN_ROLE) { + ) external override onlyRole(SUBMISSION_ROLE) { if (targets.length == 0) revert EmptyTargets(); uint256 targetsLength = targets.length; if ( @@ -160,7 +146,7 @@ contract Executor is IExecutor, AccessControl { } /// @inheritdoc IExecutor - function cancel(uint256 actionsSetId) external override onlyGuardian { + function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); ActionsSet storage actionsSet = _actionsSets[actionsSetId]; @@ -185,17 +171,12 @@ contract Executor is IExecutor, AccessControl { } /// @inheritdoc IExecutor - function updateGuardian(address guardian) external override onlyThis { - _updateGuardian(guardian); - } - - /// @inheritdoc IExecutor - function updateDelay(uint256 delay) external override onlyThis { + function updateDelay(uint256 delay) external override onlyRole(DEFAULT_ADMIN_ROLE) { _updateDelay(delay); } /// @inheritdoc IExecutor - function updateGracePeriod(uint256 gracePeriod) external override onlyThis { + function updateGracePeriod(uint256 gracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) { if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); _updateGracePeriod(gracePeriod); } @@ -205,7 +186,7 @@ contract Executor is IExecutor, AccessControl { external payable override - onlyThis + onlyRole(DEFAULT_ADMIN_ROLE) returns (bytes memory) { return target.functionDelegateCall(data); @@ -224,11 +205,6 @@ contract Executor is IExecutor, AccessControl { return _gracePeriod; } - /// @inheritdoc IExecutor - function getGuardian() external view override returns (address) { - return _guardian; - } - /// @inheritdoc IExecutor function getActionsSetCount() external view override returns (uint256) { return _actionsSetCounter; @@ -264,11 +240,6 @@ contract Executor is IExecutor, AccessControl { return _queuedActions[actionHash]; } - function _updateGuardian(address guardian) internal { - emit GuardianUpdate(_guardian, guardian); - _guardian = guardian; - } - function _updateDelay(uint256 delay) internal { emit DelayUpdate(_delay, delay); _delay = delay; diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index 707c256..85a53c9 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -11,13 +11,7 @@ import { IAccessControl } from 'openzeppelin-contracts/contracts/access/IAccessC interface IExecutor is IAccessControl { error InvalidInitParams(); - error NotGuardian(); - error OnlyCallableByThis(); - error MinimumDelayTooLong(); - error MaximumDelayTooShort(); error GracePeriodTooShort(); - error DelayShorterThanMin(); - error DelayLongerThanMax(); error OnlyQueuedActions(); error TimelockNotFinished(); error InvalidActionsSetId(); @@ -25,7 +19,6 @@ interface IExecutor is IAccessControl { error InconsistentParamsLength(); error DuplicateAction(); error InsufficientBalance(); - error FailedActionExecution(); /** * @notice This enum contains all possible actions set states @@ -97,13 +90,6 @@ interface IExecutor is IAccessControl { **/ event ActionsSetCanceled(uint256 indexed id); - /** - * @dev Emitted when a new guardian is set - * @param oldGuardian The address of the old guardian - * @param newGuardian The address of the new guardian - **/ - event GuardianUpdate(address oldGuardian, address newGuardian); - /** * @dev Emitted when the delay (between queueing and execution) is updated * @param oldDelay The value of the old delay @@ -118,6 +104,16 @@ interface IExecutor is IAccessControl { **/ event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); + /** + * @notice The role that allows submission of a queued action. + **/ + function SUBMISSION_ROLE() external view returns (bytes32); + + /** + * @notice The role that allows a guardian to cancel a pending action. + **/ + function GUARDIAN_ROLE() external view returns (bytes32); + /** * @notice Queue an ActionsSet * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise @@ -147,12 +143,6 @@ interface IExecutor is IAccessControl { **/ function cancel(uint256 actionsSetId) external; - /** - * @notice Update guardian - * @param guardian The address of the new guardian - **/ - function updateGuardian(address guardian) external; - /** * @notice Update the delay, time between queueing and execution of ActionsSet * @dev It does not affect to actions set that are already queued @@ -195,12 +185,6 @@ interface IExecutor is IAccessControl { **/ function getGracePeriod() external view returns (uint256); - /** - * @notice Returns the address of the guardian - * @return The address of the guardian - **/ - function getGuardian() external view returns (address); - /** * @notice Returns the total number of actions sets of the executor * @return The number of actions sets diff --git a/test/ArbitrumOneCrosschainTest.t.sol b/test/ArbitrumOneCrosschainTest.t.sol index 4ed26e2..c8afd68 100644 --- a/test/ArbitrumOneCrosschainTest.t.sol +++ b/test/ArbitrumOneCrosschainTest.t.sol @@ -45,14 +45,15 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase { bridge.destination.selectFork(); bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, - defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.guardian + defaultL2BridgeExecutorArgs.gracePeriod ); bridgeReceiver = address(new ArbitrumReceiver( defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, address(bridgeExecutor) )); - bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian); + bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this)); bridge.source.selectFork(); vm.deal(L1_EXECUTOR, 0.01 ether); diff --git a/test/BaseChainCrosschainTest.t.sol b/test/BaseChainCrosschainTest.t.sol index 51c6f50..ec4169c 100644 --- a/test/BaseChainCrosschainTest.t.sol +++ b/test/BaseChainCrosschainTest.t.sol @@ -43,14 +43,15 @@ contract BaseChainCrosschainTest is CrosschainTestBase { bridge.destination.selectFork(); bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, - defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.guardian + defaultL2BridgeExecutorArgs.gracePeriod ); bridgeReceiver = address(new OptimismReceiver( defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, address(bridgeExecutor) )); - bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian); + bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this)); bridge.source.selectFork(); } diff --git a/test/CrosschainTestBase.sol b/test/CrosschainTestBase.sol index 99e55a5..33d5dc6 100644 --- a/test/CrosschainTestBase.sol +++ b/test/CrosschainTestBase.sol @@ -306,6 +306,13 @@ abstract contract CrosschainTestBase is Test { function test_selfReconfiguration() public { bridge.destination.selectFork(); + L2BridgeExecutorArguments memory newL2BridgeExecutorParams = L2BridgeExecutorArguments({ + ethereumGovernanceExecutor: defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, + delay: 1200, + gracePeriod: 1800, + guardian: makeAddr('newGuardian') + }); + assertEq( bridgeExecutor.getDelay(), defaultL2BridgeExecutorArgs.delay @@ -315,20 +322,18 @@ abstract contract CrosschainTestBase is Test { defaultL2BridgeExecutorArgs.gracePeriod ); assertEq( - bridgeExecutor.getGuardian(), - defaultL2BridgeExecutorArgs.guardian + bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian), + true + ); + assertEq( + bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), newL2BridgeExecutorParams.guardian), + false ); - - L2BridgeExecutorArguments memory newL2BridgeExecutorParams = L2BridgeExecutorArguments({ - ethereumGovernanceExecutor: defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, - delay: 1200, - gracePeriod: 1800, - guardian: makeAddr('newGuardian') - }); IPayload reconfigurationPayload = IPayload(new ReconfigurationPayload( newL2BridgeExecutorParams.delay, newL2BridgeExecutorParams.gracePeriod, + defaultL2BridgeExecutorArgs.guardian, newL2BridgeExecutorParams.guardian )); @@ -360,8 +365,12 @@ abstract contract CrosschainTestBase is Test { newL2BridgeExecutorParams.gracePeriod ); assertEq( - bridgeExecutor.getGuardian(), - newL2BridgeExecutorParams.guardian + bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian), + false + ); + assertEq( + bridgeExecutor.hasRole(bridgeExecutor.GUARDIAN_ROLE(), newL2BridgeExecutorParams.guardian), + true ); } diff --git a/test/Executor.t.sol b/test/Executor.t.sol index 93451aa..f36c462 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -51,7 +51,6 @@ contract ExecutorTestBase is Test { bytes[] returnedData ); event ActionsSetCanceled(uint256 indexed id); - event GuardianUpdate(address oldGuardian, address newGuardian); event DelayUpdate(uint256 oldDelay, uint256 newDelay); event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); event TestEvent(); @@ -67,10 +66,10 @@ contract ExecutorTestBase is Test { function setUp() public { executor = new Executor({ delay: DELAY, - gracePeriod: GRACE_PERIOD, - guardian: guardian + gracePeriod: GRACE_PERIOD }); - executor.grantRole(executor.DEFAULT_ADMIN_ROLE(), bridge); + executor.grantRole(executor.SUBMISSION_ROLE(), bridge); + executor.grantRole(executor.GUARDIAN_ROLE(), guardian); executor.revokeRole(executor.DEFAULT_ADMIN_ROLE(), address(this)); } @@ -156,14 +155,12 @@ contract ExecutorConstructorTests is ExecutorTestBase { vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); executor = new Executor({ delay: DELAY, - gracePeriod: 10 minutes - 1, - guardian: guardian + gracePeriod: 10 minutes - 1 }); executor = new Executor({ delay: DELAY, - gracePeriod: 10 minutes, - guardian: guardian + gracePeriod: 10 minutes }); } @@ -172,27 +169,24 @@ contract ExecutorConstructorTests is ExecutorTestBase { emit DelayUpdate(0, DELAY); vm.expectEmit(); emit GracePeriodUpdate(0, GRACE_PERIOD); - vm.expectEmit(); - emit GuardianUpdate(address(0), guardian); executor = new Executor({ delay: DELAY, - gracePeriod: GRACE_PERIOD, - guardian: guardian + gracePeriod: GRACE_PERIOD }); assertEq(executor.getDelay(), DELAY); assertEq(executor.getGracePeriod(), GRACE_PERIOD); - assertEq(executor.getGuardian(), guardian); - assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(this)), true); + assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(this)), true); + assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(executor)), true); } } contract ExecutorQueueTests is ExecutorTestBase { - function test_queue_onlyDefaultAdmin() public { - vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); + function test_queue_onlySubmissionRole() public { + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.SUBMISSION_ROLE())); executor.queue(new address[](0), new uint256[](0), new string[](0), new bytes[](0), new bool[](0)); } @@ -537,7 +531,7 @@ contract ExecutorCancelTests is ExecutorTestBase { _queueAction(); skip(DELAY); - vm.expectRevert(abi.encodeWithSignature("NotGuardian()")); + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.GUARDIAN_ROLE())); executor.cancel(0); } @@ -613,26 +607,8 @@ contract ExecutorCancelTests is ExecutorTestBase { contract ExecutorUpdateTests is ExecutorTestBase { - function test_updateGuardian_notSelf() public { - vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); - executor.updateGuardian(guardian); - } - - function test_updateGuardian() public { - address newGuardian = makeAddr("newGuardian"); - - assertEq(executor.getGuardian(), guardian); - - vm.expectEmit(address(executor)); - emit GuardianUpdate(guardian, newGuardian); - vm.prank(address(executor)); - executor.updateGuardian(newGuardian); - - assertEq(executor.getGuardian(), newGuardian); - } - function test_updateDelay_notSelf() public { - vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); executor.updateDelay(2 days); } @@ -648,7 +624,7 @@ contract ExecutorUpdateTests is ExecutorTestBase { } function test_updateGracePeriod_notSelf() public { - vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); executor.updateGracePeriod(60 days); } @@ -677,7 +653,7 @@ contract ExecutorUpdateTests is ExecutorTestBase { contract ExecutorMiscTests is ExecutorTestBase { function test_executeDelegateCall_notSelf() public { - vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); executor.executeDelegateCall(address(0), ""); } diff --git a/test/GnosisCrosschainTest.t.sol b/test/GnosisCrosschainTest.t.sol index 202a419..e194e66 100644 --- a/test/GnosisCrosschainTest.t.sol +++ b/test/GnosisCrosschainTest.t.sol @@ -42,8 +42,7 @@ contract GnosisCrosschainTest is CrosschainTestBase { bridge.destination.selectFork(); bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, - defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.guardian + defaultL2BridgeExecutorArgs.gracePeriod ); bridgeReceiver = address(new AMBReceiver( AMBBridgeTesting.getGnosisMessengerFromChainAlias(bridge.destination.chain.chainAlias), @@ -51,7 +50,9 @@ contract GnosisCrosschainTest is CrosschainTestBase { defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, address(bridgeExecutor) )); - bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian); + bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this)); bridge.source.selectFork(); } diff --git a/test/OptimismCrosschainTest.t.sol b/test/OptimismCrosschainTest.t.sol index eeb6a1d..a3a6bec 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -43,14 +43,15 @@ contract OptimismCrosschainTest is CrosschainTestBase { bridge.destination.selectFork(); bridgeExecutor = new Executor( defaultL2BridgeExecutorArgs.delay, - defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.guardian + defaultL2BridgeExecutorArgs.gracePeriod ); bridgeReceiver = address(new OptimismReceiver( defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, address(bridgeExecutor) )); - bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.SUBMISSION_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.GUARDIAN_ROLE(), defaultL2BridgeExecutorArgs.guardian); + bridgeExecutor.revokeRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), address(this)); bridge.source.selectFork(); } diff --git a/test/mocks/ReconfigurationPayload.sol b/test/mocks/ReconfigurationPayload.sol index 107ba4c..7fcd178 100644 --- a/test/mocks/ReconfigurationPayload.sol +++ b/test/mocks/ReconfigurationPayload.sol @@ -12,22 +12,26 @@ contract ReconfigurationPayload is IPayload { uint256 public immutable newDelay; uint256 public immutable newGracePeriod; + address public immutable oldGuardian; address public immutable newGuardian; constructor( uint256 _newDelay, uint256 _newGracePeriod, + address _oldGuardian, address _newGuardian ) { newDelay = _newDelay; newGracePeriod = _newGracePeriod; + oldGuardian = _oldGuardian; newGuardian = _newGuardian; } function execute() external override { IExecutor(address(this)).updateDelay(getNewDelay()); IExecutor(address(this)).updateGracePeriod(getNewGracePeriod()); - IExecutor(address(this)).updateGuardian(getNewGuardian()); + IExecutor(address(this)).revokeRole(IExecutor(address(this)).GUARDIAN_ROLE(), getOldGuardian()); + IExecutor(address(this)).grantRole(IExecutor(address(this)).GUARDIAN_ROLE(), getNewGuardian()); } function getNewDelay() public view returns (uint256) { @@ -38,6 +42,10 @@ contract ReconfigurationPayload is IPayload { return newGracePeriod; } + function getOldGuardian() public view returns (address) { + return oldGuardian; + } + function getNewGuardian() public view returns (address) { return newGuardian; } From 2e7518d7e7771df2013d16f93bf2744403d55d92 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Thu, 13 Jun 2024 23:08:38 +0900 Subject: [PATCH 3/6] formatting --- test/Executor.t.sol | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/Executor.t.sol b/test/Executor.t.sol index f36c462..ccb29f8 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -65,8 +65,8 @@ contract ExecutorTestBase is Test { function setUp() public { executor = new Executor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD + delay: DELAY, + gracePeriod: GRACE_PERIOD }); executor.grantRole(executor.SUBMISSION_ROLE(), bridge); executor.grantRole(executor.GUARDIAN_ROLE(), guardian); @@ -154,13 +154,13 @@ contract ExecutorConstructorTests is ExecutorTestBase { function test_constructor_invalidInitParams_boundary() public { vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); executor = new Executor({ - delay: DELAY, - gracePeriod: 10 minutes - 1 + delay: DELAY, + gracePeriod: 10 minutes - 1 }); executor = new Executor({ - delay: DELAY, - gracePeriod: 10 minutes + delay: DELAY, + gracePeriod: 10 minutes }); } @@ -170,8 +170,8 @@ contract ExecutorConstructorTests is ExecutorTestBase { vm.expectEmit(); emit GracePeriodUpdate(0, GRACE_PERIOD); executor = new Executor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD + delay: DELAY, + gracePeriod: GRACE_PERIOD }); assertEq(executor.getDelay(), DELAY); From 693d7e719d21405b42569f10725cd7f9a9362cc9 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Thu, 13 Jun 2024 10:46:09 -0400 Subject: [PATCH 4/6] feat: update with new cleanup --- src/Executor.sol | 180 ++++++++++++++--------------------- src/interfaces/IExecutor.sol | 135 +++++++++++++------------- test/CrosschainTestBase.sol | 8 +- test/Executor.t.sol | 56 +++++------ 4 files changed, 174 insertions(+), 205 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index f886466..aee8d06 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: AGPL-3.0 -pragma solidity ^0.8.10; +pragma solidity ^0.8.22; import { AccessControl } from "openzeppelin-contracts/contracts/access/AccessControl.sol"; import { Address } from "openzeppelin-contracts/contracts/utils/Address.sol"; @@ -18,43 +18,39 @@ contract Executor is IExecutor, AccessControl { bytes32 public constant SUBMISSION_ROLE = keccak256('SUBMISSION_ROLE'); bytes32 public constant GUARDIAN_ROLE = keccak256('GUARDIAN_ROLE'); - // Minimum allowed grace period, which reduces the risk of having an actions set expire due to network congestion - uint256 constant MINIMUM_GRACE_PERIOD = 10 minutes; + uint256 public constant MINIMUM_GRACE_PERIOD = 10 minutes; - // Time between queuing and execution - uint256 private _delay; - // Time after the execution time during which the actions set can be executed - uint256 private _gracePeriod; - - // Number of actions sets - uint256 private _actionsSetCounter; // Map of registered actions sets (id => ActionsSet) mapping(uint256 => ActionsSet) private _actionsSets; - // Map of queued actions (actionHash => isQueued) - mapping(bytes32 => bool) private _queuedActions; + + uint256 public override actionsSetCount; // Number of actions sets + uint256 public override delay; // Time between queuing and execution + uint256 public override gracePeriod; // Time after delay during which an actions set can be executed + + mapping(bytes32 => bool) public override isActionQueued; /** - * @dev Constructor - * - * @param delay The delay before which an actions set can be executed - * @param gracePeriod The time period after a delay during which an actions set can be executed + * @dev Constructor + * @param delay_ The delay before which an actions set can be executed + * @param gracePeriod_ The time period after a delay during which an actions set can be executed */ constructor( - uint256 delay, - uint256 gracePeriod + uint256 delay_, + uint256 gracePeriod_ ) { if ( - gracePeriod < MINIMUM_GRACE_PERIOD + gracePeriod_ < MINIMUM_GRACE_PERIOD ) revert InvalidInitParams(); - _updateDelay(delay); - _updateGracePeriod(gracePeriod); + _updateDelay(delay_); + _updateGracePeriod(gracePeriod_); _setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE); _setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE); + // Necessary for self-referential calls to change configuration + _grantRole(DEFAULT_ADMIN_ROLE, address(this)); _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - _grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration } /// @inheritdoc IExecutor @@ -66,21 +62,23 @@ contract Executor is IExecutor, AccessControl { bool[] memory withDelegatecalls ) external override onlyRole(SUBMISSION_ROLE) { if (targets.length == 0) revert EmptyTargets(); + uint256 targetsLength = targets.length; if ( - targetsLength != values.length || + targetsLength != values.length || targetsLength != signatures.length || - targetsLength != calldatas.length || + targetsLength != calldatas.length || targetsLength != withDelegatecalls.length ) revert InconsistentParamsLength(); - uint256 actionsSetId = _actionsSetCounter; - uint256 executionTime = block.timestamp + _delay; + uint256 actionsSetId = actionsSetCount; + uint256 executionTime = block.timestamp + delay; + unchecked { - ++_actionsSetCounter; + ++actionsSetCount; } - for (uint256 i = 0; i < targetsLength; ) { + for (uint256 i = 0; i < targetsLength; ++i) { bytes32 actionHash = keccak256( abi.encode( targets[i], @@ -91,20 +89,19 @@ contract Executor is IExecutor, AccessControl { withDelegatecalls[i] ) ); - if (isActionQueued(actionHash)) revert DuplicateAction(); - _queuedActions[actionHash] = true; - unchecked { - ++i; - } + if (isActionQueued[actionHash]) revert DuplicateAction(); + + isActionQueued[actionHash] = true; } - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; - actionsSet.targets = targets; - actionsSet.values = values; - actionsSet.signatures = signatures; - actionsSet.calldatas = calldatas; + ActionsSet storage actionsSet =_actionsSets[actionsSetId]; + + actionsSet.targets = targets; + actionsSet.values = values; + actionsSet.signatures = signatures; + actionsSet.calldatas = calldatas; actionsSet.withDelegatecalls = withDelegatecalls; - actionsSet.executionTime = executionTime; + actionsSet.executionTime = executionTime; emit ActionsSetQueued( actionsSetId, @@ -121,14 +118,16 @@ contract Executor is IExecutor, AccessControl { function execute(uint256 actionsSetId) external payable override { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; + ActionsSet storage actionsSet =_actionsSets[actionsSetId]; + if (block.timestamp < actionsSet.executionTime) revert TimelockNotFinished(); actionsSet.executed = true; + uint256 actionCount = actionsSet.targets.length; bytes[] memory returnedData = new bytes[](actionCount); - for (uint256 i = 0; i < actionCount; ) { + for (uint256 i = 0; i < actionCount; ++i) { returnedData[i] = _executeTransaction( actionsSet.targets[i], actionsSet.values[i], @@ -137,9 +136,6 @@ contract Executor is IExecutor, AccessControl { actionsSet.executionTime, actionsSet.withDelegatecalls[i] ); - unchecked { - ++i; - } } emit ActionsSetExecuted(actionsSetId, msg.sender, returnedData); @@ -149,12 +145,12 @@ contract Executor is IExecutor, AccessControl { function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; + ActionsSet storage actionsSet =_actionsSets[actionsSetId]; actionsSet.canceled = true; uint256 targetsLength = actionsSet.targets.length; - for (uint256 i = 0; i < targetsLength; ) { - _cancelTransaction( + for (uint256 i = 0; i < targetsLength; ++i) { + _removeActionFromQueue( actionsSet.targets[i], actionsSet.values[i], actionsSet.signatures[i], @@ -162,23 +158,22 @@ contract Executor is IExecutor, AccessControl { actionsSet.executionTime, actionsSet.withDelegatecalls[i] ); - unchecked { - ++i; - } } emit ActionsSetCanceled(actionsSetId); } /// @inheritdoc IExecutor - function updateDelay(uint256 delay) external override onlyRole(DEFAULT_ADMIN_ROLE) { - _updateDelay(delay); + function updateDelay(uint256 newDelay) external override onlyRole(DEFAULT_ADMIN_ROLE) { + _updateDelay(newDelay); } /// @inheritdoc IExecutor - function updateGracePeriod(uint256 gracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) { - if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); - _updateGracePeriod(gracePeriod); + function updateGracePeriod(uint256 newGracePeriod) + external override onlyRole(DEFAULT_ADMIN_ROLE) + { + if (newGracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); + _updateGracePeriod(newGracePeriod); } /// @inheritdoc IExecutor @@ -195,21 +190,6 @@ contract Executor is IExecutor, AccessControl { /// @inheritdoc IExecutor function receiveFunds() external payable override {} - /// @inheritdoc IExecutor - function getDelay() external view override returns (uint256) { - return _delay; - } - - /// @inheritdoc IExecutor - function getGracePeriod() external view override returns (uint256) { - return _gracePeriod; - } - - /// @inheritdoc IExecutor - function getActionsSetCount() external view override returns (uint256) { - return _actionsSetCounter; - } - /// @inheritdoc IExecutor function getActionsSetById(uint256 actionsSetId) external @@ -222,32 +202,24 @@ contract Executor is IExecutor, AccessControl { /// @inheritdoc IExecutor function getCurrentState(uint256 actionsSetId) public view override returns (ActionsSetState) { - if (_actionsSetCounter <= actionsSetId) revert InvalidActionsSetId(); - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; - if (actionsSet.canceled) { - return ActionsSetState.Canceled; - } else if (actionsSet.executed) { - return ActionsSetState.Executed; - } else if (block.timestamp > actionsSet.executionTime + _gracePeriod) { - return ActionsSetState.Expired; - } else { - return ActionsSetState.Queued; - } - } + if (actionsSetCount <= actionsSetId) revert InvalidActionsSetId(); - /// @inheritdoc IExecutor - function isActionQueued(bytes32 actionHash) public view override returns (bool) { - return _queuedActions[actionHash]; + ActionsSet storage actionsSet =_actionsSets[actionsSetId]; + + if (actionsSet.canceled) return ActionsSetState.Canceled; + else if (actionsSet.executed) return ActionsSetState.Executed; + else if (block.timestamp > actionsSet.executionTime + gracePeriod) return ActionsSetState.Expired; + else return ActionsSetState.Queued; } - function _updateDelay(uint256 delay) internal { - emit DelayUpdate(_delay, delay); - _delay = delay; + function _updateDelay(uint256 newDelay) internal { + emit DelayUpdate(delay, newDelay); + delay = newDelay; } - function _updateGracePeriod(uint256 gracePeriod) internal { - emit GracePeriodUpdate(_gracePeriod, gracePeriod); - _gracePeriod = gracePeriod; + function _updateGracePeriod(uint256 newGracePeriod) internal { + emit GracePeriodUpdate(gracePeriod, newGracePeriod); + gracePeriod = newGracePeriod; } function _executeTransaction( @@ -260,26 +232,18 @@ contract Executor is IExecutor, AccessControl { ) internal returns (bytes memory) { if (address(this).balance < value) revert InsufficientBalance(); - bytes32 actionHash = keccak256( - abi.encode(target, value, signature, data, executionTime, withDelegatecall) - ); - _queuedActions[actionHash] = false; + _removeActionFromQueue(target, value, signature, data, executionTime, withDelegatecall); - bytes memory callData; - if (bytes(signature).length == 0) { - callData = data; - } else { - callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data); - } + bytes memory callData = bytes(signature).length == 0 + ? data + : abi.encodePacked(bytes4(keccak256(bytes(signature))), data); - if (withDelegatecall) { - return this.executeDelegateCall{value: value}(target, callData); - } else { - return target.functionCallWithValue(callData, value); - } + if (withDelegatecall) return this.executeDelegateCall{value: value}(target, callData); + + return target.functionCallWithValue(callData, value); } - function _cancelTransaction( + function _removeActionFromQueue( address target, uint256 value, string memory signature, @@ -290,7 +254,7 @@ contract Executor is IExecutor, AccessControl { bytes32 actionHash = keccak256( abi.encode(target, value, signature, data, executionTime, withDelegatecall) ); - _queuedActions[actionHash] = false; + isActionQueued[actionHash] = false; } } diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index 85a53c9..c37a2f2 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -31,15 +31,15 @@ interface IExecutor is IAccessControl { } /** - * @notice This struct contains the data needed to execute a specified set of actions - * @param targets Array of targets to call - * @param values Array of values to pass in each call - * @param signatures Array of function signatures to encode in each call (can be empty) - * @param calldatas Array of calldatas to pass in each call, appended to the signature at the same array index if not empty - * @param withDelegateCalls Array of whether to delegatecall for each call - * @param executionTime Timestamp starting from which the actions set can be executed - * @param executed True if the actions set has been executed, false otherwise - * @param canceled True if the actions set has been canceled, false otherwise + * @notice This struct contains the data needed to execute a specified set of actions. + * @param targets Array of targets to call. + * @param values Array of values to pass in each call. + * @param signatures Array of function signatures to encode in each call (can be empty). + * @param calldatas Array of calldatas to pass in each call, appended to the signature at the same array index if not empty. + * @param withDelegateCalls Array of whether to delegatecall for each call. + * @param executionTime Timestamp starting from which the actions set can be executed. + * @param executed True if the actions set has been executed, false otherwise. + * @param canceled True if the actions set has been canceled, false otherwise. */ struct ActionsSet { address[] targets; @@ -53,14 +53,14 @@ interface IExecutor is IAccessControl { } /** - * @dev Emitted when an ActionsSet is queued - * @param id Id of the ActionsSet - * @param targets Array of targets to be called by the actions set - * @param values Array of values to pass in each call by the actions set - * @param signatures Array of function signatures to encode in each call by the actions set - * @param calldatas Array of calldata to pass in each call by the actions set - * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set - * @param executionTime The timestamp at which this actions set can be executed + * @dev Emitted when an ActionsSet is queued. + * @param id Id of the ActionsSet. + * @param targets Array of targets to be called by the actions set. + * @param values Array of values to pass in each call by the actions set. + * @param signatures Array of function signatures to encode in each call by the actions set. + * @param calldatas Array of calldata to pass in each call by the actions set. + * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set. + * @param executionTime The timestamp at which this actions set can be executed. **/ event ActionsSetQueued( uint256 indexed id, @@ -73,10 +73,10 @@ interface IExecutor is IAccessControl { ); /** - * @dev Emitted when an ActionsSet is successfully executed - * @param id Id of the ActionsSet - * @param initiatorExecution The address that triggered the ActionsSet execution - * @param returnedData The returned data from the ActionsSet execution + * @dev Emitted when an ActionsSet is successfully executed. + * @param id Id of the ActionsSet. + * @param initiatorExecution The address that triggered the ActionsSet execution. + * @param returnedData The returned data from the ActionsSet execution. **/ event ActionsSetExecuted( uint256 indexed id, @@ -85,22 +85,22 @@ interface IExecutor is IAccessControl { ); /** - * @dev Emitted when an ActionsSet is cancelled by the guardian - * @param id Id of the ActionsSet + * @dev Emitted when an ActionsSet is cancelled by the guardian. + * @param id Id of the ActionsSet. **/ event ActionsSetCanceled(uint256 indexed id); /** - * @dev Emitted when the delay (between queueing and execution) is updated - * @param oldDelay The value of the old delay - * @param newDelay The value of the new delay + * @dev Emitted when the delay (between queueing and execution) is updated. + * @param oldDelay The value of the old delay. + * @param newDelay The value of the new delay. **/ event DelayUpdate(uint256 oldDelay, uint256 newDelay); /** - * @dev Emitted when the grace period (between executionTime and expiration) is updated - * @param oldGracePeriod The value of the old grace period - * @param newGracePeriod The value of the new grace period + * @dev Emitted when the grace period (between executionTime and expiration) is updated. + * @param oldGracePeriod The value of the old grace period. + * @param newGracePeriod The value of the new grace period. **/ event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); @@ -115,13 +115,18 @@ interface IExecutor is IAccessControl { function GUARDIAN_ROLE() external view returns (bytes32); /** - * @notice Queue an ActionsSet - * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise - * @param targets Array of targets to be called by the actions set - * @param values Array of values to pass in each call by the actions set - * @param signatures Array of function signatures to encode in each call by the actions (can be empty) - * @param calldatas Array of calldata to pass in each call by the actions set - * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set + * @notice Returns the minimum grace period that can be set, 10 minutes. + */ + function MINIMUM_GRACE_PERIOD() external view returns (uint256); + + /** + * @notice Queue an ActionsSet. + * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise. + * @param targets Array of targets to be called by the actions set. + * @param values Array of values to pass in each call by the actions set. + * @param signatures Array of function signatures to encode in each call by the actions (can be empty). + * @param calldatas Array of calldata to pass in each call by the actions set. + * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set. **/ function queue( address[] memory targets, @@ -133,34 +138,34 @@ interface IExecutor is IAccessControl { /** * @notice Execute the ActionsSet - * @param actionsSetId The id of the ActionsSet to execute + * @param actionsSetId The id of the ActionsSet to execute **/ function execute(uint256 actionsSetId) external payable; /** - * @notice Cancel the ActionsSet - * @param actionsSetId The id of the ActionsSet to cancel + * @notice Cancel the ActionsSet. + * @param actionsSetId The id of the ActionsSet to cancel. **/ function cancel(uint256 actionsSetId) external; /** - * @notice Update the delay, time between queueing and execution of ActionsSet - * @dev It does not affect to actions set that are already queued - * @param delay The value of the delay (in seconds) + * @notice Update the delay, time between queueing and execution of ActionsSet. + * @dev It does not affect to actions set that are already queued. + * @param delay The value of the delay (in seconds). **/ function updateDelay(uint256 delay) external; /** * @notice Update the grace period, the period after the execution time during which an actions set can be executed - * @param gracePeriod The value of the grace period (in seconds) + * @param gracePeriod The value of the grace period (in seconds). **/ function updateGracePeriod(uint256 gracePeriod) external; /** - * @notice Allows to delegatecall a given target with an specific amount of value - * @dev This function is external so it allows to specify a defined msg.value for the delegate call, reducing - * the risk that a delegatecall gets executed with more value than intended - * @return The bytes returned by the delegate call + * @notice Allows to delegatecall a given target with an specific amount of value. + * @dev This function is external so it allows to specify a defined msg.value for the delegate call, reducing + * the risk that a delegatecall gets executed with more value than intended. + * @return The bytes returned by the delegate call. **/ function executeDelegateCall(address target, bytes calldata data) external @@ -168,8 +173,8 @@ interface IExecutor is IAccessControl { returns (bytes memory); /** - * @notice Allows to receive funds into the executor - * @dev Useful for actionsSet that needs funds to gets executed + * @notice Allows to receive funds into the executor. + * @dev Useful for actionsSet that needs funds to gets executed. */ function receiveFunds() external payable; @@ -177,39 +182,39 @@ interface IExecutor is IAccessControl { * @notice Returns the delay (between queuing and execution) * @return The value of the delay (in seconds) **/ - function getDelay() external view returns (uint256); + function delay() external view returns (uint256); /** - * @notice Returns the grace period + * @notice Time after the execution time during which the actions set can be executed. * @return The value of the grace period (in seconds) **/ - function getGracePeriod() external view returns (uint256); + function gracePeriod() external view returns (uint256); /** - * @notice Returns the total number of actions sets of the executor - * @return The number of actions sets + * @notice Returns the total number of actions sets of the executor. + * @return The number of actions sets. **/ - function getActionsSetCount() external view returns (uint256); + function actionsSetCount() external view returns (uint256); /** - * @notice Returns the data of an actions set - * @param actionsSetId The id of the ActionsSet - * @return The data of the ActionsSet + * @notice Returns the data of an actions set. + * @param actionsSetId The id of the ActionsSet. + * @return The data of the ActionsSet. **/ function getActionsSetById(uint256 actionsSetId) external view returns (ActionsSet memory); /** - * @notice Returns the current state of an actions set - * @param actionsSetId The id of the ActionsSet - * @return The current state of theI ActionsSet + * @notice Returns the current state of an actions set. + * @param actionsSetId The id of the ActionsSet. + * @return The current state of theI ActionsSet. **/ function getCurrentState(uint256 actionsSetId) external view returns (ActionsSetState); /** - * @notice Returns whether an actions set (by actionHash) is queued - * @dev actionHash = keccak256(abi.encode(target, value, signature, data, executionTime, withDelegatecall)) - * @param actionHash hash of the action to be checked - * @return True if the underlying action of actionHash is queued, false otherwise + * @notice Returns whether an actions set (by actionHash) is queued. + * @dev actionHash = keccak256(abi.encode(target, value, signature, data, executionTime, withDelegatecall)). + * @param actionHash hash of the action to be checked. + * @return True if the underlying action of actionHash is queued, false otherwise. **/ function isActionQueued(bytes32 actionHash) external view returns (bool); diff --git a/test/CrosschainTestBase.sol b/test/CrosschainTestBase.sol index 33d5dc6..ebb211c 100644 --- a/test/CrosschainTestBase.sol +++ b/test/CrosschainTestBase.sol @@ -314,11 +314,11 @@ abstract contract CrosschainTestBase is Test { }); assertEq( - bridgeExecutor.getDelay(), + bridgeExecutor.delay(), defaultL2BridgeExecutorArgs.delay ); assertEq( - bridgeExecutor.getGracePeriod(), + bridgeExecutor.gracePeriod(), defaultL2BridgeExecutorArgs.gracePeriod ); assertEq( @@ -357,11 +357,11 @@ abstract contract CrosschainTestBase is Test { bridgeExecutor.execute(0); assertEq( - bridgeExecutor.getDelay(), + bridgeExecutor.delay(), newL2BridgeExecutorParams.delay ); assertEq( - bridgeExecutor.getGracePeriod(), + bridgeExecutor.gracePeriod(), newL2BridgeExecutorParams.gracePeriod ); assertEq( diff --git a/test/Executor.t.sol b/test/Executor.t.sol index f36c462..0de9055 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -65,8 +65,8 @@ contract ExecutorTestBase is Test { function setUp() public { executor = new Executor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD + delay_: DELAY, + gracePeriod_: GRACE_PERIOD }); executor.grantRole(executor.SUBMISSION_ROLE(), bridge); executor.grantRole(executor.GUARDIAN_ROLE(), guardian); @@ -154,13 +154,13 @@ contract ExecutorConstructorTests is ExecutorTestBase { function test_constructor_invalidInitParams_boundary() public { vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); executor = new Executor({ - delay: DELAY, - gracePeriod: 10 minutes - 1 + delay_: DELAY, + gracePeriod_: 10 minutes - 1 }); executor = new Executor({ - delay: DELAY, - gracePeriod: 10 minutes + delay_: DELAY, + gracePeriod_: 10 minutes }); } @@ -170,12 +170,12 @@ contract ExecutorConstructorTests is ExecutorTestBase { vm.expectEmit(); emit GracePeriodUpdate(0, GRACE_PERIOD); executor = new Executor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD + delay_: DELAY, + gracePeriod_: GRACE_PERIOD }); - assertEq(executor.getDelay(), DELAY); - assertEq(executor.getGracePeriod(), GRACE_PERIOD); + assertEq(executor.delay(), DELAY); + assertEq(executor.gracePeriod(), GRACE_PERIOD); assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(this)), true); assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(executor)), true); @@ -231,7 +231,7 @@ contract ExecutorQueueTests is ExecutorTestBase { bytes32 actionHash1 = _encodeHash(action, block.timestamp + DELAY); bytes32 actionHash2 = _encodeHash(action, block.timestamp + DELAY + 1); - assertEq(executor.getActionsSetCount(), 0); + assertEq(executor.actionsSetCount(), 0); assertEq(executor.isActionQueued(actionHash1), false); assertEq(executor.isActionQueued(actionHash2), false); @@ -247,7 +247,7 @@ contract ExecutorQueueTests is ExecutorTestBase { ); _queueAction(action); - assertEq(executor.getActionsSetCount(), 1); + assertEq(executor.actionsSetCount(), 1); assertEq(executor.isActionQueued(actionHash1), true); assertEq(executor.isActionQueued(actionHash2), false); _assertActionSet({ @@ -272,7 +272,7 @@ contract ExecutorQueueTests is ExecutorTestBase { ); _queueAction(action); - assertEq(executor.getActionsSetCount(), 2); + assertEq(executor.actionsSetCount(), 2); assertEq(executor.isActionQueued(actionHash1), true); assertEq(executor.isActionQueued(actionHash2), true); _assertActionSet({ @@ -289,14 +289,14 @@ contract ExecutorQueueTests is ExecutorTestBase { contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_actionsSetIdTooHigh_boundary() public { - assertEq(executor.getActionsSetCount(), 0); + assertEq(executor.actionsSetCount(), 0); vm.expectRevert(abi.encodeWithSignature("InvalidActionsSetId()")); executor.execute(0); _queueAction(); skip(DELAY); - assertEq(executor.getActionsSetCount(), 1); + assertEq(executor.actionsSetCount(), 1); executor.execute(0); } @@ -304,7 +304,7 @@ contract ExecutorExecuteTests is ExecutorTestBase { _queueAction(); vm.prank(guardian); executor.cancel(0); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); executor.execute(0); } @@ -314,7 +314,7 @@ contract ExecutorExecuteTests is ExecutorTestBase { skip(DELAY); executor.execute(0); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); executor.execute(0); } @@ -322,7 +322,7 @@ contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_notQueued_expired_boundary() public { _queueAction(); skip(DELAY + GRACE_PERIOD + 1); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); executor.execute(0); @@ -334,7 +334,7 @@ contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_timelockNotFinished_boundary() public { _queueAction(); skip(DELAY - 1); - + vm.expectRevert(abi.encodeWithSignature("TimelockNotFinished()")); executor.execute(0); @@ -536,7 +536,7 @@ contract ExecutorCancelTests is ExecutorTestBase { } function test_cancel_actionsSetIdTooHigh_boundary() public { - assertEq(executor.getActionsSetCount(), 0); + assertEq(executor.actionsSetCount(), 0); vm.expectRevert(abi.encodeWithSignature("InvalidActionsSetId()")); vm.prank(guardian); executor.cancel(0); @@ -544,7 +544,7 @@ contract ExecutorCancelTests is ExecutorTestBase { _queueAction(); skip(DELAY); - assertEq(executor.getActionsSetCount(), 1); + assertEq(executor.actionsSetCount(), 1); vm.prank(guardian); executor.cancel(0); } @@ -553,7 +553,7 @@ contract ExecutorCancelTests is ExecutorTestBase { _queueAction(); vm.prank(guardian); executor.cancel(0); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); vm.prank(guardian); executor.cancel(0); @@ -564,7 +564,7 @@ contract ExecutorCancelTests is ExecutorTestBase { skip(DELAY); executor.execute(0); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); vm.prank(guardian); executor.cancel(0); @@ -573,7 +573,7 @@ contract ExecutorCancelTests is ExecutorTestBase { function test_cancel_notQueued_expired_boundary() public { _queueAction(); skip(DELAY + GRACE_PERIOD + 1); - + vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); vm.prank(guardian); executor.cancel(0); @@ -613,14 +613,14 @@ contract ExecutorUpdateTests is ExecutorTestBase { } function test_updateDelay() public { - assertEq(executor.getDelay(), 1 days); + assertEq(executor.delay(), 1 days); vm.expectEmit(address(executor)); emit DelayUpdate(1 days, 2 days); vm.prank(address(executor)); executor.updateDelay(2 days); - assertEq(executor.getDelay(), 2 days); + assertEq(executor.delay(), 2 days); } function test_updateGracePeriod_notSelf() public { @@ -638,14 +638,14 @@ contract ExecutorUpdateTests is ExecutorTestBase { } function test_updateGracePeriod() public { - assertEq(executor.getGracePeriod(), 30 days); + assertEq(executor.gracePeriod(), 30 days); vm.expectEmit(address(executor)); emit GracePeriodUpdate(30 days, 60 days); vm.prank(address(executor)); executor.updateGracePeriod(60 days); - assertEq(executor.getGracePeriod(), 60 days); + assertEq(executor.gracePeriod(), 60 days); } } From 01ed7995f3e6a9e2f413150ebccdf13549f5ed94 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Thu, 13 Jun 2024 11:02:11 -0400 Subject: [PATCH 5/6] feat: add section labels, reorder --- src/Executor.sol | 56 +++++++++++++++------ src/interfaces/IExecutor.sol | 97 ++++++++++++++++++++++++------------ 2 files changed, 106 insertions(+), 47 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index aee8d06..6e1bb09 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -7,7 +7,7 @@ import { Address } from "openzeppelin-contracts/contracts/utils/Address.so import { IExecutor } from './interfaces/IExecutor.sol'; /** - * @title Executor + * @title Executor * @author Aave * @notice Executor which queues up message calls and executes them after an optional delay */ @@ -15,6 +15,10 @@ contract Executor is IExecutor, AccessControl { using Address for address; + /******************************************************************************************************************/ + /*** State variables and constructor ***/ + /******************************************************************************************************************/ + bytes32 public constant SUBMISSION_ROLE = keccak256('SUBMISSION_ROLE'); bytes32 public constant GUARDIAN_ROLE = keccak256('GUARDIAN_ROLE'); @@ -31,8 +35,8 @@ contract Executor is IExecutor, AccessControl { /** * @dev Constructor - * @param delay_ The delay before which an actions set can be executed - * @param gracePeriod_ The time period after a delay during which an actions set can be executed + * @param delay_ The delay before which an actions set can be executed. + * @param gracePeriod_ The time period after a delay during which an actions set can be executed. */ constructor( uint256 delay_, @@ -53,6 +57,10 @@ contract Executor is IExecutor, AccessControl { _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); } + /******************************************************************************************************************/ + /*** ActionSet functions ***/ + /******************************************************************************************************************/ + /// @inheritdoc IExecutor function queue( address[] memory targets, @@ -74,9 +82,7 @@ contract Executor is IExecutor, AccessControl { uint256 actionsSetId = actionsSetCount; uint256 executionTime = block.timestamp + delay; - unchecked { - ++actionsSetCount; - } + unchecked { ++actionsSetCount; } for (uint256 i = 0; i < targetsLength; ++i) { bytes32 actionHash = keccak256( @@ -163,6 +169,10 @@ contract Executor is IExecutor, AccessControl { emit ActionsSetCanceled(actionsSetId); } + /******************************************************************************************************************/ + /*** Admin functions ***/ + /******************************************************************************************************************/ + /// @inheritdoc IExecutor function updateDelay(uint256 newDelay) external override onlyRole(DEFAULT_ADMIN_ROLE) { _updateDelay(newDelay); @@ -176,6 +186,10 @@ contract Executor is IExecutor, AccessControl { _updateGracePeriod(newGracePeriod); } + /******************************************************************************************************************/ + /*** External misc functions ***/ + /******************************************************************************************************************/ + /// @inheritdoc IExecutor function executeDelegateCall(address target, bytes calldata data) external @@ -190,6 +204,10 @@ contract Executor is IExecutor, AccessControl { /// @inheritdoc IExecutor function receiveFunds() external payable override {} + /******************************************************************************************************************/ + /*** External view functions ***/ + /******************************************************************************************************************/ + /// @inheritdoc IExecutor function getActionsSetById(uint256 actionsSetId) external @@ -212,15 +230,9 @@ contract Executor is IExecutor, AccessControl { else return ActionsSetState.Queued; } - function _updateDelay(uint256 newDelay) internal { - emit DelayUpdate(delay, newDelay); - delay = newDelay; - } - - function _updateGracePeriod(uint256 newGracePeriod) internal { - emit GracePeriodUpdate(gracePeriod, newGracePeriod); - gracePeriod = newGracePeriod; - } + /******************************************************************************************************************/ + /*** Internal ActionSet helper functions ***/ + /******************************************************************************************************************/ function _executeTransaction( address target, @@ -257,4 +269,18 @@ contract Executor is IExecutor, AccessControl { isActionQueued[actionHash] = false; } + /******************************************************************************************************************/ + /*** Internal admin helper functions ***/ + /******************************************************************************************************************/ + + function _updateDelay(uint256 newDelay) internal { + emit DelayUpdate(delay, newDelay); + delay = newDelay; + } + + function _updateGracePeriod(uint256 newGracePeriod) internal { + emit GracePeriodUpdate(gracePeriod, newGracePeriod); + gracePeriod = newGracePeriod; + } + } diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index c37a2f2..1869cf2 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -4,12 +4,16 @@ pragma solidity >=0.8.0; import { IAccessControl } from 'openzeppelin-contracts/contracts/access/IAccessControl.sol'; /** - * @title IExecutor + * @title IExecutor * @author Aave * @notice Defines the interface for the Executor */ interface IExecutor is IAccessControl { + /******************************************************************************************************************/ + /*** Errors ***/ + /******************************************************************************************************************/ + error InvalidInitParams(); error GracePeriodTooShort(); error OnlyQueuedActions(); @@ -20,6 +24,10 @@ interface IExecutor is IAccessControl { error DuplicateAction(); error InsufficientBalance(); + /******************************************************************************************************************/ + /*** Enums ***/ + /******************************************************************************************************************/ + /** * @notice This enum contains all possible actions set states */ @@ -30,6 +38,10 @@ interface IExecutor is IAccessControl { Expired } + /******************************************************************************************************************/ + /*** Events ***/ + /******************************************************************************************************************/ + /** * @notice This struct contains the data needed to execute a specified set of actions. * @param targets Array of targets to call. @@ -104,6 +116,10 @@ interface IExecutor is IAccessControl { **/ event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); + /******************************************************************************************************************/ + /*** State variables ***/ + /******************************************************************************************************************/ + /** * @notice The role that allows submission of a queued action. **/ @@ -119,6 +135,36 @@ interface IExecutor is IAccessControl { */ function MINIMUM_GRACE_PERIOD() external view returns (uint256); + /** + * @notice Returns the total number of actions sets of the executor. + * @return The number of actions sets. + **/ + function actionsSetCount() external view returns (uint256); + + /** + * @notice Returns the delay (between queuing and execution) + * @return The value of the delay (in seconds) + **/ + function delay() external view returns (uint256); + + /** + * @notice Time after the execution time during which the actions set can be executed. + * @return The value of the grace period (in seconds) + **/ + function gracePeriod() external view returns (uint256); + + /** + * @notice Returns whether an actions set (by actionHash) is queued. + * @dev actionHash = keccak256(abi.encode(target, value, signature, data, executionTime, withDelegatecall)). + * @param actionHash hash of the action to be checked. + * @return True if the underlying action of actionHash is queued, false otherwise. + **/ + function isActionQueued(bytes32 actionHash) external view returns (bool); + + /******************************************************************************************************************/ + /*** ActionSet functions ***/ + /******************************************************************************************************************/ + /** * @notice Queue an ActionsSet. * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise. @@ -148,6 +194,10 @@ interface IExecutor is IAccessControl { **/ function cancel(uint256 actionsSetId) external; + /******************************************************************************************************************/ + /*** Admin functions ***/ + /******************************************************************************************************************/ + /** * @notice Update the delay, time between queueing and execution of ActionsSet. * @dev It does not affect to actions set that are already queued. @@ -161,6 +211,10 @@ interface IExecutor is IAccessControl { **/ function updateGracePeriod(uint256 gracePeriod) external; + /******************************************************************************************************************/ + /*** Misc functions ***/ + /******************************************************************************************************************/ + /** * @notice Allows to delegatecall a given target with an specific amount of value. * @dev This function is external so it allows to specify a defined msg.value for the delegate call, reducing @@ -178,44 +232,23 @@ interface IExecutor is IAccessControl { */ function receiveFunds() external payable; - /** - * @notice Returns the delay (between queuing and execution) - * @return The value of the delay (in seconds) - **/ - function delay() external view returns (uint256); + /******************************************************************************************************************/ + /*** View functions ***/ + /******************************************************************************************************************/ - /** - * @notice Time after the execution time during which the actions set can be executed. - * @return The value of the grace period (in seconds) - **/ - function gracePeriod() external view returns (uint256); - - /** - * @notice Returns the total number of actions sets of the executor. - * @return The number of actions sets. - **/ - function actionsSetCount() external view returns (uint256); /** * @notice Returns the data of an actions set. * @param actionsSetId The id of the ActionsSet. * @return The data of the ActionsSet. **/ - function getActionsSetById(uint256 actionsSetId) external view returns (ActionsSet memory); - - /** - * @notice Returns the current state of an actions set. - * @param actionsSetId The id of the ActionsSet. - * @return The current state of theI ActionsSet. - **/ - function getCurrentState(uint256 actionsSetId) external view returns (ActionsSetState); + function getActionsSetById(uint256 actionsSetId) external view returns (ActionsSet memory); - /** - * @notice Returns whether an actions set (by actionHash) is queued. - * @dev actionHash = keccak256(abi.encode(target, value, signature, data, executionTime, withDelegatecall)). - * @param actionHash hash of the action to be checked. - * @return True if the underlying action of actionHash is queued, false otherwise. - **/ - function isActionQueued(bytes32 actionHash) external view returns (bool); + /** + * @notice Returns the current state of an actions set. + * @param actionsSetId The id of the ActionsSet. + * @return The current state of theI ActionsSet. + **/ + function getCurrentState(uint256 actionsSetId) external view returns (ActionsSetState); } From 5fd71628da7e62f11784e55953a08ba3930cf970 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Fri, 14 Jun 2024 09:25:01 -0400 Subject: [PATCH 6/6] chore: reduce diff --- src/Executor.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index 6369379..e0524e1 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -52,9 +52,8 @@ contract Executor is IExecutor, AccessControl { _setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE); _setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE); - // Necessary for self-referential calls to change configuration - _grantRole(DEFAULT_ADMIN_ROLE, address(this)); _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); + _grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration } /******************************************************************************************************************/ @@ -151,7 +150,7 @@ contract Executor is IExecutor, AccessControl { function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); - ActionsSet storage actionsSet =_actionsSets[actionsSetId]; + ActionsSet storage actionsSet = _actionsSets[actionsSetId]; actionsSet.canceled = true; uint256 targetsLength = actionsSet.targets.length;