Important require check inside L1Messenger is always true

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/ef99273a8fdb19f5912ca38ba46d6bd02071363d/code/system-contracts/contracts/L1Messenger.sol#L205-L206

Vulnerability details

Impact

There is no check of the size of logs, due to an error in the code.
An important require is always true.

The aim of L1Messenger.sol is to send messages to L1. In particular, it sends to L1 the Merkle tree root through its function publishPubdataAndClearState() function.

In order to do that, publishPubdataAndClearState() analyzes the whole pubdata from L1 Batch.
There is an attempt to mitigate DoS at the beginning of this function, that checks the number of L2->L1 logs, in order to block inputs with too many logs.
Each log has to be stored in memory, so it is important to prevent log bomb.

Furthermore, trees into L1 contracts and the server can not be too large.

We also report what is stated in System contracts bootloader description - L1Messenger Pubdata

[40053..248052] – slots where the final batch pubdata is supplied to be verified by the L1Messenger. More on how the L1Messenger system contracts handles the pubdata can be read here.

But briefly, this space is used for the calldata to the L1Messenger’s publishPubdataAndClearState function, which
accepts the list of the user L2→L1 logs, published L2→L1 messages as well as bytecodes. It also takes the list of
full state diff entries, which describe how each storage slot has changed as well as compressed state diffs. This
method will then check the correctness of the provided data and publish the hash of the correct pubdata to L1.

Proof of Concept

Contract: L1Messenger.sol

Code lines: 204-206

uint32 numberOfL2ToL1Logs = uint32(bytes4(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 4]));
require(numberOfL2ToL1Logs <= numberOfL2ToL1Logs, "Too many L2->L1 logs");

It is obvious that numberOfL2ToL1Logs <= numberOfL2ToL1Logs is always true: it is a comparison between the same valued.

So there is no check of Too many L2->L1 logs.

So it is possible to call publishPubdataAndClearState() using parameter _totalL2ToL1PubdataAndStateDiffs with any number of L2->L1 logs inside.
This behavior can be exploited to perform a DoS attack on the server, because everyone could ask to server to store in memory a huge amount of logs.

Tools Used

Visual inspection.

Recommended Mitigation Steps

It is clearly a code mistake. It should be a constant inside require, in order to assert that the number of logs is capped.

require(numberOfL2ToL1Logs <= MAX_NUMBER_OF_L2_TO_L1_LOGS, "Too many L2->L1 logs");

Assessed type

DoS

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[miladpiri]

QA.
Duplicate: #312

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

I think that the finding highlights a mistake (a vacous check)

I think that due to other checks, there's no scenario for exploit, but will double check

[GalloDaSballo]

#938 is a better example of how this could be used, however, I believe that checksum / keccak of said altered hash would have a different root meaning that it wouldn't pass the check

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[itsmetechjay]

Per the judge's request, marking as grade-C and closing.