Merge pull request #4057 from ethereum-optimism/clabby/ctb/withdrawal-dos

fix(ctb): Address DoS vulnerability in `OptimismPortal`'s `proveWithdrawalTransaction`

.changeset/beige-clocks-wave.md

+---
+'@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

@@ -150,21 +150,23 @@ 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_ifOutputRootChanges_reverts() (gas: 194874)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 197162)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39589)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 192616)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 192931)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 172988)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233188)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 234938)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 226854)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 329491)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193394)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83433)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50776)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 132205)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 178947)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 195163)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 197363)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39634)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 192839)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 193168)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 173211)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233445)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 235161)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 227144)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 329759)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193617)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83389)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50754)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 136647)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() (gas: 276836)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 188898)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 179214)
 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,8 +189,19 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         // and to prevent replay attacks.
         bytes32 withdrawalHash = Hashing.hashWithdrawal(_tx);
 
+        // Load the ProvenWithdrawal into memory
+        ProvenWithdrawal memory provenWithdrawal = provenWithdrawals[withdrawalHash];
+
+        // Only allow re-proving a withdrawal transaction if the output root has changed.
+        require(
+            provenWithdrawal.timestamp == 0 ||
+                (_l2BlockNumber == provenWithdrawal.l2BlockNumber &&
+                    outputRoot != provenWithdrawal.outputRoot),
+            "OptimismPortal: withdrawal hash 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(
@@ -202,9 +213,9 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         );
 
         // Designate the withdrawalHash as proven by storing the `outputRoot`, `timestamp`,
-        // and `l2BlockNumber` in the `provenWithdrawals` mapping. A certain withdrawal
-        // can be proved multiple times and thus overwrite a previously stored `ProvenWithdrawal`,
-        // but this is safe due to the replay check in `finalizeWithdrawalTransaction`.
+        // and `l2BlockNumber` in the `provenWithdrawals` mapping. A withdrawalHash can only
+        // be proven once 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

@@ -375,6 +375,71 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         );
     }
 
+    // Test: proveWithdrawalTransaction reverts if the passed transaction's withdrawalHash has
+    // already been proven.
+    function test_proveWithdrawalTransaction_replayProve_reverts() external {
+        vm.expectEmit(true, true, true, true);
+        emit WithdrawalProven(_withdrawalHash, alice, bob);
+        op.proveWithdrawalTransaction(
+            _defaultTx,
+            _proposedBlockNumber,
+            _outputRootProof,
+            _withdrawalProof
+        );
+
+        vm.expectRevert("OptimismPortal: withdrawal hash has already been proven");
+        op.proveWithdrawalTransaction(
+            _defaultTx,
+            _proposedBlockNumber,
+            _outputRootProof,
+            _withdrawalProof
+        );
+    }
+
+    // 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);