Dirty bytes in dictionary during publishing the compressed bytecode

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L35

Vulnerability details

Impact

The provided dictionary during publishing the compressed bytecode can have extra useless bytes, that can lead to extra gas charging when being sent to L1.

Proof of Concept

During publishing the compressed bytecode, it is not checked that the _rawCompressedData is efficiently compact or not. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L35

For a better understanding, let's say the bytecode that is going to be compressed and published is ABCB (each character denotes 8 bytes). Please note that the second and fourth characters are similar to each other.

The correct way of compressing is that the _rawCompressedData be equal to: 0x0003ABC0000000100020001. In other words, the dictionary = ABC and encodedData = 0x0000000100020001. So, by using the encodedData, the chunks 0, 1, 2, 1 in the dictionary will be pointed respectively.

But, a malicious sequencer can charge the user more by adding dirty bits to the dictionary. In the example of above, the _rawCompressedData could be equal to: 0x0004ABCX0000000100020001. In other words, the dictionary = ABCX and encodedData = 0x0000000100020001. It means an extra 8-byte X is added to the dictionary, while it is not used at all. That is, the encodedData only refers to the chunks 0, 1, 2, 1, respectively, without referring to the chunk 3, i.e. X.

By doing so, the malicious sequencer can make the _rawCompressedData large, just to increase gasToPay when sending to L1. While, the encoded chunk matches to the original bytecode

Tools Used

Recommended Mitigation Steps

It is recommended to check that the number of 2-byte chunk of encodedData be larger than or equal to the number of 8-byte chunk of dictionary. By doing so, we will ensure that all the dictionary chunks are referred, and none of them are useless.

require((encodedData.length / 2) >= (dictionary.length / 8), "dictionary has dirty bits");

[miladpiri]

It is valid.

[c4-sponsor]

miladpiri marked the issue as sponsor confirmed

[c4-sponsor]

miladpiri requested judge review

[GalloDaSballo]

The Warden has shown how, due to the ability of the sequencer to add Dirty Bits to the _rawCompressedData, a user deploying contracts could be charge more than intended.

Because the finding pertains to incorrect validation at the contract level, which can result in higher cost, while I have considered Lower Severity, I believe Medium Severity to be appropriate as the code allowed for unintended behaviour

[c4-judge]

GalloDaSballo marked the issue as selected for report

[vladbochok]

@GalloDaSballo Sorry for the late response. I don't think this should be rewarded.

It was clearly stated in the documentation as an expected trusted assumption.

Awarding this issue creates injustice towards all wardens that noticed the issue but haven't written the report because of the "trusted assumption" section in the doc. Moreover, awarding it means that any documented known issues should be awarded too.

[GalloDaSballo]

After @vladbochok's comment, I have considered keeping the finding as valid due to the code not performing the check

However, I believe that the finding was disclosed in the Readme, for this reason am closing as OOS

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope