From b8b8bb31a3706a8f0322d46a9a58837ff6cc5737 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Tue, 19 Nov 2024 08:49:37 +0100 Subject: [PATCH 1/3] improve optimismportal(2) test coverage --- .../test/L1/OptimismPortal.t.sol | 13 ++- .../test/L1/OptimismPortal2.t.sol | 90 +++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol index f19e7ca6f5c29..1b0154afba12b 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol +++ b/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol @@ -968,7 +968,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest { } /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty. - function test_finalizeWithdrawalTransaction_noTxData_succeeds() external { + function test_finalizeWithdrawalTransaction_noTxDataNonEtherGasToken_succeeds() external { Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ nonce: 0, sender: alice, @@ -1006,7 +1006,14 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest { ) ); - uint256 bobBalanceBefore = address(bob).balance; + // Fund the portal so that we can withdraw ETH. + vm.store(address(optimismPortal), bytes32(uint256(61)), bytes32(uint256(0xFFFFFFFF))); + deal(address(L1Token), address(optimismPortal), 0xFFFFFFFF); + // modify the gas token to be non ether + vm.mockCall( + address(systemConfig), abi.encodeCall(systemConfig.gasPayingToken, ()), abi.encode(address(L1Token), 18) + ); + uint256 bobBalanceBefore = L1Token.balanceOf(bob); vm.expectEmit(true, true, true, true); emit WithdrawalProven(_withdrawalHash_noData, alice, bob); @@ -1019,7 +1026,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest { emit WithdrawalFinalized(_withdrawalHash_noData, true); optimismPortal.finalizeWithdrawalTransaction(_defaultTx_noData); - assertEq(address(bob).balance, bobBalanceBefore + 100); + assertEq(L1Token.balanceOf(bob), bobBalanceBefore + 100); } /// @dev Tests that `finalizeWithdrawalTransaction` reverts if the finalization period diff --git a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol index d252609e5eea4..8fec60b827bd6 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol +++ b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol @@ -802,6 +802,96 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { assert(address(bob).balance == bobBalanceBefore + 100); } + /// @dev Tests that `finalizeWithdrawalTransaction` reverts if the target reverts and caller is the + /// ESTIMATION_ADDRESS. + function test_finalizeWithdrawalTransaction_targetFailsAndCallerIsEstimationAddress_reverts() external { + vm.etch(bob, hex"fe"); // Contract with just the invalid opcode. + + vm.expectEmit(true, true, true, true); + emit WithdrawalProven(_withdrawalHash, alice, bob); + optimismPortal.proveWithdrawalTransaction(_defaultTx, _proposedGameIndex, _outputRootProof, _withdrawalProof); + + vm.warp(block.timestamp + l2OutputOracle.FINALIZATION_PERIOD_SECONDS() + 1); + + vm.startPrank(Constants.ESTIMATION_ADDRESS, Constants.ESTIMATION_ADDRESS); + vm.expectRevert(GasEstimation.selector); + optimismPortal.finalizeWithdrawalTransaction(_defaultTx); + } + + /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty. + function test_finalizeWithdrawalTransaction_noTxDataNonEtherGasToken_succeeds() external { + Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ + nonce: 0, + sender: alice, + target: bob, + value: 100, + gasLimit: 100_000, + data: hex"" + }); + // Get withdrawal proof data we can use for testing. + ( + bytes32 _stateRoot_noData, + bytes32 _storageRoot_noData, + bytes32 _outputRoot_noData, + bytes32 _withdrawalHash_noData, + bytes[] memory _withdrawalProof_noData + ) = ffi.getProveWithdrawalTransactionInputs(_defaultTx_noData); + // Setup a dummy output root proof for reuse. + Types.OutputRootProof memory _outputRootProof_noData = Types.OutputRootProof({ + version: bytes32(uint256(0)), + stateRoot: _stateRoot_noData, + messagePasserStorageRoot: _storageRoot_noData, + latestBlockhash: bytes32(uint256(0)) + }); + uint256 _proposedBlockNumber_noData = 0xFF; + IFaultDisputeGame game_noData = IFaultDisputeGame( + payable( + address( + disputeGameFactory.create( + optimismPortal2.respectedGameType(), + Claim.wrap(_outputRoot_noData), + abi.encode(_proposedBlockNumber_noData) + ) + ) + ) + ); + uint256 _proposedGameIndex_noData = disputeGameFactory.gameCount() - 1; + // Warp beyond the chess clocks and finalize the game. + vm.warp(block.timestamp + game_noData.maxClockDuration().raw() + 1 seconds); + // Fund the portal so that we can withdraw ETH. + vm.store(address(optimismPortal2), bytes32(uint256(61)), bytes32(uint256(0xFFFFFFFF))); + deal(address(L1Token), address(optimismPortal2), 0xFFFFFFFF); + + // modify the gas token to be non ether + vm.mockCall( + address(systemConfig), abi.encodeCall(systemConfig.gasPayingToken, ()), abi.encode(address(L1Token), 18) + ); + + uint256 bobBalanceBefore = L1Token.balanceOf(bob); + + vm.expectEmit(address(optimismPortal2)); + emit WithdrawalProven(_withdrawalHash_noData, alice, bob); + vm.expectEmit(address(optimismPortal2)); + emit WithdrawalProvenExtension1(_withdrawalHash_noData, address(this)); + optimismPortal2.proveWithdrawalTransaction({ + _tx: _defaultTx_noData, + _disputeGameIndex: _proposedGameIndex_noData, + _outputRootProof: _outputRootProof_noData, + _withdrawalProof: _withdrawalProof_noData + }); + + // Warp and resolve the dispute game. + game_noData.resolveClaim(0, 0); + game_noData.resolve(); + vm.warp(block.timestamp + optimismPortal2.proofMaturityDelaySeconds() + 1 seconds); + + vm.expectEmit(true, true, false, true); + emit WithdrawalFinalized(_withdrawalHash_noData, true); + optimismPortal2.finalizeWithdrawalTransaction(_defaultTx_noData); + + assert(L1Token.balanceOf(bob) == bobBalanceBefore + 100); + } + /// @dev Tests that `finalizeWithdrawalTransaction` succeeds. function test_finalizeWithdrawalTransaction_provenWithdrawalHashEther_succeeds() external { uint256 bobBalanceBefore = address(bob).balance; From 22ae538c72ab601c3c6162a90ea8f0a411df705f Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Tue, 19 Nov 2024 09:10:45 +0100 Subject: [PATCH 2/3] improve optimismportal(2) test coverage --- .../test/L1/OptimismPortal2.t.sol | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol index 8fec60b827bd6..548ae0cfc411e 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol +++ b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol @@ -439,6 +439,12 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { constructor() { super.setUp(); + // zero out contracts that should not be used + assembly { + sstore(l2OutputOracle.slot, 0) + sstore(optimismPortal.slot, 0) + } + _defaultTx = Types.WithdrawalTransaction({ nonce: 0, sender: alice, @@ -807,15 +813,19 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { function test_finalizeWithdrawalTransaction_targetFailsAndCallerIsEstimationAddress_reverts() external { vm.etch(bob, hex"fe"); // Contract with just the invalid opcode. + vm.prank(alice); vm.expectEmit(true, true, true, true); emit WithdrawalProven(_withdrawalHash, alice, bob); - optimismPortal.proveWithdrawalTransaction(_defaultTx, _proposedGameIndex, _outputRootProof, _withdrawalProof); + optimismPortal2.proveWithdrawalTransaction(_defaultTx, _proposedGameIndex, _outputRootProof, _withdrawalProof); - vm.warp(block.timestamp + l2OutputOracle.FINALIZATION_PERIOD_SECONDS() + 1); + // Warp and resolve the dispute game. + game.resolveClaim(0, 0); + game.resolve(); + vm.warp(block.timestamp + optimismPortal2.proofMaturityDelaySeconds() + 1 seconds); - vm.startPrank(Constants.ESTIMATION_ADDRESS, Constants.ESTIMATION_ADDRESS); + vm.startPrank(alice, Constants.ESTIMATION_ADDRESS); vm.expectRevert(GasEstimation.selector); - optimismPortal.finalizeWithdrawalTransaction(_defaultTx); + optimismPortal2.finalizeWithdrawalTransaction(_defaultTx); } /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty. @@ -1489,6 +1499,12 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { contract OptimismPortal2_Upgradeable_Test is CommonTest { function setUp() public override { super.setUp(); + + // zero out contracts that should not be used + assembly { + sstore(l2OutputOracle.slot, 0) + sstore(optimismPortal.slot, 0) + } } /// @dev Tests that the proxy is initialized correctly. @@ -1534,6 +1550,12 @@ contract OptimismPortal2_ResourceFuzz_Test is CommonTest { function setUp() public override { super.setUp(); + + // zero out contracts that should not be used + assembly { + sstore(l2OutputOracle.slot, 0) + sstore(optimismPortal.slot, 0) + } } /// @dev Test that various values of the resource metering config will not break deposits. @@ -1623,6 +1645,13 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal function setUp() public virtual override { super.setUp(); + + // zero out contracts that should not be used + assembly { + sstore(l2OutputOracle.slot, 0) + sstore(optimismPortal.slot, 0) + } + token = new MockERC20("Test", "TST", 18); } @@ -1665,7 +1694,7 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal ); // Deposit the token into the portal - optimismPortal.depositERC20Transaction(_to, _mint, _value, _gasLimit, _isCreation, _data); + optimismPortal2.depositERC20Transaction(_to, _mint, _value, _gasLimit, _isCreation, _data); // Assert final balance equals the deposited amount assertEq(token.balanceOf(address(optimismPortal2)), _mint); @@ -1747,7 +1776,7 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal ); // Mock the token balance - vm.mockCall(address(token), abi.encodeCall(token.balanceOf, (address(optimismPortal))), abi.encode(0)); + vm.mockCall(address(token), abi.encodeCall(token.balanceOf, (address(optimismPortal2))), abi.encode(0)); // Call minimumGasLimit(0) before vm.expectRevert to ensure vm.expectRevert is for depositERC20Transaction uint64 gasLimit = optimismPortal2.minimumGasLimit(0); @@ -1813,7 +1842,7 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal ); // Deposit the token into the portal - optimismPortal2.depositERC20Transaction(address(0), _amount, 0, optimismPortal.minimumGasLimit(0), false, ""); + optimismPortal2.depositERC20Transaction(address(0), _amount, 0, optimismPortal2.minimumGasLimit(0), false, ""); // Check that the balance has been correctly updated assertEq(optimismPortal2.balance(), _amount); @@ -1832,7 +1861,7 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal // Deposit the token into the portal optimismPortal2.depositERC20Transaction( - address(bob), _defaultTx.value, 0, optimismPortal.minimumGasLimit(0), false, "" + address(bob), _defaultTx.value, 0, optimismPortal2.minimumGasLimit(0), false, "" ); assertEq(optimismPortal2.balance(), _defaultTx.value); From 4b600e277b1dc2a8d7181ed2e4988858296a69cb Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 20 Nov 2024 08:10:24 +0100 Subject: [PATCH 3/3] more tests, fixes --- .../test/L1/OptimismPortal.t.sol | 58 ++++++++++++ .../test/L1/OptimismPortal2.t.sol | 94 ++++++++++++++----- 2 files changed, 127 insertions(+), 25 deletions(-) diff --git a/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol index 1b0154afba12b..8172fb14b7931 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol +++ b/packages/contracts-bedrock/test/L1/OptimismPortal.t.sol @@ -968,6 +968,64 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest { } /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty. + function test_finalizeWithdrawalTransaction_noTxData_succeeds() external { + Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ + nonce: 0, + sender: alice, + target: bob, + value: 100, + gasLimit: 100_000, + data: hex"" + }); + // Get withdrawal proof data we can use for testing. + ( + bytes32 _stateRoot_noData, + bytes32 _storageRoot_noData, + bytes32 _outputRoot_noData, + bytes32 _withdrawalHash_noData, + bytes[] memory _withdrawalProof_noData + ) = ffi.getProveWithdrawalTransactionInputs(_defaultTx_noData); + // Setup a dummy output root proof for reuse. + Types.OutputRootProof memory _outputRootProof_noData = Types.OutputRootProof({ + version: bytes32(uint256(0)), + stateRoot: _stateRoot_noData, + messagePasserStorageRoot: _storageRoot_noData, + latestBlockhash: bytes32(uint256(0)) + }); + + // Configure the oracle to return the output root we've prepared. + vm.mockCall( + address(l2OutputOracle), + abi.encodePacked(IL2OutputOracle.getL2Output.selector), + abi.encode( + Types.OutputProposal( + _outputRoot_noData, + l2OutputOracle.getL2Output(_proposedOutputIndex).timestamp, + uint128(_proposedBlockNumber) + ) + ) + ); + + // Fund the portal so that we can withdraw ETH. + vm.store(address(optimismPortal), bytes32(uint256(61)), bytes32(uint256(0xFFFFFFFF))); + vm.deal(address(optimismPortal), 0xFFFFFFFF); + uint256 bobBalanceBefore = bob.balance; + + vm.expectEmit(true, true, true, true); + emit WithdrawalProven(_withdrawalHash_noData, alice, bob); + optimismPortal.proveWithdrawalTransaction( + _defaultTx_noData, _proposedOutputIndex, _outputRootProof_noData, _withdrawalProof_noData + ); + + vm.warp(block.timestamp + l2OutputOracle.FINALIZATION_PERIOD_SECONDS() + 1); + vm.expectEmit(true, true, false, true); + emit WithdrawalFinalized(_withdrawalHash_noData, true); + optimismPortal.finalizeWithdrawalTransaction(_defaultTx_noData); + + assertEq(bob.balance, bobBalanceBefore + 100); + } + + /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty and with a custom gas token. function test_finalizeWithdrawalTransaction_noTxDataNonEtherGasToken_succeeds() external { Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ nonce: 0, diff --git a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol index 548ae0cfc411e..083644755d3c7 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol +++ b/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol @@ -439,12 +439,6 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { constructor() { super.setUp(); - // zero out contracts that should not be used - assembly { - sstore(l2OutputOracle.slot, 0) - sstore(optimismPortal.slot, 0) - } - _defaultTx = Types.WithdrawalTransaction({ nonce: 0, sender: alice, @@ -829,6 +823,75 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { } /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty. + function test_finalizeWithdrawalTransaction_noTxData_succeeds() external { + Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ + nonce: 0, + sender: alice, + target: bob, + value: 100, + gasLimit: 100_000, + data: hex"" + }); + // Get withdrawal proof data we can use for testing. + ( + bytes32 _stateRoot_noData, + bytes32 _storageRoot_noData, + bytes32 _outputRoot_noData, + bytes32 _withdrawalHash_noData, + bytes[] memory _withdrawalProof_noData + ) = ffi.getProveWithdrawalTransactionInputs(_defaultTx_noData); + // Setup a dummy output root proof for reuse. + Types.OutputRootProof memory _outputRootProof_noData = Types.OutputRootProof({ + version: bytes32(uint256(0)), + stateRoot: _stateRoot_noData, + messagePasserStorageRoot: _storageRoot_noData, + latestBlockhash: bytes32(uint256(0)) + }); + uint256 _proposedBlockNumber_noData = 0xFF; + IFaultDisputeGame game_noData = IFaultDisputeGame( + payable( + address( + disputeGameFactory.create( + optimismPortal2.respectedGameType(), + Claim.wrap(_outputRoot_noData), + abi.encode(_proposedBlockNumber_noData) + ) + ) + ) + ); + uint256 _proposedGameIndex_noData = disputeGameFactory.gameCount() - 1; + // Warp beyond the chess clocks and finalize the game. + vm.warp(block.timestamp + game_noData.maxClockDuration().raw() + 1 seconds); + // Fund the portal so that we can withdraw ETH. + vm.store(address(optimismPortal2), bytes32(uint256(61)), bytes32(uint256(0xFFFFFFFF))); + vm.deal(address(optimismPortal2), 0xFFFFFFFF); + + uint256 bobBalanceBefore = bob.balance; + + vm.expectEmit(address(optimismPortal2)); + emit WithdrawalProven(_withdrawalHash_noData, alice, bob); + vm.expectEmit(address(optimismPortal2)); + emit WithdrawalProvenExtension1(_withdrawalHash_noData, address(this)); + optimismPortal2.proveWithdrawalTransaction({ + _tx: _defaultTx_noData, + _disputeGameIndex: _proposedGameIndex_noData, + _outputRootProof: _outputRootProof_noData, + _withdrawalProof: _withdrawalProof_noData + }); + + // Warp and resolve the dispute game. + game_noData.resolveClaim(0, 0); + game_noData.resolve(); + vm.warp(block.timestamp + optimismPortal2.proofMaturityDelaySeconds() + 1 seconds); + + vm.expectEmit(true, true, false, true); + emit WithdrawalFinalized(_withdrawalHash_noData, true); + optimismPortal2.finalizeWithdrawalTransaction(_defaultTx_noData); + + assert(bob.balance == bobBalanceBefore + 100); + } + + /// @dev Tests that `finalizeWithdrawalTransaction` succeeds when _tx.data is empty and with a custom gas token. function test_finalizeWithdrawalTransaction_noTxDataNonEtherGasToken_succeeds() external { Types.WithdrawalTransaction memory _defaultTx_noData = Types.WithdrawalTransaction({ nonce: 0, @@ -1499,12 +1562,6 @@ contract OptimismPortal2_FinalizeWithdrawal_Test is CommonTest { contract OptimismPortal2_Upgradeable_Test is CommonTest { function setUp() public override { super.setUp(); - - // zero out contracts that should not be used - assembly { - sstore(l2OutputOracle.slot, 0) - sstore(optimismPortal.slot, 0) - } } /// @dev Tests that the proxy is initialized correctly. @@ -1550,12 +1607,6 @@ contract OptimismPortal2_ResourceFuzz_Test is CommonTest { function setUp() public override { super.setUp(); - - // zero out contracts that should not be used - assembly { - sstore(l2OutputOracle.slot, 0) - sstore(optimismPortal.slot, 0) - } } /// @dev Test that various values of the resource metering config will not break deposits. @@ -1645,13 +1696,6 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal function setUp() public virtual override { super.setUp(); - - // zero out contracts that should not be used - assembly { - sstore(l2OutputOracle.slot, 0) - sstore(optimismPortal.slot, 0) - } - token = new MockERC20("Test", "TST", 18); }