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

0xdeadbeef - Migration will not succeed because of a mismatch between witness data and expected data #113

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

0xdeadbeef

high

Migration will not succeed because of a mismatch between witness data and expected data

Summary

Witness data is gathered in order to perform the migration from pre-bedrock to bedrock.
Currently, the witness data is generated through iteration of messages that get written to a file on every call to the the OVM_L2ToL1MessagePasser contract.

The migration scripts assumes all calls to the OVM_L2ToL1MessagePasser are from the L2CrossDomainMessenger and are to the relayMessage selector.

Until a few days ago, the assumption held until someone called the OVM_L2ToL1MessagePasser directly on mainnet:
https://optimistic.etherscan.io/tx/0xa07e49d05fab5c69613c272ca13d62d7888959460f28fb0749bddf3fb62ccf7f

The new witness data that will be generated will cause the migration script to fail.

An attacker can do the same to intentionally cause the migration to fail.

Vulnerability Detail

Pre-bedrock l2geth records every message sent to OVM_L2ToL1MessagePasser in the evm implementation of call:
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/l2geth/core/vm/evm.go#L206-L209

The witness data is created based on the messages recorded by statedumper. The messages are later parsed into a json file.
An example of the parsing can be seen in the following utility:
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/migration-data/bin/cli.ts

The parsed message end up in a format that extracts the message sender and the message itself.
The migration script attempts to create a SentMessage for every message recorded and then convert the message to LegacyWithdrawal.
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/genesis/migration/types.go#L23-L53

The issue rises when attempting to call LegacyWithdrawal.Decode() on the message data to convert it to a withdrawal.
There are checks that:

  1. The message selector is relayMessage(address,address,bytes,uint256)
  2. Validate that who (caller to OVM_L2ToL1MessagePasser) is L2CrossDomainMessenger
    https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/crossdomain/legacy_withdrawal.go#L58-L66

The above checks are valid if OVM_L2ToL1MessagePasser was called through L2CrossDomainMessenger but are invalid if the OVM_L2ToL1MessagePasser was called directly.

The code will return an error and the error will bubble up to stop the migration and cause it to fail.

Impact

Migration will be halted. Will not be able to continue without clearing the witness data or upgrading the migration code.
As the chain is paused during migration, any delay will cause business disruption to protocols/users using optimism.

Code Snippet

Added in the description

Tool used

Manual Review

Recommendation

Either:

  1. Instead of bubbling up the error, skip invalid messages and create only valid withdrawals
  2. Filter our messages that are not to relayMessage and are not from L2CrossDomainMessenger during the creation of the witness data. If this is chosen, the noCheck flag will be forced to be enabled to not cause errors in PreCheckWithdrawals which check that all the withdrawals are matching the storage slots of OVM_L2ToL1MessagePasser

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