Allow re-proving withdrawal if the outputRoot changes and the `l2Bl…

…ockNumber` stays the same

.changeset/beige-clocks-wave.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Fixes a severe vulnerability found in ToB's November 2022 audit of the Bedrock contracts

packages/contracts-bedrock/.gas-snapshot

@@ -158,23 +158,24 @@ OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10
 OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15767)
 OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_success() (gas: 16010)
 OptimismPortalUpgradeable_Test:test_upgradeToAndCall_upgrading_success() (gas: 180435)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_differential_success(address,address,uint256,uint256,bytes) (runs: 256, μ: 224693, ~: 224460)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 192897)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 195163)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39589)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 190639)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 193046)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 173111)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233303)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 232783)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 224777)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 327459)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 191395)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 81311)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50776)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 132306)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 186207)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 176926)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_differential_success(address,address,uint256,uint256,bytes) (runs: 256, μ: 224815, ~: 224581)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 193085)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 195285)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39634)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 190761)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 193190)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 173233)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233467)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 232905)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 224966)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 327581)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 191517)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 81289)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50754)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 134547)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() (gas: 274636)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 186698)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 177114)
 OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17298)
 OptimismPortal_Test:test_OptimismPortalReceiveEth_success() (gas: 127483)
 OptimismPortal_Test:test_depositTransaction_NoValueContract_success() (gas: 76706)

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

@@ -189,15 +189,19 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         // and to prevent replay attacks.
         bytes32 withdrawalHash = Hashing.hashWithdrawal(_tx);
 
-        // Ensure that the withdrawalHash has not already been proven to prevent a malicious party
-        // from censoring the withdrawal.
+        // Load the ProvenWithdrawal into memory
+        ProvenWithdrawal memory provenWithdrawal = provenWithdrawals[withdrawalHash];
+
+        // Only allow re-proving a withdrawal transaction if the output root has changed.
         require(
-            provenWithdrawals[withdrawalHash].timestamp == 0,
+            provenWithdrawal.timestamp == 0 ||
+                (_l2BlockNumber == provenWithdrawal.l2BlockNumber &&
+                    outputRoot != provenWithdrawal.outputRoot),
             "OptimismPortal: withdrawalHash has already been proven"
         );
 
         // Verify that the hash of this withdrawal was stored in the L2toL1MessagePasser contract on
-        //  L2. If this is true, then we know that this withdrawal was actually triggered on L2
+        // L2. If this is true, then we know that this withdrawal was actually triggered on L2
         // and can therefore be relayed on L1.
         require(
             _verifyWithdrawalInclusion(
@@ -210,7 +214,8 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
 
         // Designate the withdrawalHash as proven by storing the `outputRoot`, `timestamp`,
         // and `l2BlockNumber` in the `provenWithdrawals` mapping. A withdrawalHash can only
-        // be proven one time to prevent a censorship attack.
+        // be proven one time to prevent a censorship attack unless it is submitted again
+        // with a different outputRoot.
         provenWithdrawals[withdrawalHash] = ProvenWithdrawal({
             outputRoot: outputRoot,
             timestamp: uint128(block.timestamp),

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

@@ -392,6 +392,50 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         );
     }
 
+    // Test: proveWithdrawalTransaction succeeds if the passed transaction's withdrawalHash has
+    // already been proven AND the output root has changed AND the l2BlockNumber stays the same.
+    function test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() external {
+        vm.expectEmit(true, true, true, true);
+        emit WithdrawalProven(_withdrawalHash, alice, bob);
+        op.proveWithdrawalTransaction(
+            _defaultTx,
+            _proposedBlockNumber,
+            _outputRootProof,
+            _withdrawalProof
+        );
+
+        // Compute the storage slot of the outputRoot corresponding to the `withdrawalHash`
+        // inside of the `provenWithdrawal`s mapping.
+        bytes32 slot;
+        assembly {
+            mstore(0x00, sload(_withdrawalHash.slot))
+            mstore(0x20, 52) // 52 is the slot of the `provenWithdrawals` mapping in OptimismPortal
+            slot := keccak256(0x00, 0x40)
+        }
+
+        // Store a different output root within the `provenWithdrawals` mapping without
+        // touching the l2BlockNumber or timestamp.
+        vm.store(address(op), slot, bytes32(0));
+
+        // Warp ahead 1 second
+        vm.warp(block.timestamp + 1);
+
+        // Even though we have already proven this withdrawalHash, we should be allowed to re-submit
+        // our proof with a changed outputRoot
+        vm.expectEmit(true, true, true, true);
+        emit WithdrawalProven(_withdrawalHash, alice, bob);
+        op.proveWithdrawalTransaction(
+            _defaultTx,
+            _proposedBlockNumber,
+            _outputRootProof,
+            _withdrawalProof
+        );
+
+        // Ensure that the withdrawal was updated within the mapping
+        (, uint128 timestamp, ) = op.provenWithdrawals(_withdrawalHash);
+        assertEq(timestamp, block.timestamp);
+    }
+
     // Test: proveWithdrawalTransaction succeeds and emits the WithdrawalProven event.
     function test_proveWithdrawalTransaction_validWithdrawalProof_success() external {
         vm.expectEmit(true, true, true, true);