diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index b9c099add7a..549a827d4ad 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/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", diff --git a/packages/contracts-bedrock/src/L1/OptimismPortal2.sol b/packages/contracts-bedrock/src/L1/OptimismPortal2.sol index 70e7792afd2..ce25123f47d 100644 --- a/packages/contracts-bedrock/src/L1/OptimismPortal2.sol +++ b/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. diff --git a/packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol b/packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol index 9a5f5d94c43..0659cc4d5fa 100644 --- a/packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol +++ b/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 diff --git a/packages/contracts-bedrock/test/L1/OptimismPortalInterop.t.sol b/packages/contracts-bedrock/test/L1/OptimismPortalInterop.t.sol index 0a32877b822..f83b210ea1f 100644 --- a/packages/contracts-bedrock/test/L1/OptimismPortalInterop.t.sol +++ b/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