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

xiaoming90 - Malicious Withdrawals Might Be Migrated #246

Closed
github-actions bot opened this issue Feb 20, 2023 · 5 comments
Closed

xiaoming90 - Malicious Withdrawals Might Be Migrated #246

github-actions bot opened this issue Feb 20, 2023 · 5 comments
Labels
Non-Reward This issue will not receive a payout

Comments

@github-actions
Copy link

xiaoming90

high

Malicious Withdrawals Might Be Migrated

Summary

Malicious withdrawals might be migrated, which allows arbitrary messages to be relayed, leading to loss of funds.

Vulnerability Detail

Section 1 - About State Dump File

In pre-bedrock, when someone calls OVM_L2ToL1MessagePasser.passMessageToL1 function, the following Call function in the evm.go will be triggered. Then, the caller address and transaction data will be written to the state dump file at Line 208.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/l2geth/core/vm/evm.go#L202

File: evm.go
202: // Call executes the contract associated with the addr with the given input as
203: // parameters. It also handles any necessary value transfer required and takes
204: // the necessary steps to create accounts and reverses the state in case of an
205: // execution error or failed value transfer.
206: func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas uint64, value *big.Int) (ret []byte, leftOverGas uint64, err error) {
207: 	if addr == dump.MessagePasserAddress {
208: 		statedumper.WriteMessage(caller.Address(), input)
209: 	}

The following new line consists of the caller address (sender) and transaction data (msg) will be added to the state dump file located at L2GETH_STATE_DUMP_PATH environment variable.

MSG|<sender>|<msg>

Section 2 - Converting State Dump File To JSON Files (evm-messages.json)

Before the migration, the following script will be executed to parse the state dump files and generate the evm-messages.json migration data.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/migration-data/bin/cli.ts#L17

File: cli.ts
17: program
18:   .command('parse-state-dump')
19:   .description('parses state dump to json')
20:   .option('--file <file>', 'path to state dump file')
21:   .action(async (options) => {
22:     const iface = getContractInterface('OVM_L2ToL1MessagePasser')
23:     const dump = fs.readFileSync(options.file, 'utf-8')
24: 
25:     const addrs: string[] = []
26:     const msgs: any[] = []
27:     for (const line of dump.split('\n')) {
28:       if (line.startsWith('ETH')) {
29:         addrs.push(line.split('|')[1].replace('\r', ''))
30:       } else if (line.startsWith('MSG')) {
31:         const msg = '0x' + line.split('|')[2].replace('\r', '')
32:         const parsed = iface.decodeFunctionData('passMessageToL1', msg)
33:         msgs.push({
34:           who: line.split('|')[1],
35:           msg: parsed._message,
36:         })
37:       }
38:     }
39: 
40:     fs.writeFileSync(
41:       './data/evm-addresses.json',
42:       JSON.stringify(addrs, null, 2)
43:     )
44:     fs.writeFileSync('./data/evm-messages.json', JSON.stringify(msgs, null, 2))
45:   })

The generated evm-messages.json JSON file will have the following format:

{
    "who": "<caller address>",
    "msg": "<transaction data>"
},
{
    "who": "<caller address>",
    "msg": "<transaction data>"
}

Section 3 - Converting JSON files to migration data (ovmMessages, evmMessages)

During the migration, the evm-messages.json JSON file will be loaded, unmarshalled, and stored in the evmMessages migration data variable.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/genesis/migration_action/action.go#L30

File: action.go
30: func Migrate(cfg *Config) (*genesis.MigrationResult, error) {
31: 	deployConfig := cfg.DeployConfig
..SNIP..
45: 	ovmMessages, err := migration.NewSentMessage(cfg.OVMMessagesPath)
46: 	if err != nil {
47: 		return nil, err
48: 	}
49: 	evmMessages, err := migration.NewSentMessage(cfg.EVMMessagesPath)
50: 	if err != nil {
51: 		return nil, err
52: 	}
53: 
54: 	migrationData := migration.MigrationData{
55: 		OvmAddresses:  ovmAddresses,
56: 		EvmAddresses:  evmAddresess,
57: 		OvmAllowances: ovmAllowances,
58: 		OvmMessages:   ovmMessages,
59: 		EvmMessages:   evmMessages,
60: 	}
..SNIP..
85: 	return genesis.MigrateDB(ldb, deployConfig, block, &migrationData, !cfg.DryRun, cfg.NoCheck)
86: }

Section 4 - Performing migration process

During the migration, the legacy withdrawals from the legacy LegacyMessagePasser contract will be migrated to their new format in the Bedrock L2ToL1MessagePasser contract via the crossdomain.MigrateWithdrawals function at Line 188.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/genesis/db_migration.go#L187

File: db_migration.go
183: 	// Now we migrate legacy withdrawals from the LegacyMessagePasser contract to their new format
184: 	// in the Bedrock L2ToL1MessagePasser contract. Note that we do NOT delete the withdrawals from
185: 	// the LegacyMessagePasser contract. Here we operate on the list of withdrawals that we
186: 	// previously filtered and verified.
187: 	log.Info("Starting to migrate withdrawals", "no-check", noCheck)
188: 	err = crossdomain.MigrateWithdrawals(filteredWithdrawals, db, &config.L1CrossDomainMessengerProxy, noCheck)

The MigrateWithdrawal function below is responsible for turning a legacy withdrawal into a bedrock-style withdrawal. An important point to note here is that the new bedrock style withdrawal is created with the sender hardcoded to &predeploys.L2CrossDomainMessengerAddr and the target hardcoded as l1CrossDomainMessenger as per Line 90-91 below.

Therefore, the original sender and target in the legacy withdrawal will be discarded and replaced with the &predeploys.L2CrossDomainMessengerAddr and l1CrossDomainMessenger respectively. As a result, anyone can submit a legacy withdrawal before the migration and the system will consider it originated from L2CrossDomainMessengerAddr, thus providing an opportunity for malicious users to perform spoofing attacks. This is the root cause of the issue discussed later.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/crossdomain/migrate.go#L54

File: migrate.go
52: // MigrateWithdrawal will turn a LegacyWithdrawal into a bedrock
53: // style Withdrawal.
54: func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *common.Address) (*Withdrawal, error) {
..SNIP..
66: 	// Migrated withdrawals are specified as version 0. Both the
67: 	// L2ToL1MessagePasser and the CrossDomainMessenger use the same
68: 	// versioning scheme. Both should be set to version 0
69: 	versionedNonce := EncodeVersionedNonce(withdrawal.Nonce, new(big.Int))
70: 	// Encode the call to `relayMessage` on the `CrossDomainMessenger`.
71: 	// The minGasLimit can safely be 0 here.
72: 	data, err := abi.Pack(
73: 		"relayMessage",
74: 		versionedNonce,
75: 		withdrawal.Sender,
76: 		withdrawal.Target,
77: 		value,
78: 		new(big.Int),
79: 		withdrawal.Data,
80: 	)
..SNIP..
88: 	w := NewWithdrawal(
89: 		versionedNonce,
90: 		&predeploys.L2CrossDomainMessengerAddr,
91: 		l1CrossDomainMessenger,
92: 		value,
93: 		new(big.Int).SetUint64(gasLimit),
94: 		data,
95: 	)
96: 	return w, nil
97: }

After converting the legacy withdrawal into a bedrock-style withdrawal, it will be inserted into the storage of Bedrock's L2ToL1MessagePasser contract. This will ensure that the relayers can prove the withdrawal via the OptimismPortal.proveWithdrawalTransaction after the migration.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/crossdomain/migrate.go#L22

File: migrate.go
21: // MigrateWithdrawals will migrate a list of pending withdrawals given a StateDB.
22: func MigrateWithdrawals(withdrawals []*LegacyWithdrawal, db vm.StateDB, l1CrossDomainMessenger *common.Address, noCheck bool) error {
23: 	for i, legacy := range withdrawals {
24: 		legacySlot, err := legacy.StorageSlot()
..SNIP..
29: 		if !noCheck {
30: 			legacyValue := db.GetState(predeploys.LegacyMessagePasserAddr, legacySlot)
31: 			if legacyValue != abiTrue {
32: 				return fmt.Errorf("%w: %s", errLegacyStorageSlotNotFound, legacySlot)
33: 			}
34: 		}
35: 
36: 		withdrawal, err := MigrateWithdrawal(legacy, l1CrossDomainMessenger)
..SNIP..
41: 		slot, err := withdrawal.StorageSlot()
..SNIP..
46: 		db.SetState(predeploys.L2ToL1MessagePasserAddr, slot, abiTrue)
47: 		log.Info("Migrated withdrawal", "number", i, "slot", slot)
48: 	}
49: 	return nil
50: }

Section 5 - Permissionless OVM_L2ToL1MessagePasser.passMessageToL1 function

Before the migration (pre-bedrock), anyone can call the passMessageToL1 function within the Message Passer Contract (OVM_L2ToL1MessagePasser) to submit a withdrawal. The transaction data (_message) and the caller address (msg.sender) will be encoded and hashed to produce a key for the sentMessages mapping.

As mentioned in the comments, there is a mechanism within the legacy L1CrossDomainMessenger._verifyStorageProof() contract to ensure that only messages sent from L2CrossDomainMessenger will be relayed before the migration (pre-bedrock). An important point to note is that this mechanism is no longer used in bedrock after the migration.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts/contracts/L2/predeploys/OVM_L2ToL1MessagePasser.sol#L29

File: OVM_L2ToL1MessagePasser.sol
07: /**
08:  * @title OVM_L2ToL1MessagePasser
09:  * @dev The L2 to L1 Message Passer is a utility contract which facilitate an L1 proof of the
10:  * of a message on L2. The L1 Cross Domain Messenger performs this proof in its
11:  * _verifyStorageProof function, which verifies the existence of the transaction hash in this
12:  * contract's `sentMessages` mapping.
13:  */
14: contract OVM_L2ToL1MessagePasser is iOVM_L2ToL1MessagePasser {
15:     /**********************
16:      * Contract Variables *
17:      **********************/
18: 
19:     mapping(bytes32 => bool) public sentMessages;
20: 
21:     /********************
22:      * Public Functions *
23:      ********************/
24: 
25:     /**
26:      * @inheritdoc iOVM_L2ToL1MessagePasser
27:      */
28:     // slither-disable-next-line external-function
29:     function passMessageToL1(bytes memory _message) public {
30:         // Note: although this function is public, only messages sent from the
31:         // L2CrossDomainMessenger will be relayed by the L1CrossDomainMessenger.
32:         // This is enforced by a check in L1CrossDomainMessenger._verifyStorageProof().
33:         sentMessages[keccak256(abi.encodePacked(_message, msg.sender))] = true;
34:     }
35: }

Section 6 - Crafting Malicious Payload

Assume that the following data will be the payload passes to the OVM_L2ToL1MessagePasser.passMessageToL1 function.

abi.encodeWithSignature(
    "relayMessage(address,address,bytes,uint256)",
    _target,
    _sender,
    _message,
    _messageNonce
);

The _target is set to L1Bridge address and the _sender is set to L2Bridge address. The _message will be set to as follows:

abi.encodeWithSelector(
    this.finalizeBridgeETH.selector,
    _from,
    _to,
    _amount,
    _extraData
),

The _to is set to the attacker's wallet and _amount is set to a sufficiently large ETH amount that the L1 bridge is expected to hold.

Section 7 - Exploitation

Right before the migration start, the attacker can submit the malicious payload in the previous section to the OVM_L2ToL1MessagePasser.passMessageToL1 function. When the migration starts, the attacker's legacy withdrawal will be migrated to the bedrock-style withdrawal, and the sender and target of the malicious withdrawal will be set to &predeploys.L2CrossDomainMessengerAddr and l1CrossDomainMessenger respectively.

After the migration, the attacker can call the prove the malicious withdrawal via the OptimismPortal.proveWithdrawalTransaction. Since the malicious withdrawal has already been inserted into the storage of Bedrock's L2ToL1MessagePasser contract during the migration process (Refer to Section 4), the attacker will have no issue proving the malicious withdrawal.

After the 7-day waiting period, the attacker can call the OptimismPortal.finalizeWithdrawalTransaction to finalize the withdrawal, and the Optimism Portal will call the L1CrossDomainMessenger.relayMessage function to relay the message within the withdrawal. The L1CrossDomainMessenger will in turn call the L1StandardBridge.finalizeBridgeETH function to finalize the ETH bridging, and the ETH will be forwarded to the attacker's address.

At this point, the attacker successfully steals ETH Optimism's L1 infrastructure. Note that the attacker could also drain any ERC20 tokens held by Optimism's L1 infrastructure by switching the selector within the malicious payload to finalizeBridgeERC20 instead.

Impact

The impact is High as it results in loss of funds.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/genesis/migration/types.go#L120

Tool used

Manual Review

Recommendation

It is recommended only to migrate legacy withdrawals initiated by Legacy's L2CrossDomainMessenger.

Update the ToWithdrawals function to only process legacy withdrawals initiated by Legacy's L2CrossDomainMessenger.

func (m *MigrationData) ToWithdrawals() ([]*crossdomain.LegacyWithdrawal, error) {
	messages := make([]*crossdomain.LegacyWithdrawal, 0)
-	for _, msg := range m.OvmMessages {
+	for who, msg := range m.OvmMessages {
+		if who != predeploys.L2CrossDomainMessengerAddr {
+			continue // skip if the message is not initated by L2CrossDomainMessenger
+		}
		wd, err := msg.ToLegacyWithdrawal()
		if err != nil {
			return nil, err
		}
		messages = append(messages, wd)
		if err != nil {
			return nil, err
		}
	}
-	for _, msg := range m.EvmMessages {
+	for who, msg := range m.EvmMessages {
+		if who != predeploys.L2CrossDomainMessengerAddr {
+			continue // skip if the message is not initated by L2CrossDomainMessenger
+		}
		wd, err := msg.ToLegacyWithdrawal()
		if err != nil {
			return nil, err
		}
		messages = append(messages, wd)
	}
	return messages, nil
}
@github-actions github-actions bot added the High A valid High severity issue label Feb 20, 2023
@rcstanciu
Copy link
Contributor

rcstanciu commented Feb 21, 2023 •
edited by Evert0x
Loading

Comment from Optimism

Description: Withdrawals not from the L2 Crossdomain Messenger halt the migration process

Reason: When migrationData.ToWithdrawals() is called, any tx that isn’t from L2CrossDomainMessenger will throw an error. This will cause a halt of the migration process.

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Feb 21, 2023
@Evert0x Evert0x added Medium A valid Medium severity issue and removed High A valid High severity issue labels Feb 21, 2023
@maurelian
Copy link

I suspect that this issue may in fact be a duplicate of #105, and IMO should be subject to escalation without staking.

@xiaoming9090
Copy link

Hi @maurelian, I have a look at Issue 105. I would like to clarify that this issue is not a duplicate of Issue 105.

For issue 105, the report highlights a bug in the Decode function within the legacy_withdrawal.go that will cause the migration process to halt.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/crossdomain/legacy_withdrawal.go#L58-L66

File: legacy_withdrawal.go
52: // Decode will decode a serialized LegacyWithdrawal
53: func (w *LegacyWithdrawal) Decode(data []byte) error {
54: 	if len(data) < len(predeploys.L2CrossDomainMessengerAddr)+4 {
55: 		return fmt.Errorf("withdrawal data too short: %d", len(data))
56: 	}
57: 
58: 	selector := crypto.Keccak256([]byte("relayMessage(address,address,bytes,uint256)"))[0:4]
59: 	if !bytes.Equal(data[0:4], selector) {
60: 		return fmt.Errorf("invalid selector: 0x%x", data[0:4])
61: 	}
62: 
63: 	msgSender := data[len(data)-len(predeploys.L2CrossDomainMessengerAddr):]
64: 	if !bytes.Equal(msgSender, predeploys.L2CrossDomainMessengerAddr.Bytes()) {
65: 		return errors.New("invalid msg.sender")
66: 	}

However, in this report, it is highlighting a bug in the MigrationData.ToWithdrawals function within the types.go that cause the migration process to halt.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/genesis/migration/types.go#L120

File: types.go
120: func (m *MigrationData) ToWithdrawals() ([]*crossdomain.LegacyWithdrawal, error) {
121: 	messages := make([]*crossdomain.LegacyWithdrawal, 0)
122: 	for _, msg := range m.OvmMessages {
123: 		wd, err := msg.ToLegacyWithdrawal()
124: 		if err != nil {
125: 			return nil, err
126: 		}
127: 		messages = append(messages, wd)
128: 		if err != nil {
129: 			return nil, err
130: 		}
131: 	}
132: 	for _, msg := range m.EvmMessages {
133: 		wd, err := msg.ToLegacyWithdrawal()
134: 		if err != nil {
135: 			return nil, err
136: 		}
137: 		messages = append(messages, wd)
138: 	}
139: 	return messages, nil
140: }

These are two distinct bugs found in two different contracts. As such, they should be rewarded separately. We should not group all bugs that halt the migration under one issue, unless the remediation action to fix the issue is the same.

If the OP team only refers to Issue 105, they would only patch the bug in the Decode function of legacy_withdrawal.go, but that would not stop someone from halting the migration process.

To prevent someone from halting the migration process, the OP team would also need to fix the bug highlighted in this report, which is to patch the bug in the MigrationData.ToWithdrawals function of types.go. As far as I know, I believe there are no other reports in this contest that highlight that there is a bug in the MigrationData.ToWithdrawals function of types.go.

I hope that help to clarify the distinction between Issue 105 and this report. Let me know if any further clarification is needed. Thanks.

@zobront
Copy link
Collaborator

zobront commented Feb 23, 2023 •
edited
Loading

Hi @xiaoming9090 — can you clarify which bug in MigrationData.ToWithdrawals you are referring to?

As far as I can tell, MigrationData.ToWithdrawals would not cause the migration to halt. This can be seen in your Recommended Fix, which adds a check that would halt migration where none exists now. That is why your report incorrectly assumed that the fraudulent withdrawal would be migrated.

Instead, the reason it crashes is that MigrationData.ToWithdrawals calls msg.ToLegacyWithdrawal which calls w.Decode, which is where the bug in #105 occurs.

@Evert0x
Copy link
Contributor

Evert0x commented Mar 2, 2023

Downgrading to false as this attack describe is not possible, and the proposed attack will instead lead to the migration crash described on #105 (and the Watson for this issue already submitted #232, which is an approved dup of #105). Here is the explanation of why the attack is not valid:

@Evert0x Evert0x closed this as completed Mar 2, 2023
@rcstanciu rcstanciu added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue Reward A payout will be made for this issue labels Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Non-Reward This issue will not receive a payout
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rcstanciu @zobront @maurelian @Evert0x @sherlock-admin @xiaoming9090