From ddc20afb5745330a7667e96c319dfb37c892329c Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Thu, 16 May 2024 15:09:43 +0900 Subject: [PATCH] review fixes --- test/AuthBridgeExecutor.t.sol | 252 ++++++++++++++++++++++-------- test/OptimismCrosschainTest.t.sol | 2 +- 2 files changed, 188 insertions(+), 66 deletions(-) diff --git a/test/AuthBridgeExecutor.t.sol b/test/AuthBridgeExecutor.t.sol index 26c81b5..81844f2 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/AuthBridgeExecutor.t.sol @@ -26,7 +26,7 @@ contract RevertingPayload { } } -contract AuthBridgeExecutorTest is Test { +contract AuthBridgeExecutorTestBase is Test { struct Action { address[] targets; @@ -36,6 +36,15 @@ contract AuthBridgeExecutorTest is Test { bool[] withDelegatecalls; } + event ActionsSetQueued( + uint256 indexed id, + address[] targets, + uint256[] values, + string[] signatures, + bytes[] calldatas, + bool[] withDelegatecalls, + uint256 executionTime + ); event ActionsSetExecuted( uint256 indexed id, address indexed initiatorExecution, @@ -68,7 +77,85 @@ contract AuthBridgeExecutorTest is Test { executor.revokeRole(executor.DEFAULT_ADMIN_ROLE(), address(this)); } - function test_constructor_invalidInitParams() public { + /******************************************************************************************************************/ + /*** Helper functions ***/ + /******************************************************************************************************************/ + + function _getDefaultAction() internal returns (Action memory) { + address[] memory targets = new address[](1); + targets[0] = address(new DefaultPayload()); + uint256[] memory values = new uint256[](1); + values[0] = 0; + string[] memory signatures = new string[](1); + signatures[0] = "execute()"; + bytes[] memory calldatas = new bytes[](1); + calldatas[0] = ""; + bool[] memory withDelegatecalls = new bool[](1); + withDelegatecalls[0] = true; + + return Action({ + targets: targets, + values: values, + signatures: signatures, + calldatas: calldatas, + withDelegatecalls: withDelegatecalls + }); + } + + function _queueAction(Action memory action) internal { + vm.prank(bridge); + executor.queue( + action.targets, + action.values, + action.signatures, + action.calldatas, + action.withDelegatecalls + ); + } + + function _queueAction() internal { + _queueAction(_getDefaultAction()); + } + + function _queueActionWithValue(uint256 value) internal { + Action memory action = _getDefaultAction(); + action.targets[0] = address(new PayablePayload()); + action.values[0] = value; + _queueAction(action); + } + + function _encodeHash(Action memory action, uint256 index, uint256 executionTime) internal pure returns (bytes32) { + return keccak256(abi.encode( + action.targets[index], + action.values[index], + action.signatures[index], + action.calldatas[index], + executionTime, + action.withDelegatecalls[index] + )); + } + + function _encodeHash(Action memory action, uint256 executionTime) internal pure returns (bytes32) { + return _encodeHash(action, 0, executionTime); + } + + function _assertActionSet(uint256 id, bool executed, bool canceled, uint256 executionTime, Action memory actions) internal view { + IExecutorBase.ActionsSet memory actionsSet = executor.getActionsSetById(id); + assertEq(actionsSet.targets, actions.targets); + assertEq(actionsSet.values, actions.values); + assertEq(actionsSet.signatures, actions.signatures); + assertEq(actionsSet.calldatas, actions.calldatas); + assertEq(actionsSet.withDelegatecalls, actions.withDelegatecalls); + assertEq(actionsSet.executionTime, executionTime); + assertEq(actionsSet.executed, executed); + assertEq(actionsSet.canceled, canceled); + } + +} + +contract AuthBridgeExecutorConstructorTests is AuthBridgeExecutorTestBase { + + function test_constructor_invalidInitParams_boundary() public { vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); executor = new AuthBridgeExecutor({ delay: DELAY, @@ -77,9 +164,23 @@ contract AuthBridgeExecutorTest is Test { maximumDelay: 365 days, guardian: guardian }); + + executor = new AuthBridgeExecutor({ + delay: DELAY, + gracePeriod: 10 minutes, + minimumDelay: 0, + maximumDelay: 365 days, + guardian: guardian + }); } function test_constructor() public { + vm.expectEmit(); + 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, @@ -93,9 +194,13 @@ contract AuthBridgeExecutorTest is Test { 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()); + 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())); executor.queue(new address[](0), new uint256[](0), new string[](0), new bytes[](0), new bool[](0)); @@ -146,21 +251,59 @@ contract AuthBridgeExecutorTest is Test { assertEq(executor.isActionQueued(actionHash1), false); assertEq(executor.isActionQueued(actionHash2), false); + vm.expectEmit(address(executor)); + emit ActionsSetQueued( + 0, + action.targets, + action.values, + action.signatures, + action.calldatas, + action.withDelegatecalls, + block.timestamp + DELAY + ); _queueAction(action); assertEq(executor.getActionsSetCount(), 1); assertEq(executor.isActionQueued(actionHash1), true); assertEq(executor.isActionQueued(actionHash2), false); + _assertActionSet({ + id: 0, + executed: false, + canceled: false, + executionTime: block.timestamp + DELAY, + actions: action + }); // Can queue up the same action 1 second later skip(1); + vm.expectEmit(address(executor)); + emit ActionsSetQueued( + 1, + action.targets, + action.values, + action.signatures, + action.calldatas, + action.withDelegatecalls, + block.timestamp + DELAY + ); _queueAction(action); assertEq(executor.getActionsSetCount(), 2); assertEq(executor.isActionQueued(actionHash1), true); assertEq(executor.isActionQueued(actionHash2), true); + _assertActionSet({ + id: 1, + executed: false, + canceled: false, + executionTime: block.timestamp + DELAY, + actions: action + }); } +} + +contract AuthBridgeExecutorExecutorTests is AuthBridgeExecutorTestBase { + function test_execute_actionsSetIdTooHigh_boundary() public { assertEq(executor.getActionsSetCount(), 0); vm.expectRevert(abi.encodeWithSignature("InvalidActionsSetId()")); @@ -204,7 +347,7 @@ contract AuthBridgeExecutorTest is Test { executor.execute(0); } - function test_execute_timelock_not_finished_boundary() public { + function test_execute_timelockNotFinished_boundary() public { _queueAction(); skip(DELAY - 1); @@ -216,7 +359,7 @@ contract AuthBridgeExecutorTest is Test { executor.execute(0); } - function test_execute_balance_too_low_boundary() public { + function test_execute_balanceTooLow_boundary() public { _queueActionWithValue(1 ether); skip(DELAY); @@ -324,6 +467,36 @@ contract AuthBridgeExecutorTest is Test { assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); } + function test_execute_callWithCalldata() public { + Action memory action = _getDefaultAction(); + action.signatures[0] = ""; + action.calldatas[0] = abi.encodeWithSignature("execute()"); + action.withDelegatecalls[0] = false; + bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); + _queueAction(action); + skip(DELAY); + + assertEq(executor.isActionQueued(actionHash), true); + assertEq(executor.getActionsSetById(0).executed, false); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Queued)); + + bytes[] memory returnedData = new bytes[](1); + returnedData[0] = ""; + vm.expectEmit(action.targets[0]); + emit TestEvent(); + vm.expectEmit(address(executor)); + emit ActionsSetExecuted(0, address(this), returnedData); + executor.execute(0); + + assertEq(executor.isActionQueued(actionHash), false); + assertEq(executor.getActionsSetById(0).executed, true); + assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Executed)); + } + +} + +contract AuthBridgeExecutorCancelTests is AuthBridgeExecutorTestBase { + function test_cancel_notGuardian() public { _queueAction(); skip(DELAY); @@ -360,8 +533,7 @@ contract AuthBridgeExecutorTest is Test { _queueAction(); skip(DELAY); - vm.prank(guardian); - executor.cancel(0); + executor.execute(0); vm.expectRevert(abi.encodeWithSignature("OnlyQueuedActions()")); vm.prank(guardian); @@ -401,6 +573,10 @@ contract AuthBridgeExecutorTest is Test { assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutorBase.ActionsSetState.Canceled)); } +} + +contract AuthBridgeExecutorUpdateTests is AuthBridgeExecutorTestBase { + function test_updateGuardian_notSelf() public { vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); executor.updateGuardian(guardian); @@ -460,6 +636,10 @@ contract AuthBridgeExecutorTest is Test { assertEq(executor.getGracePeriod(), 60 days); } +} + +contract AuthBridgeExecutorMiscTests is AuthBridgeExecutorTestBase { + function test_executeDelegateCall_notSelf() public { vm.expectRevert(abi.encodeWithSignature("OnlyCallableByThis()")); executor.executeDelegateCall(address(0), ""); @@ -482,62 +662,4 @@ contract AuthBridgeExecutorTest is Test { assertEq(address(executor).balance, 1 ether); } - function _getDefaultAction() internal returns (Action memory) { - address[] memory targets = new address[](1); - targets[0] = address(new DefaultPayload()); - uint256[] memory values = new uint256[](1); - values[0] = 0; - string[] memory signatures = new string[](1); - signatures[0] = "execute()"; - bytes[] memory calldatas = new bytes[](1); - calldatas[0] = ""; - bool[] memory withDelegatecalls = new bool[](1); - withDelegatecalls[0] = true; - - return Action({ - targets: targets, - values: values, - signatures: signatures, - calldatas: calldatas, - withDelegatecalls: withDelegatecalls - }); - } - - function _queueAction(Action memory action) internal { - vm.prank(bridge); - executor.queue( - action.targets, - action.values, - action.signatures, - action.calldatas, - action.withDelegatecalls - ); - } - - function _queueAction() internal { - _queueAction(_getDefaultAction()); - } - - function _queueActionWithValue(uint256 value) internal { - Action memory action = _getDefaultAction(); - action.targets[0] = address(new PayablePayload()); - action.values[0] = value; - _queueAction(action); - } - - function _encodeHash(Action memory action, uint256 index, uint256 executionTime) internal pure returns (bytes32) { - return keccak256(abi.encode( - action.targets[index], - action.values[index], - action.signatures[index], - action.calldatas[index], - executionTime, - action.withDelegatecalls[index] - )); - } - - function _encodeHash(Action memory action, uint256 executionTime) internal pure returns (bytes32) { - return _encodeHash(action, 0, executionTime); - } - } diff --git a/test/OptimismCrosschainTest.t.sol b/test/OptimismCrosschainTest.t.sol index 8d029e0..21fe8f7 100644 --- a/test/OptimismCrosschainTest.t.sol +++ b/test/OptimismCrosschainTest.t.sol @@ -63,7 +63,7 @@ contract OptimismCrosschainTest is CrosschainTestBase { bridgeExecutor ); - assertEq(receiver.l1Authority(), defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor); + assertEq(receiver.l1Authority(), defaultL2BridgeExecutorArgs.ethereumGovernanceExecutor); assertEq(address(receiver.executor()), address(bridgeExecutor)); }