improve contracts code coverage (#12900)

* improve L1 contracts code cov

* improve L1 contracts code cov

* improve L1 contracts code cov

* improve L1 contracts code cov

* improve L1 contracts code cov

* improve L1 contracts code cov

packages/contracts-bedrock/test/L1/DataAvailabilityChallenge.t.sol

@@ -52,6 +52,26 @@ contract DataAvailabilityChallengeTest is CommonTest {
         assertEq(sender.balance, amount);
     }
 
+    function test_withdraw_fails_reverts(address sender, uint256 amount) public {
+        assumePayable(sender);
+        assumeNotPrecompile(sender);
+        // EntryPoint will revert if using amount > type(uint112).max.
+        vm.assume(sender != Preinstalls.EntryPoint_v060);
+        vm.assume(sender != address(dataAvailabilityChallenge));
+        vm.assume(sender.balance == 0);
+        vm.deal(sender, amount);
+
+        vm.prank(sender);
+        dataAvailabilityChallenge.deposit{ value: amount }();
+
+        assertEq(dataAvailabilityChallenge.balances(sender), amount);
+        assertEq(sender.balance, 0);
+
+        vm.etch(sender, hex"fe");
+        vm.expectRevert(abi.encodeWithSelector(IDataAvailabilityChallenge.WithdrawalFailed.selector));
+        dataAvailabilityChallenge.withdraw();
+    }
+
     function test_challenge_succeeds(
         address challenger,
         uint256 challengedBlockNumber,
@@ -220,6 +240,7 @@ contract DataAvailabilityChallengeTest is CommonTest {
         bytes memory preImage,
         uint256 challengedBlockNumber,
         uint256 resolverRefundPercentage,
+        uint64 bondSize,
         uint128 txGasPrice
     )
         public
@@ -229,6 +250,9 @@ contract DataAvailabilityChallengeTest is CommonTest {
         vm.assume(resolver != address(0));
         vm.assume(challenger != resolver);
 
+        vm.prank(dataAvailabilityChallenge.owner());
+        dataAvailabilityChallenge.setBondSize(bondSize);
+
         // Bound the resolver refund percentage to 100
         resolverRefundPercentage = bound(resolverRefundPercentage, 0, 100);
 
@@ -251,7 +275,6 @@ contract DataAvailabilityChallengeTest is CommonTest {
         vm.roll(challengedBlockNumber + 1);
 
         // Challenge the hash
-        uint256 bondSize = dataAvailabilityChallenge.bondSize();
         vm.deal(challenger, bondSize);
         vm.prank(challenger);
         dataAvailabilityChallenge.challenge{ value: bondSize }(challengedBlockNumber, challengedCommitment);
@@ -259,6 +282,26 @@ contract DataAvailabilityChallengeTest is CommonTest {
         // Store the address(0) balance before resolving to assert the burned amount later
         uint256 zeroAddressBalanceBeforeResolve = address(0).balance;
 
+        // Assert challenger balance after bond distribution
+        uint256 resolutionCost = (
+            dataAvailabilityChallenge.fixedResolutionCost()
+                + preImage.length * dataAvailabilityChallenge.variableResolutionCost()
+                    / dataAvailabilityChallenge.variableResolutionCostPrecision()
+        ) * block.basefee;
+        uint256 challengerRefund = bondSize > resolutionCost ? bondSize - resolutionCost : 0;
+        uint256 resolverRefund = resolutionCost * dataAvailabilityChallenge.resolverRefundPercentage() / 100;
+        resolverRefund = resolverRefund > resolutionCost ? resolutionCost : resolverRefund;
+        resolverRefund = resolverRefund > bondSize ? bondSize : resolverRefund;
+
+        if (challengerRefund > 0) {
+            vm.expectEmit(true, true, true, true);
+            emit BalanceChanged(challenger, challengerRefund);
+        }
+        if (resolverRefund > 0) {
+            vm.expectEmit(true, true, true, true);
+            emit BalanceChanged(resolver, resolverRefund);
+        }
+
         // Resolve the challenge
         vm.prank(resolver);
         dataAvailabilityChallenge.resolve(challengedBlockNumber, challengedCommitment, preImage);
@@ -274,27 +317,72 @@ contract DataAvailabilityChallengeTest is CommonTest {
             uint8(dataAvailabilityChallenge.getChallengeStatus(challengedBlockNumber, challengedCommitment)),
             uint8(ChallengeStatus.Resolved)
         );
-
-        // Assert challenger balance after bond distribution
-        uint256 resolutionCost = (
-            dataAvailabilityChallenge.fixedResolutionCost()
-                + preImage.length * dataAvailabilityChallenge.variableResolutionCost()
-                    / dataAvailabilityChallenge.variableResolutionCostPrecision()
-        ) * block.basefee;
-        uint256 challengerRefund = bondSize > resolutionCost ? bondSize - resolutionCost : 0;
-        assertEq(dataAvailabilityChallenge.balances(challenger), challengerRefund, "challenger refund");
-
-        // Assert resolver balance after bond distribution
-        uint256 resolverRefund = resolutionCost * dataAvailabilityChallenge.resolverRefundPercentage() / 100;
-        resolverRefund = resolverRefund > resolutionCost ? resolutionCost : resolverRefund;
-        resolverRefund = resolverRefund > bondSize ? bondSize : resolverRefund;
-        assertEq(dataAvailabilityChallenge.balances(resolver), resolverRefund, "resolver refund");
+        address _challenger = challenger;
+        address _resolver = resolver;
+        assertEq(dataAvailabilityChallenge.balances(_challenger), challengerRefund, "challenger refund");
+        assertEq(dataAvailabilityChallenge.balances(_resolver), resolverRefund, "resolver refund");
 
         // Assert burned amount after bond distribution
         uint256 burned = bondSize - challengerRefund - resolverRefund;
         assertEq(address(0).balance - zeroAddressBalanceBeforeResolve, burned, "burned bond");
     }
 
+    function test_resolve_invalidInputData_reverts(
+        address challenger,
+        address resolver,
+        bytes memory preImage,
+        bytes memory wrongPreImage,
+        uint256 challengedBlockNumber,
+        uint256 resolverRefundPercentage,
+        uint128 txGasPrice
+    )
+        public
+    {
+        // Assume neither the challenger nor resolver is address(0) and that they're not the same entity
+        vm.assume(challenger != address(0));
+        vm.assume(resolver != address(0));
+        vm.assume(challenger != resolver);
+        vm.assume(keccak256(preImage) != keccak256(wrongPreImage));
+
+        // Bound the resolver refund percentage to 100
+        resolverRefundPercentage = bound(resolverRefundPercentage, 0, 100);
+
+        // Set the gas price to a fuzzed value to test bond distribution logic
+        vm.txGasPrice(txGasPrice);
+
+        // Change the resolver refund percentage
+        vm.prank(dataAvailabilityChallenge.owner());
+        dataAvailabilityChallenge.setResolverRefundPercentage(resolverRefundPercentage);
+
+        // Assume the block number is not close to the max uint256 value
+        vm.assume(
+            challengedBlockNumber
+                < type(uint256).max - dataAvailabilityChallenge.challengeWindow()
+                    - dataAvailabilityChallenge.resolveWindow()
+        );
+        bytes memory challengedCommitment = computeCommitmentKeccak256(wrongPreImage);
+
+        // Move to block after challenged block
+        vm.roll(challengedBlockNumber + 1);
+
+        // Challenge the hash
+        uint256 bondSize = dataAvailabilityChallenge.bondSize();
+        vm.deal(challenger, bondSize);
+        vm.prank(challenger);
+        dataAvailabilityChallenge.challenge{ value: bondSize }(challengedBlockNumber, challengedCommitment);
+
+        // Resolve the challenge
+        vm.prank(resolver);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IDataAvailabilityChallenge.InvalidInputData.selector,
+                computeCommitmentKeccak256(preImage),
+                challengedCommitment
+            )
+        );
+        dataAvailabilityChallenge.resolve(challengedBlockNumber, challengedCommitment, preImage);
+    }
+
     function test_resolve_nonExistentChallenge_reverts() public {
         bytes memory preImage = "some preimage";
         uint256 challengedBlockNumber = 1;

packages/contracts-bedrock/test/L1/L2OutputOracle.t.sol

@@ -111,6 +111,10 @@ contract L2OutputOracle_getter_Test is L2OutputOracle_TestBase {
         // Querying with exact same block as proposed returns the proposal.
         uint256 index1 = l2OutputOracle.getL2OutputIndexAfter(nextBlockNumber1);
         assertEq(index1, 0);
+        assertEq(
+            keccak256(abi.encode(l2OutputOracle.getL2Output(index1))),
+            keccak256(abi.encode(output1, block.timestamp, nextBlockNumber1))
+        );
     }
 
     /// @dev Tests that `getL2OutputIndexAfter` returns the correct value
@@ -125,6 +129,10 @@ contract L2OutputOracle_getter_Test is L2OutputOracle_TestBase {
         // Querying with previous block returns the proposal too.
         uint256 index1 = l2OutputOracle.getL2OutputIndexAfter(nextBlockNumber1 - 1);
         assertEq(index1, 0);
+        assertEq(
+            keccak256(abi.encode(l2OutputOracle.getL2Output(index1))),
+            keccak256(abi.encode(output1, block.timestamp, nextBlockNumber1))
+        );
     }
 
     /// @dev Tests that `getL2OutputIndexAfter` returns the correct value.
@@ -156,14 +164,26 @@ contract L2OutputOracle_getter_Test is L2OutputOracle_TestBase {
         // Querying with a block number between the first and second proposal
         uint256 index1 = l2OutputOracle.getL2OutputIndexAfter(nextBlockNumber1 + 1);
         assertEq(index1, 1);
+        assertEq(
+            keccak256(abi.encode(l2OutputOracle.getL2Output(index1))),
+            keccak256(abi.encode(output2, l2OutputOracle.computeL2Timestamp(nextBlockNumber2) + 1, nextBlockNumber2))
+        );
 
         // Querying with a block number between the second and third proposal
         uint256 index2 = l2OutputOracle.getL2OutputIndexAfter(nextBlockNumber2 + 1);
         assertEq(index2, 2);
+        assertEq(
+            keccak256(abi.encode(l2OutputOracle.getL2Output(index2))),
+            keccak256(abi.encode(output3, l2OutputOracle.computeL2Timestamp(nextBlockNumber3) + 1, nextBlockNumber3))
+        );
 
         // Querying with a block number between the third and fourth proposal
         uint256 index3 = l2OutputOracle.getL2OutputIndexAfter(nextBlockNumber3 + 1);
         assertEq(index3, 3);
+        assertEq(
+            keccak256(abi.encode(l2OutputOracle.getL2Output(index3))),
+            keccak256(abi.encode(output4, l2OutputOracle.computeL2Timestamp(nextBlockNumber4) + 1, nextBlockNumber4))
+        );
     }
 
     /// @dev Tests that `getL2OutputIndexAfter` reverts when no output exists.

packages/contracts-bedrock/test/L1/OptimismPortal.t.sol

@@ -598,7 +598,6 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest {
         // Get withdrawal proof data we can use for testing.
         (_stateRoot, _storageRoot, _outputRoot, _withdrawalHash, _withdrawalProof) =
             ffi.getProveWithdrawalTransactionInputs(_defaultTx);
-
         // Setup a dummy output root proof for reuse.
         _outputRootProof = Types.OutputRootProof({
             version: bytes32(uint256(0)),
@@ -606,6 +605,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest {
             messagePasserStorageRoot: _storageRoot,
             latestBlockhash: bytes32(uint256(0))
         });
+
         _proposedBlockNumber = l2OutputOracle.nextBlockNumber();
         _proposedOutputIndex = l2OutputOracle.nextOutputIndex();
     }
@@ -934,7 +934,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest {
         assertEq(bobBalanceBefore, address(bob).balance);
     }
 
-    /// @dev Tests that `finalizeWithdrawalTransaction` reverts if the target reverts.
+    /// @dev Tests that `finalizeWithdrawalTransaction` fails if the target reverts.
     function test_finalizeWithdrawalTransaction_targetFails_fails() external {
         uint256 bobBalanceBefore = address(bob).balance;
         vm.etch(bob, hex"fe"); // Contract with just the invalid opcode.
@@ -951,6 +951,77 @@ contract OptimismPortal_FinalizeWithdrawal_Test is CommonTest {
         assert(address(bob).balance == bobBalanceBefore);
     }
 
+    /// @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, _proposedOutputIndex, _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_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)
+                )
+            )
+        );
+
+        uint256 bobBalanceBefore = address(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(address(bob).balance, bobBalanceBefore + 100);
+    }
+
     /// @dev Tests that `finalizeWithdrawalTransaction` reverts if the finalization period
     ///      has not yet passed.
     function test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() external {

packages/contracts-bedrock/test/setup/Events.sol

@@ -104,4 +104,6 @@ contract Events {
     event Paused(string identifier);
 
     event Unpaused();
+
+    event BalanceChanged(address account, uint256 balance);
 }