sherlock-audit / 2023-03-optimism-judging

7 stars 0 forks source link

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

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

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.

hrishibhat commented 1 year ago

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.

GalloDaSballo commented 1 year ago

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

zobront commented 1 year ago

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 commented 1 year ago

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.

hrishibhat commented 1 year ago

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 commented 1 year ago

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.