xiaoming90 - Migration Process Can Be DOSed By Anyone

[github-actions]

xiaoming90

medium

Migration Process Can Be DOSed By Anyone

Summary

The migration process can be DOSed by anyone causing reputation damage, delaying the migration, and causing griefing to the OP team.

Vulnerability Detail

Section 1 - OVM_L2ToL1MessagePasser.passMessageToL1 function

In pre-bedrock, OVM_L2ToL1MessagePasser.passMessageToL1 function is permissionless. Thus, anyone can call this function with an arbitrary _message. Under normal circumstances, it is expected that only the L2CrossDomainMessenger will call this function. However, since it is permissionless, malicious users can call this function to insert withdrawal into the storage slot of the OVM_L2ToL1MessagePasser contract, which will prove to be problematic later during the migration.

Subsequently, the caller's address and their _message will be hashed and encoded to generate the transaction hash to be used as the key for the sentMessages mapping. The contract's sentMessages mapping is used to verify the existence of the transaction hash that the user has submitted.

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

File: OVM_L2ToL1MessagePasser.sol
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:     }

Section 2 - About State Dump File

When someone calls OVM_L2ToL1MessagePasser.passMessageToL1 function, the following Call function in the evm.go will be triggered. Within the Call function, it will call the WriteMessage function of the state dumper at Line 208.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/op-geth/core/vm/evm.go

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 caller address and transaction data pass into the WriteMessage function and these data will be written to the state dump file located at L2GETH_STATE_DUMP_PATH environment variable.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/l2geth/statedumper/dumper.go#L55

File: dumper.go
55: func (s *FileStateDumper) WriteMessage(sender common.Address, msg []byte) {
56: 	s.mtx.Lock()
57: 	defer s.mtx.Unlock()
58: 	if _, err := s.f.Write([]byte(fmt.Sprintf("MSG|%s|%x\n", sender.Hex(), msg))); err != nil {
59: 		panic(err)
60: 	}
61: }

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

MSG|<sender>|<msg>

Assume that Alice's address is 0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b. If she calls the OVM_L2ToL1MessagePasser.passMessageToL1 function with an arbitrary message, the following line will be found in the state dump file.

MSG|0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b|<tx_data_from_alice>

Section 3 - Converting State Dump File To SentMessageJSON objects in 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#L18

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>"
}

Referring back to the same example in the previous section, assume that Alice's address is 0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b, then one of the lines within the state dumper file will be as follows:

MSG|0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b|<tx_data_from_alice>

As such, the generated evm-messages.json JSON file will be as follows:

..other SentMessageJSON objects..
{
    "who": "0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b",
    "msg": "<tx_data_from_alice>"
},
  {
    "who": "0x4200000000000000000000000000000000000007",
    "msg": "0xcbd4ece900000000000000000000000099c9fc46f92e8a1c0dec1b1747d010903e884be100000000000000000000000042000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000261600000000000000000000000000000000000000000000000000000000000000a41532ec340000000000000000000000008d126b531e39f1838db27bc88e861a76612b96970000000000000000000000008d126b531e39f1838db27bc88e861a76612b96970000000000000000000000000000000000000000000000000029110738fc7ac20000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
  },
..other SentMessageJSON objects..

Almost all of the SentMessageJSON objects will have their who key set to 0x4200000000000000000000000000000000000007 as this is the address of the L2CrossDomainMessenger.

However, there is one (1) SentMessageJSON object that has its who key set to 0x536fbBaE279fd77FAe3E29b410f7B605bf45BC8b which is Alice's address. Note that this one JSON object will be the reason that breaks the migration process.

Section 4 - Execution Will Abort During Migration

During the migration, it will attempt to convert a SentMessageJSON object to a LegacyWithdrawal struct as shown below. The Decode function will be called during the conversion.

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

File: types.go
40: // ToLegacyWithdrawal will convert a SentMessageJSON to a LegacyWithdrawal
41: // struct. This is useful because the LegacyWithdrawal struct has helper
42: // functions on it that can compute the withdrawal hash and the storage slot.
43: func (s *SentMessage) ToLegacyWithdrawal() (*crossdomain.LegacyWithdrawal, error) {
44: 	data := make([]byte, len(s.Who)+len(s.Msg))
45: 	copy(data, s.Msg)
46: 	copy(data[len(s.Msg):], s.Who[:])
47: 
48: 	var w crossdomain.LegacyWithdrawal
49: 	if err := w.Decode(data); err != nil {
50: 		return nil, err
51: 	}
52: 	return &w, nil
53: }

Within the Decode function, Line 64 will check if the sender of the withdrawal is initiated by L2CrossDomainMessenger. If not, the function will return an error and the execution will halt.

Recall that in our example, one of the withdrawals is initiated by Alice. Thus, this will cause the Decode function to return an error, and the migration process will stop.

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

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: 	}

Impact

Malicious users or competitors could attempt to DOS the migration process causing reputation damage, delaying the migration, and causing griefing to the OP team.

Code Snippet

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

Tool used

Manual Review

Recommendation

Understood from the sponsor that only legacy withdrawals initiated by the L2CrossDomainMessenger will be migrated.

Consider updating the script to push only legacy withdrawals initiated by L2CrossDomainMessenger to the JSON file (evm-messages.json). This ensures that the JSON file only contains legacy withdrawals initiated by L2CrossDomainMessenger so that it is impossible for the decoding process to error out during the migration.

program
  .command('parse-state-dump')
  .description('parses state dump to json')
  .option('--file <file>', 'path to state dump file')
  .action(async (options) => {
    const iface = getContractInterface('OVM_L2ToL1MessagePasser')
    const dump = fs.readFileSync(options.file, 'utf-8')

    const addrs: string[] = []
    const msgs: any[] = []
    for (const line of dump.split('\n')) {
      if (line.startsWith('ETH')) {
        addrs.push(line.split('|')[1].replace('\r', ''))
      } else if (line.startsWith('MSG')) {
        const msg = '0x' + line.split('|')[2].replace('\r', '')
        const parsed = iface.decodeFunctionData('passMessageToL1', msg)
+		const who = line.split('|')[1]
+
+		if (who == 0x4200000000000000000000000000000000000007) {
            msgs.push({
+             who: who,
-			  who: line.split('|')[1],
              msg: parsed._message,
            })
+		}
      }
    }

    fs.writeFileSync(
      './data/evm-addresses.json',
      JSON.stringify(addrs, null, 2)
    )
    fs.writeFileSync('./data/evm-messages.json', JSON.stringify(msgs, null, 2))
  })

Duplicate of #105