Flaky Plum Pig
Medium
In the Rollup contract, in the function commitBatch there is an assembly block which is allocating a memory pointer in an incorrect way.
See the first block where batchDataInput.chunks is type []bytes:
uint256 _chunksLength = batchDataInput.chunks.length;
...
uint256 dataPtr;
assembly {
dataPtr := mload(0x40)
mstore(0x40, add(dataPtr, mul(_chunksLength, 32)))
}See the second block, where BATCH_HEADER_FIXED_LENGTH = 249 is uint256 constant and batchDataInput.skippedL1MessageBitmap is type bytes.
uint256 _headerLength = BatchHeaderCodecV0.BATCH_HEADER_FIXED_LENGTH +
batchDataInput.skippedL1MessageBitmap.length;
assembly {
_batchPtr := mload(0x40)
mstore(0x40, add(_batchPtr, mul(_headerLength, 32)))
}The problem is, that the mul should no be used in the second case, instead a simple addition should be performed add(_batchPtr, _headerLength)).
Why? Because we use array of []bytes in the first case, it means that every single item in the array is a word aka 32 bytes. It means we have to multiply length with 32 to allocate enough memory.
However in the second case _headerLength is the value representing the total length of the batch header which is:
constant (number of bytes) + bitmap length (number of bytes) hence the _headerLength is already measured in bytes.
As a result we are allocating 32 times more memory.
What does is mean.. In a most basic case where skippedL1MessageBitmap.length == 0 we will assign a value 249*32=7968 to the free memory pointer (0x40), instead of 249.
In a case where skippedL1MessageBitmap.length > 0, for every single byte in skippedL1MessageBitmap we will allocate 32 times more. Example where skippedL1MessageBitmap.length == 500 we will assign a value (249+500)*32=23968 to the free memory pointer (0x40), instead of 749.
How big problem it is?
The function will most likely do the job, however the gas can become problematic and unimplemented functions may become problematic. With skippedL1MessageBitmap, which is unbounded because it is a dynamic type bytes, the function can become overly expensive. The function is already quite complex and computational heavy. There is also an extreme scenario - based on the docs and documents provided, the team is aware of potential ZK circuit overflow. To avoid overflow to happen, some transactions have to be skipped. That's where skippedL1MessageBitmap come in to play. What if:
- big amount of transaction that can cause ZK circuit overflow are submitted
- they have to be skipped, otherwise overflow happen and system will panic
- the bitmap size increases dramatically and big amount of memory is allocated
- the price of batch submitting become overly expensive or even cause out-of-gas error
Where does it leave the system? TXs have to be skipped, but batch committing is not possible.. It feels like DoS unless some manual action is performed. However this attack feels rather expensive without real profit to attacker profit.
Final note:
Several functions are not yet implemented. Some of them are even called from the vulnerable function (_getValidSequencerSet, _getBLSMsgHash). More code followed after wrong memory allocation, higher risk of unexpected behavior.
Assembly block in the Rollup contract:
Manual Review
Wake
Remove multiplication from the assembly block.
uint256 _headerLength = BatchHeaderCodecV0.BATCH_HEADER_FIXED_LENGTH +
batchDataInput.skippedL1MessageBitmap.length;
assembly {
_batchPtr := mload(0x40)
mstore(0x40, add(_batchPtr, _headerLength))
}