code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In Solidity, it is an error to divide before multiplying because of lots of rounding errors that can come from that. In this case:

    uint256 pubdataLen;
    unchecked {
        pubdataLen = ((_message.length + 31) / 32) * 32 + 64;
    }
    uint256 gasToPay = pubdataLen * gasPerPubdataBytes;

The calculation of pubdataLen will be wrong for certain numbers that round down: (_message.length + 31) / 32. Therefore the calculation of the gas to Pay will be wrong, possibly causing other undetected issue apart from the wrong calculation.

Proof of Concept

Let's take the example of sending a message to L1 of 5 bytes:

Current case:

        //5 + 31 = 36 
        //36 / 32 = 1  * 32 = 32 + 64 = 96

The result is 96 due to a rounding down error between 36/32

Optimal case: //5 + 31 = 36 //36 * 32 = 1152 / 32 = 36 + 64 = 100 The result of a right done arithmetic operation in this case would be 100

As you can see there is an arithmetic error due to division before multiplication

Tools Used

Manual

Recommended Mitigation Steps

You always have to multiply before divide in solidity:

pubdataLen = ((_message.length + 31) * 32) / 32 + 64;

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

c4-sponsor commented 1 year ago

vladbochok marked the issue as sponsor disputed

vladbochok commented 1 year ago

((_message.length + 31) * 32) / 32 is basically the rounding the number of bytes to 32. It is also stated in the comment above:

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

c4-sponsor commented 1 year ago

vladbochok requested judge review

miladpiri commented 1 year ago

It is rounded up to a multiple of 32. Using warden’s recomendation will result also in wrong answer. So, it is invalid.

GalloDaSballo commented 1 year ago

Thank you for the clarification!

Closing as the math is intended to round