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..ccb29f8 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(); @@ -66,11 +65,11 @@ contract ExecutorTestBase is Test { function setUp() public { executor = new Executor({ - delay: DELAY, - gracePeriod: GRACE_PERIOD, - guardian: guardian + 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)); } @@ -155,15 +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, - guardian: guardian + delay: DELAY, + gracePeriod: 10 minutes - 1 }); executor = new Executor({ - delay: DELAY, - gracePeriod: 10 minutes, - guardian: guardian + delay: DELAY, + 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 + 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 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; }