fix: portal withdrawal checks

packages/contracts-bedrock/snapshots/semver-lock.json

@@ -20,12 +20,12 @@
     "sourceCodeHash": "0x1c37bb41a46a1c0fe6c8cef88037ecc6700675c4cfa8d673349f026ffd919526"
   },
   "src/L1/OptimismPortal2.sol": {
-    "initCodeHash": "0xfaa5f7f911871a00e43024bcf1344cfe846c3cb1aec83d898f5acdd98e3ae223",
-    "sourceCodeHash": "0x104a63a791328f3cfb762368a0e1249187b428e1b18a805e0cac441689603a60"
+    "initCodeHash": "0x7488f3311a94278272a5b642d9abd90bb68d72230d5dc3b82fb8c10ddfc2829b",
+    "sourceCodeHash": "0x1cad4f3a6d624ac0a84c8361e16f7f1785d65248b1be00637e003231b1c1a3f0"
   },
   "src/L1/OptimismPortalInterop.sol": {
-    "initCodeHash": "0x71aaaf0c19eb19549024ba0eabd4cfe4f2ff5d4d0a9628acd539c3bc685fdfe2",
-    "sourceCodeHash": "0x6b6e82f72f34f89ed619e9e285ab46118b231b5e03c946dc482a08d52adf2831"
+    "initCodeHash": "0x7094f190af70aed2bc8519f9ea6cfee5412d07ba5571a4376dfb56152b8eaed7",
+    "sourceCodeHash": "0x81275a82d66ef7d6786a5fd641e1f79c1cf90a1a7ba9c83bd4cfe78658f02b00"
   },
   "src/L1/ProtocolVersions.sol": {
     "initCodeHash": "0x5a76c8530cb24cf23d3baacc6eefaac226382af13f1e2a35535d2ec2b0573b29",

packages/contracts-bedrock/src/L1/OptimismPortal2.sol

@@ -294,6 +294,15 @@ contract OptimismPortal2 is Initializable, ResourceMetering, ISemver {
         }
     }
 
+    /// @notice Validates a withdrawal before it is proved or finalized.
+    /// @param _tx Withdrawal transaction to validate.
+    function _validateWithdrawal(Types.WithdrawalTransaction memory _tx) internal view virtual {
+        // Prevent users from creating a deposit transaction where this address is the message
+        // sender on L2. Because this is checked here, we do not need to check again in
+        // `finalizeWithdrawalTransaction`.
+        if (_tx.target == address(this)) revert BadTarget();
+    }
+
     /// @notice Proves a withdrawal transaction.
     /// @param _tx               Withdrawal transaction to finalize.
     /// @param _disputeGameIndex Index of the dispute game to prove the withdrawal against.
@@ -308,10 +317,8 @@ contract OptimismPortal2 is Initializable, ResourceMetering, ISemver {
         external
         whenNotPaused
     {
-        // Prevent users from creating a deposit transaction where this address is the message
-        // sender on L2. Because this is checked here, we do not need to check again in
-        // `finalizeWithdrawalTransaction`.
-        if (_tx.target == address(this)) revert BadTarget();
+        // Validate the withdrawal before it is proved.
+        _validateWithdrawal(_tx);
 
         // Fetch the dispute game proxy from the `DisputeGameFactory` contract.
         (GameType gameType,, IDisputeGame gameProxy) = disputeGameFactory.gameAtIndex(_disputeGameIndex);
@@ -402,6 +409,9 @@ contract OptimismPortal2 is Initializable, ResourceMetering, ISemver {
         public
         whenNotPaused
     {
+        // Validate the withdrawal before it is finalized.
+        _validateWithdrawal(_tx);
+
         // Make sure that the l2Sender has not yet been set. The l2Sender is set to a value other
         // than the default value when a withdrawal transaction is being finalized. This check is
         // a defacto reentrancy guard.

packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol

@@ -95,15 +95,23 @@ contract OptimismPortalInterop is OptimismPortal2 {
         return _storage().migrated;
     }
 
-    /// @notice Unlock and receive the ETH from the SharedLockbox.
-    /// @param _tx Withdrawal transaction to finalize.
-    function _unlockETH(Types.WithdrawalTransaction memory _tx) internal virtual override {
+    /// @notice Validates a withdrawal before it is proved or finalized.
+    /// @param _tx Withdrawal transaction to validate.
+    function _validateWithdrawal(Types.WithdrawalTransaction memory _tx) internal view virtual override {
+        super._validateWithdrawal(_tx);
+
         OptimismPortalStorage storage s = _storage();
 
         // We don't allow the SharedLockbox to be the target of a withdrawal.
         // This is to prevent the SharedLockbox from being drained.
         // This check needs to be done for every withdrawal.
         if (_tx.target == s.sharedLockbox) revert MessageTargetSharedLockbox();
+    }
+
+    /// @notice Unlock and receive the ETH from the SharedLockbox.
+    /// @param _tx Withdrawal transaction to finalize.
+    function _unlockETH(Types.WithdrawalTransaction memory _tx) internal virtual override {
+        OptimismPortalStorage storage s = _storage();
 
         // If ETH liquidity has not been migrated to the SharedLockbox yet, maintain legacy behavior
         // where ETH accumulates in the portal contract itself rather than being managed by the lockbox

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

@@ -1709,6 +1709,10 @@ contract OptimismPortalInteropMock is OptimismPortalInterop {
         OptimismPortalInterop(_proofMaturityDelaySeconds, _disputeGameFinalityDelaySeconds)
     { }
 
+    function exposed_validateWithdrawal(Types.WithdrawalTransaction memory _tx) external view {
+        _validateWithdrawal(_tx);
+    }
+
     function exposed_lockETH() external payable {
         _lockETH();
     }
@@ -1870,8 +1874,8 @@ contract OptimismPortalInterop_InternalFunctions_Test is OptimismPortalInterop_B
         assertEq(address(sharedLockbox).balance, 0);
     }
 
-    /// @dev Tests _unlockETH reverts when target is sharedLockbox
-    function testFuzz_unlockETH_targetIsSharedLockbox_reverts(
+    /// @dev Tests _validateWithdrawal reverts when target is sharedLockbox
+    function testFuzz_validateWithdrawal_targetIsSharedLockbox_reverts(
         uint256 _nonce,
         address _sender,
         uint256 _value,
@@ -1880,8 +1884,6 @@ contract OptimismPortalInterop_InternalFunctions_Test is OptimismPortalInterop_B
     )
         external
     {
-        vm.assume(_value > 0);
-
         Types.WithdrawalTransaction memory wTx = Types.WithdrawalTransaction({
             nonce: _nonce,
             sender: _sender,
@@ -1892,7 +1894,34 @@ contract OptimismPortalInterop_InternalFunctions_Test is OptimismPortalInterop_B
         });
 
         vm.expectRevert(IOptimismPortalInterop.MessageTargetSharedLockbox.selector);
-        _mockPortal().exposed_unlockETH(wTx);
+        _mockPortal().exposed_validateWithdrawal(wTx);
+    }
+
+    /// @dev Tests _validateWithdrawal not reverts when target is not sharedLockbox
+    function testFuzz_validateWithdrawal_targetIsNotSharedLockbox_succeeds(
+        uint256 _nonce,
+        address _sender,
+        address _target,
+        uint256 _value,
+        uint256 _gasLimit,
+        bytes memory _data
+    )
+        external
+        view
+    {
+        vm.assume(_target != address(sharedLockbox));
+
+        Types.WithdrawalTransaction memory wTx = Types.WithdrawalTransaction({
+            nonce: _nonce,
+            sender: _sender,
+            target: _target,
+            value: _value,
+            gasLimit: _gasLimit,
+            data: _data
+        });
+
+        // Should not revert
+        _mockPortal().exposed_validateWithdrawal(wTx);
     }
 
     /// @dev Tests _unlockETH when value is zero