[High] Function PreCheckWithdrawals() assumes that all the messages in the LegacyMessagePasserAddr are from L2CrossDomainMessanger and attacker can break migration script by calling OVM_L2ToL1MessagePasser.passMessageToL1() before migration

Summary

Function PreCheckWithdrawals() checks that the given list of withdrawals represents all withdrawals made in the legacy system and filters out any extra withdrawals not included in the legacy system and the code would return error when there is message in LegacyMessagePasserAddr that are not included in the withdrawals and withdrawals only includes the L2CrossDomainMessage messages so if attacker calls OVM_L2ToL1MessagePasser.passMessageToL1() before the migration then that message won't be in the withdrawal list and migration code would exit with error. I reported this as High because any bug in the withdrawal filtering and migration is crucial and the current code has wrong assumptions about the messages in the LegacyMessagePasserAddr and L2CrossDomainMessanger. fixing the code would be require to change a huge part of the script from migration data generation, withdrawal encoding and hashing, message encoding and hashing, filtering withdrawals,.... this bug can cause a lot of delay for the migration time if it's not fixed by that time.

Vulnerability Detail

This LegacyMessageParsser code:

contract OVM_L2ToL1MessagePasser is iOVM_L2ToL1MessagePasser {
    /**********************
     * Contract Variables *
     **********************/

    mapping(bytes32 => bool) public sentMessages;

    /********************
     * Public Functions *
     ********************/

    /**
     * @inheritdoc iOVM_L2ToL1MessagePasser
     */
    // slither-disable-next-line external-function
    function passMessageToL1(bytes memory _message) public {
        // Note: although this function is public, only messages sent from the
        // L2CrossDomainMessenger will be relayed by the L1CrossDomainMessenger.
        // This is enforced by a check in L1CrossDomainMessenger._verifyStorageProof().
        sentMessages[keccak256(abi.encodePacked(_message, msg.sender))] = true;
    }
}

As you can see any address can call passMessageToL1() and update the storage state of the contract in sendMessage[] variable and the storage can contain L2CrossDomainMessenger and other address messages. This is PreCheckWithdrawals() code:

// PreCheckWithdrawals checks that the given list of withdrawals represents all withdrawals made
// in the legacy system and filters out any extra withdrawals not included in the legacy system.
func PreCheckWithdrawals(db *state.StateDB, withdrawals []*LegacyWithdrawal) ([]*LegacyWithdrawal, error) {
    // Convert each withdrawal into a storage slot, and build a map of those slots.
    slotsInp := make(map[common.Hash]*LegacyWithdrawal)
    for _, wd := range withdrawals {
        slot, err := wd.StorageSlot()
        if err != nil {
            return nil, fmt.Errorf("cannot check withdrawals: %w", err)
        }

        slotsInp[slot] = wd
    }

    // 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
    })
    if err != nil {
        return nil, fmt.Errorf("cannot iterate over LegacyMessagePasser: %w", err)
    }

    // Log the number of messages we found.
    log.Info("Iterated legacy messages", "count", count)

    // 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)
        }
    }

    // Iterate over the list of input messages and check that we have a known slot for each one.
    // We'll filter out any extra messages that are not in the legacy system.
    filtered := make([]*LegacyWithdrawal, 0)
    for slot := range slotsInp {
        _, ok := slotsAct[slot]
        if !ok {
            log.Info("filtering out unknown input message", "slot", slot.String())
            continue
        }

        filtered = append(filtered, slotsInp[slot])
    }

    // At this point, we know that the list of filtered withdrawals MUST be exactly the same as the
    // list of withdrawals in the state. If we didn't have enough withdrawals, we would've errored
    // out, and if we had too many, we would've filtered them out.
    return filtered, nil
}

As you can see in the part "Iterate over the list of actual slots and check that we have an input message for each one." code checks that for each message in LegacyMessagePasserAddr's storage there is a withdrawal in the withdrawal list. but withdrawal list is only for L2CrossDomainMessanger legacy withdrawal messages and so if there were any other message in the LegacyMessagePasserAddr from another address then the checks won't be passed and code would return error and migration script won't run. to exploit this attacker needs to perform this steps;

call OVM_L2ToL1MessagePasser.passMessageToL1() before the migration.
then OVM_L2ToL1MessagePasser would set the attacker sender message slot as true. (sentMessages[keccak256(abi.encodePacked(_message, msg.sender))] = true)
when developer team runs the migration script function PreCheckWithdrawals() checks would find the attacker message in LegacyMessagePasserAddr storage slot but there won't be any corresponding L2CrossDomainMessanger withdrawal for that message and code would return with error.

Impact

this issue would cause migration to be blocked until the code changes again. there is a lot of code needs to be changed and those changes can cause new bugs and they require more security review. The migration time can be delayed some weeks.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/op-chain-ops/crossdomain/precheck.go#L13-L77

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/op-chain-ops/crossdomain/legacy_withdrawal.go#L36-L50

Tool used

Manual Review

Recommendation

get list of the all LegacyMessagePasserAddr messages and verify them by contract storage state and then filter L2CrossDomainMessanger withdrawal messages.

Duplicate of #105

sherlock-audit / 2023-01-optimism-judging

unforgiven - [High] Function PreCheckWithdrawals() assumes that all the messages in the LegacyMessagePasserAddr are from L2CrossDomainMessanger and attacker can break migration script by calling OVM_L2ToL1MessagePasser.passMessageToL1() before migration #218