Skip to content
This repository has been archived by the owner on Oct 8, 2023. It is now read-only.

obront - Migration can brick high gas transactions due to delivery cost exceeding block gas limit #98

Closed
sherlock-admin opened this issue Apr 7, 2023 · 6 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity

Comments

@sherlock-admin
Copy link
Contributor

obront

high

Migration can brick high gas transactions due to delivery cost exceeding block gas limit

Summary

The various protections around withdrawal migration do not take into account that withdrawals in the new architecture use far more gas (possibly above L1 block gas limit) before reaching the replayability protection checkpoint, which means some are forever undeliverable.

Vulnerability Detail

Before Bedrock, users have bridged directly using the CrossDomainMessenger (XDM) contract. Bedrock has introduced the OptimismPortal, which all withdrawals must go through, before advancing to the XDM (or delivering the message directly).

The migration encodes all old withdrawals as going through the XDM. Relevant code below:

// MigrateWithdrawal will turn a LegacyWithdrawal into a bedrock
// style Withdrawal.
func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *common.Address) (*Withdrawal, error) {
	// Attempt to parse the value
	value, err := withdrawal.Value()
	if err != nil {
		return nil, fmt.Errorf("cannot migrate withdrawal: %w", err)
	}
	abi, err := bindings.L1CrossDomainMessengerMetaData.GetAbi()
	if err != nil {
		return nil, err
	}
	// Migrated withdrawals are specified as version 0. Both the
	// L2ToL1MessagePasser and the CrossDomainMessenger use the same
	// versioning scheme. Both should be set to version 0
	versionedNonce := EncodeVersionedNonce(withdrawal.XDomainNonce, new(big.Int))
	// Encode the call to `relayMessage` on the `CrossDomainMessenger`.
	// The minGasLimit can safely be 0 here.
	data, err := abi.Pack(
		"relayMessage",
		versionedNonce,
		withdrawal.XDomainSender,
		withdrawal.XDomainTarget,
		value,
		new(big.Int),
		[]byte(withdrawal.XDomainData),
	)
	if err != nil {
		return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err)
	}
	gasLimit := MigrateWithdrawalGasLimit(data)
	w := NewWithdrawal(
		versionedNonce,
		&predeploys.L2CrossDomainMessengerAddr,
		l1CrossDomainMessenger,
		value,
		new(big.Int).SetUint64(gasLimit),
		data,
	)
	return w, nil
}
func MigrateWithdrawalGasLimit(data []byte) uint64 {
	// Compute the cost of the calldata
	dataCost := uint64(0)
	for _, b := range data {
		if b == 0 {
			dataCost += params.TxDataZeroGas
		} else {
			dataCost += params.TxDataNonZeroGasEIP2028
		}
	}
	// Set the outer gas limit. This cannot be zero
	gasLimit := dataCost + 200_000
	// Cap the gas limit to be 25 million to prevent creating withdrawals
	// that go over the block gas limit.
	if gasLimit > 25_000_000 {
		gasLimit = 25_000_000
	}
	return gasLimit
}

During migration, pending withdrawals are decoded and re-encoded as relayMessage calls on the XDM, which need to be finalized on the Portal.

It turns out this process increases the delivery cost substantially because the entire calldata is passed to Portal and then passed to XDM (previously only sent to XDM).

This means that calldata that costs 15M to deliver in the CALL opcode will now cost 30M (since it is passed twice), which is above the L1 block gas limit. Such withdrawals are therefore now undeliverable.

It's important to understand that the calldata cost is mandatory and spent before reaching the replayability mechanism of XDM.

Impact

Withdrawals with calldata costs exceeding 15M will be undeliverable, whereas pre-Bedrock they can be executed successfully.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/op-chain-ops/crossdomain/migrate.go#L73-L84

Additional discussion

This finding draws inspiration from unforgiven's finding, which has since been addressed.

However, we believe the likelihood represented by this issue is several magnitudes higher as any TX over the call data threshold is undeliverable, rather than only TXs with millions of zero bytes.

Tool used

Manual Review

Recommendation

Consider migrating withdrawals directly as failed messages on the XDM. This will mimic the expected pre-Bedrock behavior and ensure they cannot be lost from this cause or from other unforeseen issues.

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Apr 10, 2023
@github-actions github-actions bot reopened this Apr 10, 2023
@github-actions github-actions bot added High A valid High severity issue and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Apr 10, 2023
@hrishibhat
Copy link
Contributor

Sponsor comment:
Closely related to issue 90 submitted by same sherlock user. The maximum withdrawal intrinsic gas usage,
with code golfing, caps out because of L2 not being able to produce a
withdrawal message larger than approximately 0.93 MB because of the 15M legacy L2 gas limit.
Under 15M intrinsic gas, even if the bytes are all non-zero.
And this only applies to crafted code-golfed withdrawals: regular withdrawals that incur a regular calldata
cost on L2 have less gas available to withdraw a large pre-image with, thus a smaller pre-image,
and thus more gas available on L1 to complete the relay.

@hrishibhat hrishibhat added the Sponsor Disputed The sponsor disputed this issue's validity label Apr 16, 2023
@GalloDaSballo
Copy link
Collaborator

Gas Limit on OP is currently 15MLN, verified on the explorer

@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed High A valid High severity issue labels Apr 23, 2023
@zobront
Copy link
Collaborator

zobront commented Apr 25, 2023

Escalate for 10 USDC

The submission states:

During migration, pending withdrawals are decoded and re-encoded as relayMessage calls on the XDM, which need to be finalized on the Portal.

This process increases the delivery cost substantially because the entire calldata is passed to Portal and then passed to XDM (previously only sent to XDM).

This means that calldata that costs 15M to deliver in the CALL opcode will now cost 30M (since it is passed twice), which is above the L1 block gas limit. Such withdrawals are therefore now undeliverable.

Although we admit to have not been aware of the 15MM limit (which stops the specific example outcome of the error described), the root error covered still manifests and can cause bricking of funds.

The gasLimit set on migration is 1x calldata costs, computed into dataCost variable. The 200,000 added does not suffice to cover for 2x calldata costs, as required before reaching XDM. Therefore, with large enough calldata length (far less than the 0.93MB figure raised), an attacker can pass the specified gasLimit and revert in the Portal->XDM call, at which point the withdrawal is marked as spent and bricked.

To summarize: The original gasLimit miscalculation detailed in the issue is correct. While the 15MM L2 gas limit stops one version of the attack, the problem remains and is still significant enough to brick many withdrawals, which is in-scope as high severity issue.

@sherlock-admin
Copy link
Contributor Author

Escalate for 10 USDC

The submission states:

During migration, pending withdrawals are decoded and re-encoded as relayMessage calls on the XDM, which need to be finalized on the Portal.

This process increases the delivery cost substantially because the entire calldata is passed to Portal and then passed to XDM (previously only sent to XDM).

This means that calldata that costs 15M to deliver in the CALL opcode will now cost 30M (since it is passed twice), which is above the L1 block gas limit. Such withdrawals are therefore now undeliverable.

Although we admit to have not been aware of the 15MM limit (which stops the specific example outcome of the error described), the root error covered still manifests and can cause bricking of funds.

The gasLimit set on migration is 1x calldata costs, computed into dataCost variable. The 200,000 added does not suffice to cover for 2x calldata costs, as required before reaching XDM. Therefore, with large enough calldata length (far less than the 0.93MB figure raised), an attacker can pass the specified gasLimit and revert in the Portal->XDM call, at which point the withdrawal is marked as spent and bricked.

To summarize: The original gasLimit miscalculation detailed in the issue is correct. While the 15MM L2 gas limit stops one version of the attack, the problem remains and is still significant enough to brick many withdrawals, which is in-scope as high severity issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Apr 25, 2023
@hrishibhat
Copy link
Contributor

hrishibhat commented May 19, 2023

Escalation rejected

Lead Judge Comment:

Maintaining Low Severity We have gone through all withdrawals and know this will not happen Additionally, 15MLN in calldata is not actually possible due to overhead of withdrawals on current L2 architecture Meaning that the 30MLN cap would not be reached due to calldata itself

@sherlock-admin
Copy link
Contributor Author

Escalation rejected

Lead Judge Comment:

Maintaining Low Severity We have gone through all withdrawals and know this will not happen Additionally, 15MLN in calldata is not actually possible due to overhead of withdrawals on current L2 architecture Meaning that the 30MLN cap would not be reached due to calldata itself

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity
Projects
None yet
Development

No branches or pull requests

4 participants