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

Bobface - DoS during the migration process by calling L2 message passer directly #11

Closed
github-actions bot opened this issue Feb 20, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@github-actions
Copy link

github-actions bot commented Feb 20, 2023

Bobface

medium

DoS during the migration process by calling L2 message passer directly

Summary

The migration can be temporarily DoS'ed by directly calling the legacy OVM_L2ToL1MessagePasser predeploy directly, shortly before the process starts.

The Bedrock migration involves upgrading system contracts and modifying contract state while the chain is halted. This includes upgrading the legacy OVM_L2ToL1MessagePasser predeploy to the new L2ToL1MessagePasser predeploy. Since the legacy contract can hold pending cross-chain messages, its state is also converted and directly written to the new contract's storage.

The legacy predeploy is not intended to be called directly by users or contracts, since messages submitted this way are not executable on L1 due to a check in the L1 messenger. All L2 -> L1 cross-chain messages (aka withdrawals) are designed to flow through the L2 CrossDomainMessenger. Calling the predeploy directly is possible however, since the OVM_L2ToL1MessagePasser.passMessageToL1 function is public, though this likely never happens.

Calling the legacy contract directly, and not through the CrossDomainMessenger as intended, shortly before the migration starts can cause checks during the migration to fail.

Vulnerability Detail

During the migration the migration-data package is used to read state off the chain and store it in a JSON file which is later used to make state changes to the contracts. This includes reading and saving cross-chain messages from the OVM_L2ToL1MessagePasser.

This package is unfortunately not really documented and there seem to be two ways in which the script can be run:

  • evm-sent-messages will include only messages which were submitted through the CrossDomainMessenger
  • parse-state-dump will additionally include messages which were submitted by directly calling the predeploy without going through the CrossDomainMessenger. Note that if this would not be true and the state dump also only contains messages sent through the CrossDomainMessenger, this variant would effectively turn into Variant 1.

Both variants can lead to DoS, though through a differing code path.

Variant 1: Using evm-sent-messages

When the script is invoked with the evm-sent-messages parameter, it will read the messages by filtering CrossDomainMessenger's events. Thus, the resulting JSON file will not include messages sent by directly calling the predeploy.

During the migration, the node's database is migrated. The previously generated JSON files are read again

// op-chain-ops/cmd/op-migrate/main.go
// ...
evmMessages, err := migration.NewSentMessage(ctx.String("evm-messages"))
if err != nil {
    return err
}

migrationData := migration.MigrationData{
    OvmAddresses:  ovmAddresses,
    EvmAddresses:  evmAddresess,
    OvmAllowances: ovmAllowances,
    OvmMessages:   ovmMessages,
    EvmMessages:   evmMessages,
}

and the migration is started by calling MigrateDB. In MigrateDB, the messages are sanity checked:

// op-chain-ops/genesis/db_migration.go
log.Info("Checking withdrawals...")
filteredWithdrawals, err = crossdomain.PreCheckWithdrawals(db, unfilteredWithdrawals)
if err != nil {
    return nil, fmt.Errorf("withdrawals mismatch: %w", err)
}

If there is an error during this check, the application will exit.

PreCheckWithdrawals will read all storage slots from the OVM_L2ToL1MessagePasser predeploy

// op-chain-ops/crossdomain/precheck.go
// Build a mapping of the slots of all messages actually sent in the legacy system.
var count int
slotsAct := make(map[common.Hash]bool)
err := db.ForEachStorage(predeploys.LegacyMessagePasserAddr, func(key, value common.Hash) bool {
    // When a message is inserted into the LegacyMessagePasser, it is stored with the value
    // of the ABI encoding of "true". Although there should not be any other storage slots, we
    // can safely ignore anything that is not "true".
    if value != abiTrue {
        // Should not happen!
        log.Error("found unknown slot in LegacyMessagePasser", "key", key.String(), "val", value.String())
        return true
    }

    // Slot exists, so add it to the map.
    slotsAct[key] = true
    count++
    return true
})

and then checks that for all read storage slots, a corresponding entry exists in the previously read list:

// op-chain-ops/crossdomain/precheck.go
// Iterate over the list of actual slots and check that we have an input message for each one.
for slot := range slotsAct {
    _, ok := slotsInp[slot]
    if !ok {
        return nil, fmt.Errorf("unknown storage slot in state: %s", slot)
    }
}

Since the list was generated by reading only messages which were submitted through the CrossDomainMessenger, a message which was sent by directly calling the predeploy will not be included, this check will fail, and the database migration aborts.

Variant 2: Using parse-state-dump

When the script is invoked with the parse-state-dump parameter, it will not read the chain through a RPC but instead use a supplied state dump file, from which it will extract all cross-chain messages. Other than in variant 1, this may include messages which were directly submitted to the predeploy without going through the CrossDomainMessenger. The PreCheckWithdrawals check will thus pass without issue.

Before PreCheckWithdrawals is called, the encoded byte messages from the JSON files are read into the LegacyWithdrawal data structure using MigrationData.ToWithdrawals() which in turn calls msg.ToLegacyWithdrawal() which then calls LegacyWithdrawal.decode(data []byte). The data parameter is the message parameter supplied to OVM_L2ToL1MessagePasser .passMessageToL1 with msg.sender appended to the end. When the CrossDomainMessenger submits the cross-chain message, its address will be appended to the end. If however, the predeploy is called directly, the caller's address would be appended to the end. This address is later checked:

// op-chain-ops/crossdomain/legacy_withdrawal.go
msgSender := data[len(data)-len(predeploys.L2CrossDomainMessengerAddr):]
if !bytes.Equal(msgSender, predeploys.L2CrossDomainMessengerAddr.Bytes()) {
    return errors.New("invalid msg.sender")
}

If msg.sender is not the CrossDomainMessenger, this check will error and the database migration will abort. Since the direct call to the predeploy is included in the list, this check can be caused to fail.

Impact

Abusing this behaviour can cause the duration during which the network is halted to be significantly extended. An attacker could time calling the predeploy directly just shortly before the migration starts, which will result in previous dry-runs to not catch this issue. If the chain is then down and this issue appears, it would need to first be investigated, a hotfix developed, the hotfix strictly audited since this affects a critical part of the system, and the fix pushed out. This would effectively cause a DoS to the system during the migration process.

I therefore believe this report should fall into the Client node vulnerabilities - Medium - DoS attacks on critical services category.

Code Snippet

Available in the report above

Tool used

Manual Review

Recommendation

The pre-checks and conversions need to be designed in a way to handle direct calls to the predeploy. All messages should be read off the chain, not just the ones which were submitted through the CrossDomainMessenger. Then during database migration, iterate over the messages and drop the ones which were submitted through a direct call. This will effectively remove them from the state, but this should not cause any issues, since these messages would not be executable on L1 in any case.

Duplicate of #105

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue labels Feb 20, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant