code-423n4 / 2023-03-zksync-findings

6 stars 1 forks source link

Gas check inaccuracy #176

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L1Messenger.sol#L43-L46 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L152

Vulnerability details

Impact

Since the gas forwarded will be limited to 63/64 of the total gasleft, L1 transactions will be vulnerable of being reverted. To achieve 1:1 partity with the EVM, the ZKEVM should account for 1/64 rule. Please refer to the 1/64 rule here. The actual amount of gas left during the call will be less than expected.

Proof of Concept

By running L1Messenger.sendToL1() in a sandbox environment (regular EVM), we can get gasleft ~14766 before the call.

If we make the following calculation:

gasleft = 14766
gasleft * 63/64 = 14535 (floored)

We can see that if the amount of gas to pay is smaller than ~14766 and bigger than ~14535, the require check will pass, but the transaction will fail.

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L1Messenger.sol#L43-L46

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L152

Recommended Mitigation Steps

There should be a margin between gasleft and the gas to pay. Consider multiplying _gasToBurn by 64/63 and also adding a buffer for safety.

constant uint256 BUFFER = 12_000;
require(gasleft() >= (_gasToBurn * 64) / 63 + BUFFER);
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

PrecompileCall is an opcode, that burn the exact min(gasleft(), providedGas). There is no room to the 63/64 rule.

Please note, the syntax that is used to precompileCall is a compiler simulation, not a real call. That was described in the documentation.

c4-sponsor commented 1 year ago

vladbochok requested judge review

GalloDaSballo commented 1 year ago

Technically speaking, because of where require(gasleft() >= _gasToBurn); then slightly more gas than _gasToBurn will have to be burned in the precompileCall function.

That in itself could be viewed as a valid QA finding.

However, in the context provided by the Warden, of sendToL1, the gas difference is not meaningful, and, per the Sponsor dispute, simulated opcode will burn w/e gas is actually available

For these reasons am closing for lack of proof

I recommend the warden to produce a POC and follow up with either me or the Sponsor

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof