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 75% rename from src/executors/BridgeExecutorBase.sol rename to src/Executor.sol index 90602ce..f886466 100644 --- a/src/executors/BridgeExecutorBase.sol +++ b/src/Executor.sol @@ -1,21 +1,23 @@ // 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; + 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; @@ -23,8 +25,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { 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; @@ -33,33 +33,15 @@ abstract contract BridgeExecutorBase is IExecutorBase { // 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 @@ -67,10 +49,75 @@ abstract contract BridgeExecutorBase is IExecutorBase { _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 + function queue( + address[] memory targets, + uint256[] memory values, + string[] memory signatures, + bytes[] memory calldatas, + bool[] memory withDelegatecalls + ) external override onlyRole(SUBMISSION_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 IExecutorBase + /// @inheritdoc IExecutor function execute(uint256 actionsSetId) external payable override { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); @@ -98,8 +145,8 @@ abstract contract BridgeExecutorBase is IExecutorBase { emit ActionsSetExecuted(actionsSetId, msg.sender, returnedData); } - /// @inheritdoc IExecutorBase - function cancel(uint256 actionsSetId) external override onlyGuardian { + /// @inheritdoc IExecutor + function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) { if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions(); ActionsSet storage actionsSet = _actionsSets[actionsSetId]; @@ -123,57 +170,47 @@ abstract contract BridgeExecutorBase is IExecutorBase { emit ActionsSetCanceled(actionsSetId); } - /// @inheritdoc IExecutorBase - function updateGuardian(address guardian) external override onlyThis { - _updateGuardian(guardian); - } - - /// @inheritdoc IExecutorBase - function updateDelay(uint256 delay) external override onlyThis { + /// @inheritdoc IExecutor + function updateDelay(uint256 delay) external override onlyRole(DEFAULT_ADMIN_ROLE) { _updateDelay(delay); } - /// @inheritdoc IExecutorBase - function updateGracePeriod(uint256 gracePeriod) external override onlyThis { + /// @inheritdoc IExecutor + function updateGracePeriod(uint256 gracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) { if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); _updateGracePeriod(gracePeriod); } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function executeDelegateCall(address target, bytes calldata data) external payable override - onlyThis + onlyRole(DEFAULT_ADMIN_ROLE) returns (bytes memory) { 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 - 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 +220,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,16 +235,11 @@ abstract contract BridgeExecutorBase is IExecutorBase { } } - /// @inheritdoc IExecutorBase + /// @inheritdoc IExecutor function isActionQueued(bytes32 actionHash) public view override returns (bool) { 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; @@ -218,74 +250,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 83% rename from src/interfaces/IExecutorBase.sol rename to src/interfaces/IExecutor.sol index d7290b0..85a53c9 100644 --- a/src/interfaces/IExecutorBase.sol +++ b/src/interfaces/IExecutor.sol @@ -1,20 +1,17 @@ // 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(); - error MinimumDelayTooLong(); - error MaximumDelayTooShort(); error GracePeriodTooShort(); - error DelayShorterThanMin(); - error DelayLongerThanMax(); error OnlyQueuedActions(); error TimelockNotFinished(); error InvalidActionsSetId(); @@ -22,7 +19,6 @@ interface IExecutorBase { error InconsistentParamsLength(); error DuplicateAction(); error InsufficientBalance(); - error FailedActionExecution(); /** * @notice This enum contains all possible actions set states @@ -94,13 +90,6 @@ interface IExecutorBase { **/ 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 @@ -115,6 +104,33 @@ interface IExecutorBase { **/ 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 + * @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 @@ -127,12 +143,6 @@ interface IExecutorBase { **/ 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 @@ -175,12 +185,6 @@ interface IExecutorBase { **/ 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 @@ -208,4 +212,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..c8afd68 100644 --- a/test/ArbitrumOneCrosschainTest.t.sol +++ b/test/ArbitrumOneCrosschainTest.t.sol @@ -43,16 +43,17 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + 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 b87f747..ec4169c 100644 --- a/test/BaseChainCrosschainTest.t.sol +++ b/test/BaseChainCrosschainTest.t.sol @@ -41,16 +41,17 @@ contract BaseChainCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + 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 7249ec5..33d5dc6 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); } @@ -307,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 @@ -316,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 )); @@ -361,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/AuthBridgeExecutor.t.sol b/test/Executor.t.sol similarity index 87% rename from test/AuthBridgeExecutor.t.sol rename to test/Executor.t.sol index d7fb049..ccb29f8 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; @@ -51,7 +51,6 @@ contract AuthBridgeExecutorTestBase 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(); @@ -62,15 +61,15 @@ contract AuthBridgeExecutorTestBase is Test { address bridge = makeAddr("bridge"); address guardian = makeAddr("guardian"); - AuthBridgeExecutor executor; + Executor executor; function setUp() public { - executor = new AuthBridgeExecutor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD, - guardian: guardian + executor = new Executor({ + delay: DELAY, + 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)); } @@ -137,7 +136,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,20 +149,18 @@ 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({ - delay: DELAY, - gracePeriod: 10 minutes - 1, - guardian: guardian + executor = new Executor({ + delay: DELAY, + gracePeriod: 10 minutes - 1 }); - executor = new AuthBridgeExecutor({ - delay: DELAY, - gracePeriod: 10 minutes, - guardian: guardian + executor = new Executor({ + delay: DELAY, + gracePeriod: 10 minutes }); } @@ -172,27 +169,24 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { emit DelayUpdate(0, DELAY); vm.expectEmit(); emit GracePeriodUpdate(0, GRACE_PERIOD); - vm.expectEmit(); - emit GuardianUpdate(address(0), guardian); - executor = new AuthBridgeExecutor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD, - guardian: guardian + executor = new Executor({ + delay: DELAY, + 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 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())); + 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)); } @@ -292,7 +286,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 +433,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 +445,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 +457,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 +469,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 +482,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 +494,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 +508,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,18 +520,18 @@ 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(); skip(DELAY); - vm.expectRevert(abi.encodeWithSignature("NotGuardian()")); + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.GUARDIAN_ROLE())); executor.cancel(0); } @@ -597,7 +591,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,33 +600,15 @@ 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 { - - 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); - } +contract ExecutorUpdateTests is ExecutorTestBase { 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 AuthBridgeExecutorUpdateTests is AuthBridgeExecutorTestBase { } 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); } @@ -674,10 +650,10 @@ contract AuthBridgeExecutorUpdateTests is AuthBridgeExecutorTestBase { } -contract AuthBridgeExecutorMiscTests is AuthBridgeExecutorTestBase { +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 1707e8e..e194e66 100644 --- a/test/GnosisCrosschainTest.t.sol +++ b/test/GnosisCrosschainTest.t.sol @@ -40,10 +40,9 @@ contract GnosisCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + 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 6895e1e..a3a6bec 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -41,16 +41,17 @@ contract OptimismCrosschainTest is CrosschainTestBase { ); bridge.destination.selectFork(); - bridgeExecutor = new AuthBridgeExecutor( + 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 74f0100..7fcd178 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'; @@ -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 { - 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)).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; }