`refundCurrentL2Transaction` does not check if there is enough gas to cover the computational overhead for the `ZKSYNC_NEAR_CALL_callPostOp`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1425-L1427

Vulnerability details

Impact

The gas provided to the ZKSYNC_NEAR_CALL_callPostOp may not be enough to cover the computational overhead, as it is not checked, so the operator may use his computational resources without being fully reimbursed for them.

Proof of concept

In bootloader, function checkEnoughGas, it is clearly stated that, before doing nearCalls, the current frame must have enough gas to pay for the computation overhead:

bootloader, lines 1781 to 1790

            /// @dev Checks whether the current frame has enough gas
            /// @dev It does not use 63/64 rule and should only be called before nearCalls. 
            function checkEnoughGas(gasToProvide) {
                debugLog("gas()", gas())
                debugLog("gasToProvide", gasToProvide)

                // Using margin of CHECK_ENOUGH_GAS_OVERHEAD gas to make sure that the operation will indeed
                // have enough gas 
                if lt(gas(), safeAdd(gasToProvide, CHECK_ENOUGH_GAS_OVERHEAD(), "cjq")) {
                    revertWithReason(NOT_ENOUGH_GAS_PROVIDED_ERR_CODE(), 0)
                }
            }

and if we go to all occurrences of nearCalls, we see this recurrent pattern:

    let gasBefore := gas()
    checkEnoughGas(gasLeft) // from the function arguments

    nearCall()
    let gasUsedInNearCall := sub(gasBefore, gas())

so that the operator does not incur in a cost the caller is not able to pay. However, the function refundCurrentL2Transaction does not call checkEnoughGas(gasLeft) before executing the ZKSYNC_NEAR_CALL_callPostOp (although it does check that gasLeft > 0, removing the possibility of sending all the gas from the parent frame, so the Medium instead of the High)

Recommended Mitigation steps

bootloader, lines 1425 to 1438

                    if gt(gasLeft, 0) {
+                       checkEnoughGas(gasLeft)
                        let nearCallAbi := getNearCallABI(gasLeft)
                        let gasBeforePostOp := gas()
                        pop(ZKSYNC_NEAR_CALL_callPostOp(
                            // Maximum number of gas that the postOp could spend
                            nearCallAbi,
                            paymaster,
                            txDataOffset,
                            success,
                            // Since the paymaster will be refunded with reservedGas,
                            // it should know about it
                            safeAdd(gasLeft, reservedGas, "jkl"),
                        ))
                        let gasSpentByPostOp := sub(gasBeforePostOp, gas())

                        switch gt(gasLeft, gasSpentByPostOp) 
                        case 1 { 
                            gasLeft := sub(gasLeft, gasSpentByPostOp)
                        }
                        default {
                            gasLeft := 0
                        }
                    }
                }

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[miladpiri]

I don’t see a clear path to exploiting it (as it has been already checked in the execution step of the tx), but we should add it. So, Low.

[miladpiri]

The duplicates indicated by the pre-sorter are irrelevant to this issue. They should be judged seperately.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Agree with lack of impact, downgrading

[nethoxa]

I'm ok with QA but I would like to highlight the idea I had back then for completeness. My idea was that checkEnoughGas checked for the current frame to have gas() >= gasToProvide + overhead. However, between that check before the execution step and the postOp one, there are many operations in between, so the "parent transaction" gas() is being reduced by those operations too (I think I did read somewhere in your docs that the gas constants in your config files were taken experimentally, so, if that's true, then this comment does not make sense and you do not have to keep reading). Anyway, it would be possible that

• nearCall_executeL2Tx is done with gas() ~= gasToProvide + overhead, consuming like $99$% of gasToProvide so that gasLeft is non-zero and we pass the check in line 1425
• After the execution step, gas() ~= overhead but the operations in between keep decrementing gas()
• postOp call is done with gas() << overhead
• Because of that, it reverts in his frame, consuming all gas
• Back to the bootloader frame, gas() ~= 0, so that the next instructions incurr in an OOG exception
• As we are in the parent frame, the whole transaction reverts

The reason behind is because the "gas hierarchy" looks like

so if the bootloader frame reverts, everything reverts. It could be used by anyone to censor batches by placing a flawed transaction at the end of the batch through sending it with enough gas to get picked the last one (high impact, low likelihood BUT high complexity leads to QA, indeed).

Please, note that the check wouldn't fix this, although, if I were in charge of writing the server code, I would put a hook to reorder the last transaction of the failed batch with a new one or even remove it and put that transaction in the next batch if this situation did occur.

[miladpiri]

We do anyway reorder the batch in case the last transaction gets picked up last (i.e. we remove the last transaction and put into the next batch). The last trasaction leads to the bootloader being OOG, so we basically already do what the warden suggests.

[nethoxa]

Perfect, then. Thanks!