unforgiven - [High] Function MigrateWithdrawal() may set gas limit so high for old withdrawals when migrating them by mistake and they can't be relayed in the L1 and users funds would be lost

[github-actions]

unforgiven

high

[High] Function MigrateWithdrawal() may set gas limit so high for old withdrawals when migrating them by mistake and they can't be relayed in the L1 and users funds would be lost

Summary

Function MigrateWithdrawal() in migrate.go will turn a LegacyWithdrawal into a bedrock style Withdrawal. it should set a min gas limit value for the withdrawals. to calculate a gas limit contract overestimates it and if the value goes higher than L1 maximum gas in the block then the withdraw can't be relayed in the L1 and users funds would be lost while the withdraw could be possible before the migration it won't be possible after it.

Vulnerability Detail

This is MigrateWithdrawal() code:

// 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.Nonce, 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.Sender,
		withdrawal.Target,
		value,
		new(big.Int),
		withdrawal.Data,
	)
	if err != nil {
		return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err)
	}

	// Set the outer gas limit. This cannot be zero
	gasLimit := uint64(len(data)*16 + 200_000)

	w := NewWithdrawal(
		versionedNonce,
		&predeploys.L2CrossDomainMessengerAddr,
		l1CrossDomainMessenger,
		value,
		new(big.Int).SetUint64(gasLimit),
		data,
	)
	return w, nil
}

As you can see it sets the gas limit as gasLimit := uint64(len(data)*16 + 200_000) and contract set 16 gas per data byte but in Ethereum when data byte is 0 then the overhead intrinsic gas is 4 and contract overestimate the gas limit by setting 16 gas for each data. this can cause messages with big data(which calculated gas is higher than 30M) to not be relay able in the L1 because if transaction gas set lower than calculated gas then OptimisimPortal would reject it and if gas set higher than calculated gas then miners would reject the transaction. while if code correctly estimated the required gas the gas limit could be lower by the factor of 4.
for example a message with about 2M zeros would get gas limit higher than 30M and it won't be withdrawable in the L1 while the real gas limit is 8M which is relayable.

Impact

some withdraw messages from L2 to L1 that could be relayed before the migration can't be relayed after the migration because of the wrong gas estimation.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/op-chain-ops/crossdomain/migrate.go#L83-L88

Tool used

Manual Review

Recommendation

calculate gas estimation correctly, 4 for 0 bytes and 16 for none zero bytes.

[rcstanciu]

Comment from Optimism

---

Description: Incorrect gas limit calculation for migrated withdrawals

Reason: This is a legitimate issue.

Action: Implement correct gas estimation

[0xunforgiven]

Escalate for 111 USDC

This should labeled as High because some of old legit withdrawals(L2 -> L1) won't be executed in the L1 after the bedrock migration. Those withdrawals belongs to L2CrossDomainMessages which promised to be delivered in L1.

Users may lose funds and there is no limit for it. there may be users who wants to withdraw 1M funds with L2StandardBridge and because of this issue their funds would be lost even so the L2StandardBridge is supposed to deliver the funds!
The withdrawals with large amount of data isn't that rare, There may be other protocols or users in L2 which uses the bridge to store data in the L1 and send withdrawals with large data to the L2CrossDomainMessenger and because of the issue their withdrawal message won't be finalized in L1 after migration.

It is true that this would only happen to some of the withdrawal messages but as we have other gas related issues that blocks withdrawal of limited number of withdrawals(#96 which is only happen to withdrawals that would revert if gas was 5K lower and #109 which happened to withdrawals with higher than ~1M gas) and they considered as High so to be fair This issue should be High too.

This is direct fund loss if it happens to user ETH bridges and also cause L2CrossDomainMessenger's messages to not finalized and be replayable in L1 if it happens to it's withdrawal messages.

[sherlock-admin]

Escalate for 111 USDC

This should labeled as High because some of old legit withdrawals(L2 -> L1) won't be executed in the L1 after the bedrock migration. Those withdrawals belongs to L2CrossDomainMessages which promised to be delivered in L1.

Users may lose funds and there is no limit for it. there may be users who wants to withdraw 1M funds with L2StandardBridge and because of this issue their funds would be lost even so the L2StandardBridge is supposed to deliver the funds!
The withdrawals with large amount of data isn't that rare, There may be other protocols or users in L2 which uses the bridge to store data in the L1 and send withdrawals with large data to the L2CrossDomainMessenger and because of the issue their withdrawal message won't be finalized in L1 after migration.

It is true that this would only happen to some of the withdrawal messages but as we have other gas related issues that blocks withdrawal of limited number of withdrawals(#96 which is only happen to withdrawals that would revert if gas was 5K lower and #109 which happened to withdrawals with higher than ~1M gas) and they considered as High so to be fair This issue should be High too.

This is direct fund loss if it happens to user ETH bridges and also cause L2CrossDomainMessenger's messages to not finalized and be replayable in L1 if it happens to it's withdrawal messages.

You've created a valid escalation for 111 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[Evert0x]

Escalation rejected as there is insufficient evidence for high severity.

[sherlock-admin]

Escalation rejected as there is insufficient evidence for high severity.

This issue's escalations have been rejected!

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