fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos #4954

clabby · 2023-02-23T03:22:27Z

Overview

Note
WIP

Warning
In advance, this fix introduces a major landmine and is not pretty. Needs thorough review- scrutinize away 😄

Introduces a fix for an issue where users could DoS withdrawal transactions by exploiting one of two bugs in the finalizeWithdrawalTransaction function of the OptimismPortal:

A certain amount of gas is consumed between the GAS opcode in the check for whether or not the callframe has enough gas remaining and the CALL opcode invoked in the SafeCall library itself (This figure is 230 gas after these changes - See Fixes for how it was derived). If the minGasLimit specified for the withdrawal transaction is within 230 gas of the actual cost of execution, a malicious user can call finalizeWithdrawalTransaction with an amount of gas that passes the check, but causes the external call to receive less than minGasLimit gas.
EIP-150 specifies that ALL BUT 1/64th of remaining gas may be forwarded to an external call. Even if the above issue is
fixed, a critical problem remains:
- We need to assert that at least minGasLimit gas is always sent to the external call performed by the OptimismPortal's finalizeWithdrawalTransaction function.
- At the time of the call, this means we need at least minGasLimit * 64 / 63 gas remaining per EIP-150.
- For any situation where minGasLimit / 64 > 19770 (minGasLimit > 1_265_280), the caller can specify an amount of gas for finalizeWithdrawalTransaction that passes the check, but sends less than minGasLimit to the external call due to the implicit truncation performed by the EVM.

TODO

Tests
- Clean up existing tests and verify assumptions.
- Manually verify the integrity of the system of linear inequalities established by the assertions.
- Upstream support for asserting gas passed to a call to foundry for a higher level of assurance. (New expectCall variant)
- Maybe halmos could be useful here?

Invariants

An external call performed by finalizeWithdrawalTransaction MUST ALWAYS be supplied at LEAST the minGasLimit specified by the user who initiated the withdrawal on L2.
- If the callframe does not have enough gas available to send at LEAST minGasLimit gas to the external call, it should ALWAYS revert prior to performing the call.

The ugly

The following assumptions are made in this fix:

The solc version MUST be 0.8.15 for the OptimismPortal.
The optimizer_runs configuration value in foundry.toml MUST be 999_999.
The new _safeCall function in the OptimismPortal MUST not be changed without altering GAS_CHECK_BUFFER.

If any of the above items are changed, the GAS_CHECK_BUFFER MUST be altered to account for the change in gas consumed between the min gas limit check and the CALL opcode invoked within _safeCall.

Fixes

First, let's start with accounting for the gas consumed between the check for the min gas limit and the external call itself.

To find the amount of gas consumed in this window, we need the gas consumed between the GAS opcode invoked in the check for sufficient remaining gas and the CALL opcode invoked by the SafeCall library.

Compiler settings:

Optimizer runs: 999_999
Solc version: 0.8.15

Debugged Test Case: test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds

Pre EIP-150 fix

PC	OPCODE	GAS USED BEFORE OPCODE EXECUTION	DESC
8167	`GAS`	`35618`	`GAS` opcode invoked in the check
8321	`GAS`	`35655`	`GAS` opcode invoked in the call to `SafeCall.call`
8542	`CALL`	`35848`	`CALL` opcode invoked in `SafeCall.call`

Gas consumed between check and call: 35848 - 35618 = 230

Post EIP-150 fix (see below)

PC	OPCODE	GAS USED BEFORE OPCODE EXECUTION	DESC
8190	`GAS`	`35770`	`GAS` opcode invoked in the check
8344	`GAS`	`35808`	`GAS` opcode invoked in the call to `SafeCall.call`
8565	`CALL`	`36000`	`CALL` opcode invoked in `SafeCall.call`

Gas consumed between check and call: 36000 - 35770 = 230

Exactly 230 gas is consumed between the invocation of the GAS opcode in the check and the CALL opcode itself. We must account for this gas consumption when passing the gas to SafeCall.call to ensure that at LEAST the min gas limit is forwarded to the call.

bool success = SafeCall.call(
    _target,
-   gasleft() - FINALIZE_GAS_BUFFER,
+   gasleft() - FINALIZE_GAS_BUFFER + GAS_CHECK_BUFFER,
    _value,
    _data
);

In addition to considering the gas consumed between the check and the call itself, we must also consider another factor: EIP-150 specifies that ALL but 1/64th of remaining gas may be forwarded to a call.

We currently check that the callframe has minGasLimit + 20_000 gas remaining a few operations prior to the call. With the above fix (considering the gas consumed between the check and the call), we can now guarantee that the callframe has at LEAST minGasLimit + 19770 gas remaining at the time of the call.

Even with the above fix, for any situation where minGasLimit / 64 > 19770 (minGasLimit > 1_265_280), an attacker can submit a finalizeWithdrawalTransaction call with an amount of gas that passes the required check, but at the time of the call, the gas passed is greater than 63/64ths of the gas remaining. The EVM will silently reduce the amount of gas to 63/64ths of the gas remaining in the callframe, causing the transaction to fail.

Within the remaining gas check in _safeCall, we must account for the 63/64ths rule laid out by EIP-150:

require(
-   gasleft() >= _minGasLimit + FINALIZE_GAS_BUFFER 
+   gasleft() >= (_minGasLimit + FINALIZE_GAS_BUFFER) * 64 / 63,
    "OptimismPortal: insufficient gas to finalize withdrawal"
);

When we combine these two fixes together, we can make the following assertions:

At the time of the check, the callframe has at LEAST (_minGasLimit + 20_000) * 64 / 63 gas remaining.
At the time of the external call, the outer callframe has at LEAST ((_minGasLimit + 20_000) * 64 / 63) - 230 gas remaining due to the above check and factoring in the gas consumed between the check and the call itself.
The SafeCall lib's call will always pass at LEAST (((_minGasLimit + 20_000) * 64 / 63) - 38) - 19770 gas to the external call.
- This is value is derived as follows:
  - Before we specify an amount of gas to pass to the call, we assert that the callframe has at least (_minGasLimit + 20_000) * 64 / 63 gas remaining.
  - At the time of the GAS opcode invocation within the parameters to SafeCall.call, exactly 38 gas has been consumed since the above check.
  - So, we can assert that gasleft() is at LEAST (((_minGasLimit + 20_000) * 64 / 63) - 38) here.
  - We also subtract FINALIZE_GAS_BUFFER - GAS_CHECK_BUFFER from the above value, which brings us to (((_minGasLimit + 20_000) * 64 / 63) - 38) - 19770.

Because we know that the call will always receive (((_minGasLimit + 20_000) * 64 / 63) - 38) - 19770, we can solve the following inequality to show that the external call will always receive at least _minGasLimit gas:

$$ (((\text{minGasLimit} + 20000) * \frac{64}{63}) - 38) - 19770 > \text{minGasLimit} $$

This inequality holds true for all $\text{minGasLimit}$ values in the range $(-32096, \infty)$, and because we're dealing with unsigned integers, $[0, \infty)$.

Metadata

Fixes CLI-3386
Fixes CLI-3387

changeset-bot · 2023-02-23T03:22:30Z

⚠️ No Changeset found

Latest commit: 134816d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

clabby · 2023-02-23T03:22:40Z

Current dependencies on/for this PR:

develop
- PR fix(ctb): Fix finalizeWithdrawalTransaction gas dos #4954 👈

This comment was auto-generated by Graphite.

tynes · 2023-02-23T04:33:21Z

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

+            }
+
+            // Revert back to the snapshot state (before the withdrawal was finalized).
+            assertTrue(vm.revertTo(snapshotId));


If this resets hot storage slots thats amazing

It does 😄

The issue still remains with any storage slots that were warm before the snapshot due to deployment / proving happening in the same transaction, though. This means the slot for the withdrawalHash within the provenWithdrawals mapping is warm- same for the slot for the withdrawal's corresponding output in the L2OutputOracle's l2Outputs array, etc.

codecov · 2023-02-23T04:33:54Z

Codecov Report

Merging #4954 (134816d) into develop (5d19411) will decrease coverage by 6.08%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4954      +/-   ##
===========================================
- Coverage    40.90%   34.82%   -6.08%     
===========================================
  Files          324      300      -24     
  Lines        19677    17277    -2400     
  Branches       770      770              
===========================================
- Hits          8048     6016    -2032     
+ Misses       11019    10878     -141     
+ Partials       610      383     -227

Flag	Coverage Δ
bedrock-go-tests	`27.39% <ø> (-8.84%)`	⬇️
contracts-bedrock-tests	`49.87% <100.00%> (+0.12%)`	⬆️
contracts-tests	`98.86% <ø> (ø)`
core-utils-tests	`60.41% <ø> (ø)`
dtl-tests	`47.15% <ø> (ø)`
fault-detector-tests	`33.88% <ø> (ø)`
sdk-tests	`38.74% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../contracts-bedrock/contracts/L1/OptimismPortal.sol	`87.75% <100.00%> (+0.52%)`	⬆️
op-bindings/hardhat/utils.go	`0.00% <0.00%> (-100.00%)`	⬇️
op-chain-ops/crossdomain/alias.go	`0.00% <0.00%> (-100.00%)`	⬇️
op-chain-ops/crossdomain/precheck.go	`0.00% <0.00%> (-88.00%)`	⬇️
op-chain-ops/crossdomain/message.go	`0.00% <0.00%> (-81.25%)`	⬇️
op-chain-ops/deployer/deployer.go	`0.00% <0.00%> (-77.50%)`	⬇️
op-chain-ops/genesis/genesis.go	`0.00% <0.00%> (-77.31%)`	⬇️
op-chain-ops/crossdomain/withdrawals.go	`0.00% <0.00%> (-75.95%)`	⬇️
op-chain-ops/genesis/layer_one.go	`0.00% <0.00%> (-75.35%)`	⬇️
op-chain-ops/immutables/immutables.go	`0.00% <0.00%> (-67.37%)`	⬇️
... and 43 more

mergify · 2023-02-24T18:07:46Z

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

maurelian · 2023-02-24T20:50:54Z

Just wanted to record here the alternative idea of only finalizing the withdrawal if the external call succeeds.

It feels a bit off because there is also replay protection in the L1xDM, but it does more or less make the problem go away.

clabby · 2023-02-24T20:53:49Z

Just wanted to record here the alternative idea of only finalizing the withdrawal if the external call succeeds.

It feels a bit off because there is also replay protection in the L1xDM, but it does more or less make the problem go away.

Yeah, we discussed this yesterday I believe, thanks for shouting here- I much prefer this route, but it changes a few important assumptions about the behavior of the portal late in the game. Interested to hear @smartcontracts' opinion on this when he's back.

…hdrawalTransaction`

smartcontracts · 2023-02-27T21:53:15Z

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

+        VM.deal(address(OP), _defaultTx.value);
+
+        (bool success, bytes memory returndata) = address(OP).call{ gas: gas }(
+            abi.encodeWithSelector(OP.finalizeWithdrawalTransaction.selector, _defaultTx)


Nit: abi.encodeCall will give you type safety here

smartcontracts · 2023-02-27T21:57:49Z

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

+     * @param _value The amount of ETH to send with the call.
+     * @param _data The calldata to send with the call.
+     */
+    function _safeCall(


IMO this could probably be shoved into the SafeCall library (maybe SafeCall.callWithMinGasLimit or something and should be very easy to fuzz test. The idea is that the receiving call frame ALWAYS has at least _minGasLimit.

We could then also use the same logic inside of CrossDomainMessenger.

mergify · 2023-03-01T03:25:13Z

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

clabby · 2023-03-08T17:16:06Z

Closing in favor of #5017

clabby requested a review from tynes February 23, 2023 03:39

tynes reviewed Feb 23, 2023

View reviewed changes

clabby mentioned this pull request Feb 24, 2023

feat(ci-builder/ctb): Update foundry & forge-std #4968

Merged

mergify bot added the conflict label Feb 24, 2023

clabby added 2 commits February 27, 2023 10:07

Initial changes + sketchy tests

fb1151e

Resolve conflicts

673b346

clabby force-pushed the clabby/ctb/min-gas-limit-dos-fix branch from 4d661d1 to 673b346 Compare February 27, 2023 15:16

mergify bot removed the conflict label Feb 27, 2023

clabby added 2 commits February 27, 2023 12:05

Update tests

9254946

Beef up invariant test to fuzz the amount of gas sent to `finalizeWit…

134816d

…hdrawalTransaction`

smartcontracts reviewed Feb 27, 2023

View reviewed changes

clabby mentioned this pull request Feb 28, 2023

fix(ctb): finalizeWithdrawalTransaction gas limit dos (v2) #5017

Merged

mergify bot added the conflict label Mar 1, 2023

clabby closed this Mar 8, 2023

mergify bot removed the conflict label Mar 8, 2023

clabby deleted the clabby/ctb/min-gas-limit-dos-fix branch March 15, 2023 19:03

sherlock-admin mentioned this pull request Apr 7, 2023

ShadowForce - Malicious user can finalize other’s withdrawal with precise amount of gas, leading to loss of funds even after the fix sherlock-audit/2023-03-optimism-judging#37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos #4954

fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos #4954

clabby commented Feb 23, 2023 •

edited

Loading

changeset-bot bot commented Feb 23, 2023 •

edited

Loading

clabby commented Feb 23, 2023

tynes Feb 23, 2023

clabby Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

mergify bot commented Feb 24, 2023

maurelian commented Feb 24, 2023

clabby commented Feb 24, 2023

smartcontracts Feb 27, 2023

smartcontracts Feb 27, 2023

smartcontracts Feb 27, 2023

mergify bot commented Mar 1, 2023

clabby commented Mar 8, 2023

fix(ctb): Fix finalizeWithdrawalTransaction gas dos #4954

fix(ctb): Fix finalizeWithdrawalTransaction gas dos #4954

Conversation

clabby commented Feb 23, 2023 • edited Loading

Overview

TODO

Invariants

The ugly

Fixes

Metadata

changeset-bot bot commented Feb 23, 2023 • edited Loading

⚠️ No Changeset found

clabby commented Feb 23, 2023

tynes Feb 23, 2023

Choose a reason for hiding this comment

clabby Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

mergify bot commented Feb 24, 2023

maurelian commented Feb 24, 2023

clabby commented Feb 24, 2023

smartcontracts Feb 27, 2023

Choose a reason for hiding this comment

smartcontracts Feb 27, 2023

Choose a reason for hiding this comment

smartcontracts Feb 27, 2023

Choose a reason for hiding this comment

mergify bot commented Mar 1, 2023

clabby commented Mar 8, 2023

fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos #4954

fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos #4954

clabby commented Feb 23, 2023 •

edited

Loading

changeset-bot bot commented Feb 23, 2023 •

edited

Loading

clabby Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading