From 25ddc69324bfdd32946ca48643bc15190135ec0f Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Tue, 11 Jun 2024 16:27:24 +0900 Subject: [PATCH 1/2] remove min and max delay (#15) --- src/executors/AuthBridgeExecutor.sol | 6 --- src/executors/BridgeExecutorBase.sol | 55 +-------------------------- src/interfaces/IExecutorBase.sol | 38 ------------------ test/ArbitrumOneCrosschainTest.t.sol | 2 - test/AuthBridgeExecutor.t.sol | 8 ---- test/BaseChainCrosschainTest.t.sol | 2 - test/CrosschainTestBase.sol | 24 ------------ test/GnosisCrosschainTest.t.sol | 2 - test/OptimismCrosschainTest.t.sol | 2 - test/mocks/ReconfigurationPayload.sol | 16 -------- 10 files changed, 1 insertion(+), 154 deletions(-) diff --git a/src/executors/AuthBridgeExecutor.sol b/src/executors/AuthBridgeExecutor.sol index 72db024..bde9190 100644 --- a/src/executors/AuthBridgeExecutor.sol +++ b/src/executors/AuthBridgeExecutor.sol @@ -19,22 +19,16 @@ contract AuthBridgeExecutor is IAuthBridgeExecutor, AccessControl, BridgeExecuto * * @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 minimumDelay The minimum bound a delay can be set to - * @param maximumDelay The maximum bound a delay can be set to * @param guardian The address of the guardian, which can cancel queued proposals (can be zero) */ constructor( uint256 delay, uint256 gracePeriod, - uint256 minimumDelay, - uint256 maximumDelay, address guardian ) BridgeExecutorBase( delay, gracePeriod, - minimumDelay, - maximumDelay, guardian ) { diff --git a/src/executors/BridgeExecutorBase.sol b/src/executors/BridgeExecutorBase.sol index f2d8f3a..8d156ee 100644 --- a/src/executors/BridgeExecutorBase.sol +++ b/src/executors/BridgeExecutorBase.sol @@ -18,10 +18,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; - // Minimum allowed delay - uint256 private _minimumDelay; - // Maximum allowed delay - uint256 private _maximumDelay; // Address with the ability of canceling actions sets address private _guardian; @@ -53,28 +49,19 @@ abstract contract BridgeExecutorBase is IExecutorBase { * * @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 minimumDelay The minimum bound a delay can be set to - * @param maximumDelay The maximum bound a delay can be set to * @param guardian The address of the guardian, which can cancel queued proposals (can be zero) */ constructor( uint256 delay, uint256 gracePeriod, - uint256 minimumDelay, - uint256 maximumDelay, address guardian ) { if ( - gracePeriod < MINIMUM_GRACE_PERIOD || - minimumDelay >= maximumDelay || - delay < minimumDelay || - delay > maximumDelay + gracePeriod < MINIMUM_GRACE_PERIOD ) revert InvalidInitParams(); _updateDelay(delay); _updateGracePeriod(gracePeriod); - _updateMinimumDelay(minimumDelay); - _updateMaximumDelay(maximumDelay); _updateGuardian(guardian); } @@ -138,7 +125,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { /// @inheritdoc IExecutorBase function updateDelay(uint256 delay) external override onlyThis { - _validateDelay(delay); _updateDelay(delay); } @@ -148,20 +134,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { _updateGracePeriod(gracePeriod); } - /// @inheritdoc IExecutorBase - function updateMinimumDelay(uint256 minimumDelay) external override onlyThis { - if (minimumDelay >= _maximumDelay) revert MinimumDelayTooLong(); - _updateMinimumDelay(minimumDelay); - _validateDelay(_delay); - } - - /// @inheritdoc IExecutorBase - function updateMaximumDelay(uint256 maximumDelay) external override onlyThis { - if (maximumDelay <= _minimumDelay) revert MaximumDelayTooShort(); - _updateMaximumDelay(maximumDelay); - _validateDelay(_delay); - } - /// @inheritdoc IExecutorBase function executeDelegateCall(address target, bytes calldata data) external @@ -190,16 +162,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { return _gracePeriod; } - /// @inheritdoc IExecutorBase - function getMinimumDelay() external view override returns (uint256) { - return _minimumDelay; - } - - /// @inheritdoc IExecutorBase - function getMaximumDelay() external view override returns (uint256) { - return _maximumDelay; - } - /// @inheritdoc IExecutorBase function getGuardian() external view override returns (address) { return _guardian; @@ -255,16 +217,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { _gracePeriod = gracePeriod; } - function _updateMinimumDelay(uint256 minimumDelay) internal { - emit MinimumDelayUpdate(_minimumDelay, minimumDelay); - _minimumDelay = minimumDelay; - } - - function _updateMaximumDelay(uint256 maximumDelay) internal { - emit MaximumDelayUpdate(_maximumDelay, maximumDelay); - _maximumDelay = maximumDelay; - } - /** * @notice Queue an ActionsSet * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise @@ -380,11 +332,6 @@ abstract contract BridgeExecutorBase is IExecutorBase { _queuedActions[actionHash] = false; } - function _validateDelay(uint256 delay) internal view { - if (delay < _minimumDelay) revert DelayShorterThanMin(); - if (delay > _maximumDelay) revert DelayLongerThanMax(); - } - function _verifyCallResult(bool success, bytes memory returnData) private pure returns (bytes memory) { diff --git a/src/interfaces/IExecutorBase.sol b/src/interfaces/IExecutorBase.sol index 20fc7dc..9f4d182 100644 --- a/src/interfaces/IExecutorBase.sol +++ b/src/interfaces/IExecutorBase.sol @@ -115,20 +115,6 @@ interface IExecutorBase { **/ event GracePeriodUpdate(uint256 oldGracePeriod, uint256 newGracePeriod); - /** - * @dev Emitted when the minimum delay (lower bound of delay) is updated - * @param oldMinimumDelay The value of the old minimum delay - * @param newMinimumDelay The value of the new minimum delay - **/ - event MinimumDelayUpdate(uint256 oldMinimumDelay, uint256 newMinimumDelay); - - /** - * @dev Emitted when the maximum delay (upper bound of delay)is updated - * @param oldMaximumDelay The value of the old maximum delay - * @param newMaximumDelay The value of the new maximum delay - **/ - event MaximumDelayUpdate(uint256 oldMaximumDelay, uint256 newMaximumDelay); - /** * @notice Execute the ActionsSet * @param actionsSetId The id of the ActionsSet to execute @@ -160,18 +146,6 @@ interface IExecutorBase { **/ function updateGracePeriod(uint256 gracePeriod) external; - /** - * @notice Update the minimum allowed delay - * @param minimumDelay The value of the minimum delay (in seconds) - **/ - function updateMinimumDelay(uint256 minimumDelay) external; - - /** - * @notice Update the maximum allowed delay - * @param maximumDelay The maximum delay (in seconds) - **/ - function updateMaximumDelay(uint256 maximumDelay) 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 @@ -202,18 +176,6 @@ interface IExecutorBase { **/ function getGracePeriod() external view returns (uint256); - /** - * @notice Returns the minimum delay - * @return The value of the minimum delay (in seconds) - **/ - function getMinimumDelay() external view returns (uint256); - - /** - * @notice Returns the maximum delay - * @return The value of the maximum delay (in seconds) - **/ - function getMaximumDelay() external view returns (uint256); - /** * @notice Returns the address of the guardian * @return The address of the guardian diff --git a/test/ArbitrumOneCrosschainTest.t.sol b/test/ArbitrumOneCrosschainTest.t.sol index d285664..f488829 100644 --- a/test/ArbitrumOneCrosschainTest.t.sol +++ b/test/ArbitrumOneCrosschainTest.t.sol @@ -46,8 +46,6 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase { bridgeExecutor = new AuthBridgeExecutor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.minimumDelay, - defaultL2BridgeExecutorArgs.maximumDelay, defaultL2BridgeExecutorArgs.guardian ); bridgeReceiver = address(new BridgeExecutorReceiverArbitrum( diff --git a/test/AuthBridgeExecutor.t.sol b/test/AuthBridgeExecutor.t.sol index 5f6b752..d19d1ce 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/AuthBridgeExecutor.t.sol @@ -68,8 +68,6 @@ contract AuthBridgeExecutorTestBase is Test { executor = new AuthBridgeExecutor({ delay: DELAY, gracePeriod: GRACE_PERIOD, - minimumDelay: 0, // TODO: removing this in next PR - maximumDelay: 365 days, // TODO: removing this in next PR guardian: guardian }); executor.grantRole(executor.AUTHORIZED_BRIDGE_ROLE(), bridge); @@ -160,16 +158,12 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { executor = new AuthBridgeExecutor({ delay: DELAY, gracePeriod: 10 minutes - 1, - minimumDelay: 0, - maximumDelay: 365 days, guardian: guardian }); executor = new AuthBridgeExecutor({ delay: DELAY, gracePeriod: 10 minutes, - minimumDelay: 0, - maximumDelay: 365 days, guardian: guardian }); } @@ -184,8 +178,6 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { executor = new AuthBridgeExecutor({ delay: DELAY, gracePeriod: GRACE_PERIOD, - minimumDelay: 0, - maximumDelay: 365 days, guardian: guardian }); diff --git a/test/BaseChainCrosschainTest.t.sol b/test/BaseChainCrosschainTest.t.sol index 820b915..4cf4f43 100644 --- a/test/BaseChainCrosschainTest.t.sol +++ b/test/BaseChainCrosschainTest.t.sol @@ -44,8 +44,6 @@ contract BaseChainCrosschainTest is CrosschainTestBase { bridgeExecutor = new AuthBridgeExecutor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.minimumDelay, - defaultL2BridgeExecutorArgs.maximumDelay, defaultL2BridgeExecutorArgs.guardian ); bridgeReceiver = address(new BridgeExecutorReceiverOptimism( diff --git a/test/CrosschainTestBase.sol b/test/CrosschainTestBase.sol index b43f77c..6fc2261 100644 --- a/test/CrosschainTestBase.sol +++ b/test/CrosschainTestBase.sol @@ -20,8 +20,6 @@ struct L2BridgeExecutorArguments { address ethereumGovernanceExecutor; uint256 delay; uint256 gracePeriod; - uint256 minimumDelay; - uint256 maximumDelay; address guardian; } @@ -72,8 +70,6 @@ abstract contract CrosschainTestBase is Test { ethereumGovernanceExecutor: L1_EXECUTOR, delay: 600, gracePeriod: 1200, - minimumDelay: 0, - maximumDelay: 2400, guardian: GUARDIAN }); @@ -316,14 +312,6 @@ abstract contract CrosschainTestBase is Test { bridgeExecutor.getGracePeriod(), defaultL2BridgeExecutorArgs.gracePeriod ); - assertEq( - bridgeExecutor.getMinimumDelay(), - defaultL2BridgeExecutorArgs.minimumDelay - ); - assertEq( - bridgeExecutor.getMaximumDelay(), - defaultL2BridgeExecutorArgs.maximumDelay - ); assertEq( bridgeExecutor.getGuardian(), defaultL2BridgeExecutorArgs.guardian @@ -333,16 +321,12 @@ abstract contract CrosschainTestBase is Test { ethereumGovernanceExecutor: defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, delay: 1200, gracePeriod: 1800, - minimumDelay: 100, - maximumDelay: 3600, guardian: makeAddr('newGuardian') }); IPayload reconfigurationPayload = IPayload(new ReconfigurationPayload( newL2BridgeExecutorParams.delay, newL2BridgeExecutorParams.gracePeriod, - newL2BridgeExecutorParams.minimumDelay, - newL2BridgeExecutorParams.maximumDelay, newL2BridgeExecutorParams.guardian )); @@ -373,14 +357,6 @@ abstract contract CrosschainTestBase is Test { bridgeExecutor.getGracePeriod(), newL2BridgeExecutorParams.gracePeriod ); - assertEq( - bridgeExecutor.getMinimumDelay(), - newL2BridgeExecutorParams.minimumDelay - ); - assertEq( - bridgeExecutor.getMaximumDelay(), - newL2BridgeExecutorParams.maximumDelay - ); assertEq( bridgeExecutor.getGuardian(), newL2BridgeExecutorParams.guardian diff --git a/test/GnosisCrosschainTest.t.sol b/test/GnosisCrosschainTest.t.sol index 9d2ccd8..4dc701b 100644 --- a/test/GnosisCrosschainTest.t.sol +++ b/test/GnosisCrosschainTest.t.sol @@ -46,8 +46,6 @@ contract GnosisCrosschainTest is CrosschainTestBase { bridgeExecutor = new AuthBridgeExecutor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.minimumDelay, - defaultL2BridgeExecutorArgs.maximumDelay, defaultL2BridgeExecutorArgs.guardian ); bridgeReceiver = address(new BridgeExecutorReceiverGnosis( diff --git a/test/OptimismCrosschainTest.t.sol b/test/OptimismCrosschainTest.t.sol index 21fe8f7..a330f59 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -44,8 +44,6 @@ contract OptimismCrosschainTest is CrosschainTestBase { bridgeExecutor = new AuthBridgeExecutor( defaultL2BridgeExecutorArgs.delay, defaultL2BridgeExecutorArgs.gracePeriod, - defaultL2BridgeExecutorArgs.minimumDelay, - defaultL2BridgeExecutorArgs.maximumDelay, defaultL2BridgeExecutorArgs.guardian ); bridgeReceiver = address(new BridgeExecutorReceiverOptimism( diff --git a/test/mocks/ReconfigurationPayload.sol b/test/mocks/ReconfigurationPayload.sol index 4e4e571..74f0100 100644 --- a/test/mocks/ReconfigurationPayload.sol +++ b/test/mocks/ReconfigurationPayload.sol @@ -12,29 +12,21 @@ contract ReconfigurationPayload is IPayload { uint256 public immutable newDelay; uint256 public immutable newGracePeriod; - uint256 public immutable newMinimumDelay; - uint256 public immutable newMaximumDelay; address public immutable newGuardian; constructor( uint256 _newDelay, uint256 _newGracePeriod, - uint256 _newMinimumDelay, - uint256 _newMaximumDelay, address _newGuardian ) { newDelay = _newDelay; newGracePeriod = _newGracePeriod; - newMinimumDelay = _newMinimumDelay; - newMaximumDelay = _newMaximumDelay; newGuardian = _newGuardian; } function execute() external override { IExecutorBase(address(this)).updateDelay(getNewDelay()); IExecutorBase(address(this)).updateGracePeriod(getNewGracePeriod()); - IExecutorBase(address(this)).updateMinimumDelay(getNewMinimumDelay()); - IExecutorBase(address(this)).updateMaximumDelay(getNewMaximumDelay()); IExecutorBase(address(this)).updateGuardian(getNewGuardian()); } @@ -46,14 +38,6 @@ contract ReconfigurationPayload is IPayload { return newGracePeriod; } - function getNewMinimumDelay() public view returns (uint256) { - return newMinimumDelay; - } - - function getNewMaximumDelay() public view returns (uint256) { - return newMaximumDelay; - } - function getNewGuardian() public view returns (address) { return newGuardian; } From 2fb0afc7131eb73b9ab5bee008ae6b8f6c1f8dfb Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Tue, 11 Jun 2024 16:31:40 +0900 Subject: [PATCH 2/2] remove bridge role (#16) --- src/executors/AuthBridgeExecutor.sol | 5 +---- test/ArbitrumOneCrosschainTest.t.sol | 2 +- test/AuthBridgeExecutor.t.sol | 10 ++++------ test/BaseChainCrosschainTest.t.sol | 2 +- test/GnosisCrosschainTest.t.sol | 2 +- test/OptimismCrosschainTest.t.sol | 2 +- 6 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/executors/AuthBridgeExecutor.sol b/src/executors/AuthBridgeExecutor.sol index bde9190..2772574 100644 --- a/src/executors/AuthBridgeExecutor.sol +++ b/src/executors/AuthBridgeExecutor.sol @@ -11,8 +11,6 @@ import { BridgeExecutorBase } from './BridgeExecutorBase.sol'; * @notice Queue up proposals from an authorized bridge. */ contract AuthBridgeExecutor is IAuthBridgeExecutor, AccessControl, BridgeExecutorBase { - - bytes32 public constant AUTHORIZED_BRIDGE_ROLE = keccak256('AUTHORIZED_BRIDGE_ROLE'); /** * @dev Constructor @@ -33,7 +31,6 @@ contract AuthBridgeExecutor is IAuthBridgeExecutor, AccessControl, BridgeExecuto ) { _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - _setRoleAdmin(AUTHORIZED_BRIDGE_ROLE, DEFAULT_ADMIN_ROLE); } /// @inheritdoc IAuthBridgeExecutor @@ -43,7 +40,7 @@ contract AuthBridgeExecutor is IAuthBridgeExecutor, AccessControl, BridgeExecuto string[] memory signatures, bytes[] memory calldatas, bool[] memory withDelegatecalls - ) external onlyRole(AUTHORIZED_BRIDGE_ROLE) { + ) external onlyRole(DEFAULT_ADMIN_ROLE) { _queue(targets, values, signatures, calldatas, withDelegatecalls); } diff --git a/test/ArbitrumOneCrosschainTest.t.sol b/test/ArbitrumOneCrosschainTest.t.sol index f488829..27f9628 100644 --- a/test/ArbitrumOneCrosschainTest.t.sol +++ b/test/ArbitrumOneCrosschainTest.t.sol @@ -52,7 +52,7 @@ contract ArbitrumOneCrosschainTest is CrosschainTestBase { defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, bridgeExecutor )); - bridgeExecutor.grantRole(bridgeExecutor.AUTHORIZED_BRIDGE_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); hostDomain.selectFork(); vm.deal(L1_EXECUTOR, 0.01 ether); diff --git a/test/AuthBridgeExecutor.t.sol b/test/AuthBridgeExecutor.t.sol index d19d1ce..b9290ae 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/AuthBridgeExecutor.t.sol @@ -70,9 +70,8 @@ contract AuthBridgeExecutorTestBase is Test { gracePeriod: GRACE_PERIOD, guardian: guardian }); - executor.grantRole(executor.AUTHORIZED_BRIDGE_ROLE(), bridge); - executor.grantRole(executor.DEFAULT_ADMIN_ROLE(), bridge); - executor.revokeRole(executor.DEFAULT_ADMIN_ROLE(), address(this)); + executor.grantRole(executor.DEFAULT_ADMIN_ROLE(), bridge); + executor.revokeRole(executor.DEFAULT_ADMIN_ROLE(), address(this)); } /******************************************************************************************************************/ @@ -186,15 +185,14 @@ contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { assertEq(executor.getGuardian(), guardian); assertEq(executor.hasRole(executor.DEFAULT_ADMIN_ROLE(), address(this)), true); - assertEq(executor.getRoleAdmin(executor.AUTHORIZED_BRIDGE_ROLE()), executor.DEFAULT_ADMIN_ROLE()); } } contract AuthBridgeExecutorQueueTests is AuthBridgeExecutorTestBase { - function test_queue_onlyBridge() public { - vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.AUTHORIZED_BRIDGE_ROLE())); + function test_queue_onlyDefaultAdmin() public { + vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), executor.DEFAULT_ADMIN_ROLE())); executor.queue(new address[](0), new uint256[](0), new string[](0), new bytes[](0), new bool[](0)); } diff --git a/test/BaseChainCrosschainTest.t.sol b/test/BaseChainCrosschainTest.t.sol index 4cf4f43..85f0924 100644 --- a/test/BaseChainCrosschainTest.t.sol +++ b/test/BaseChainCrosschainTest.t.sol @@ -50,7 +50,7 @@ contract BaseChainCrosschainTest is CrosschainTestBase { defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, bridgeExecutor )); - bridgeExecutor.grantRole(bridgeExecutor.AUTHORIZED_BRIDGE_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); hostDomain.selectFork(); } diff --git a/test/GnosisCrosschainTest.t.sol b/test/GnosisCrosschainTest.t.sol index 4dc701b..5ae376c 100644 --- a/test/GnosisCrosschainTest.t.sol +++ b/test/GnosisCrosschainTest.t.sol @@ -54,7 +54,7 @@ contract GnosisCrosschainTest is CrosschainTestBase { defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, bridgeExecutor )); - bridgeExecutor.grantRole(bridgeExecutor.AUTHORIZED_BRIDGE_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); hostDomain.selectFork(); } diff --git a/test/OptimismCrosschainTest.t.sol b/test/OptimismCrosschainTest.t.sol index a330f59..4fd3f77 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -50,7 +50,7 @@ contract OptimismCrosschainTest is CrosschainTestBase { defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor, bridgeExecutor )); - bridgeExecutor.grantRole(bridgeExecutor.AUTHORIZED_BRIDGE_ROLE(), bridgeReceiver); + bridgeExecutor.grantRole(bridgeExecutor.DEFAULT_ADMIN_ROLE(), bridgeReceiver); hostDomain.selectFork(); }