Closed c4-submissions closed 10 months ago
bytes032 marked the issue as low quality report
141345 marked the issue as primary issue
miladpiri marked the issue as disagree with severity
miladpiri (sponsor) confirmed
GalloDaSballo changed the severity to 2 (Med Risk)
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 changed the severity to QA (Quality Assurance)
Per the judge's request, marking as grade-C and closing.
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 alwaystrue
.The aim of
L1Messenger.sol
is to send messages to L1. In particular, it sends to L1 the Merkle tree root through its functionpublishPubdataAndClearState()
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 ofL2->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
Proof of Concept
Contract: L1Messenger.sol
Code lines: 204-206
It is obvious that
numberOfL2ToL1Logs <= numberOfL2ToL1Logs
is alwaystrue
: 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 ofL2->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.Assessed type
DoS