`getCanonicalL1TxHash()` may return the same hash for different transactions

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L692-L707

Vulnerability details

Impact

Function getCanonicalL1TxHash() returns a canonical hash of the L1->L2 transaction, that will be sent to L1 as a message to the L1 contract that a certain operation has been processed.

Since it accepts just a txDataOffset, two different transactions with the same offset will return the same hash.

Proof of Concept

File: bootloader.yul

/// @dev Calculates the canonical hash of the L1->L2 transaction that will be
            /// sent to L1 as a message to the L1 contract that a certain operation has been processed.
            function getCanonicalL1TxHash(txDataOffset) -> ret {
                // Putting the correct value at the `txDataOffset` just in case, since 
                // the correctness of this value is not part of the system invariants.
                // Note, that the correct ABI encoding of the Transaction structure starts with 0x20
                mstore(txDataOffset, 32)

                let innerTxDataOffset := add(txDataOffset, 32)
                let dataLength := safeAdd(32, getDataLength(innerTxDataOffset), "qev")

                debugLog("HASH_OFFSET", innerTxDataOffset)
                debugLog("DATA_LENGTH", dataLength)

                ret := keccak256(txDataOffset, dataLength)
            }

Let's consider two different L1->L2 transactions with the same offset. Since for both transactions offset is the same, the parameter txDataOffset will be the same for function getCanonicalL1TxHash(). This leads to the conclusion, that getCanonicalL1TxHash() will return the same hash, for two different transactions, when their offsets are the same.

Tools Used

Manual code review

Recommended Mitigation Steps

Refactor getCanonicalL1TxHash() - so that it will contain additional parameter - nonce. This nonce should also be used for hash calculation. Adding additional parameter nonce will make sure that getCanonicalL1TxHash() will return different hashes even for the transactions with the same txDataOffset.

Assessed type

Other

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

Considered invalid because offset is just a pointer to the memory, the memory contains actual data including the unique identifier.

[razzorsec]

txDataOffset is a memory pointer calculated from txPtr for every transaction in a batch. Here is a reference for the code section. Thus it will be unique for every transaction.

[c4-sponsor]

razzorsec marked the issue as disagree with severity

[c4-sponsor]

razzorsec marked the issue as agree with severity

[alex-ppg]

The Warden specifies that the generated canonical L1 transaction hashes may result in collisions due to the usage of a txDataOffset that may be the same between transactions.

As the Sponsor has correctly clarified, the txDataOffset is precisely what its name implies; an offset from which data is read to generate the hash. As this data will be different between transactions, whether the offset is the same or not has no bearing on the resulting transaction hash's uniqueness.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid