fix(ctb): Fix `finalizeWithdrawalTransaction` gas dos

[clabby]

Overview

Note WIP

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

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:

1. 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.
2. 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.
  • [x] 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[bot]]

⚠️ No Changeset found

Latest commit: 134816d0493e0116d83caf0b8f7013b991f9675f

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]

Current dependencies on/for this PR:

• develop
  • PR #4954 👈

This comment was auto-generated by Graphite.

[codecov[bot]]

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%)	:arrow_down:
contracts-bedrock-tests	49.87% <100.00%> (+0.12%)	:arrow_up:
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%)	:arrow_up:
op-bindings/hardhat/utils.go	0.00% <0.00%> (-100.00%)	:arrow_down:
op-chain-ops/crossdomain/alias.go	0.00% <0.00%> (-100.00%)	:arrow_down:
op-chain-ops/crossdomain/precheck.go	0.00% <0.00%> (-88.00%)	:arrow_down:
op-chain-ops/crossdomain/message.go	0.00% <0.00%> (-81.25%)	:arrow_down:
op-chain-ops/deployer/deployer.go	0.00% <0.00%> (-77.50%)	:arrow_down:
op-chain-ops/genesis/genesis.go	0.00% <0.00%> (-77.31%)	:arrow_down:
op-chain-ops/crossdomain/withdrawals.go	0.00% <0.00%> (-75.95%)	:arrow_down:
op-chain-ops/genesis/layer_one.go	0.00% <0.00%> (-75.35%)	:arrow_down:
op-chain-ops/immutables/immutables.go	0.00% <0.00%> (-67.37%)	:arrow_down:
... and 43 more

[mergify[bot]]

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

[maurelian]

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]

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.

[mergify[bot]]

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

[clabby]

Closing in favor of #5017