c4-bot-2 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L1792-L1847 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L797-L822

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L797-L822

            function mintEther(to, amount, useNearCallPanic) {
                mstore(0, {{RIGHT_PADDED_MINT_ETHER_SELECTOR}})
                mstore(4, to)
                mstore(36, amount)
                let success := call(
                    gas(),
                    ETH_L2_TOKEN_ADDR(),
                    0,
                    0,
                    68,
                    0,
                    0
                )
                if iszero(success) {
                    switch useNearCallPanic
                    case 0 {
                        revertWithReason(
                            MINT_ETHER_FAILED_ERR_CODE(),
                            0
                        )
                    }
                    default {
                        nearCallPanic()
                    }
                }
            }

This function is used to mint ether to the recipient, as seen this function directly uses the useNearCallPanic to know if to revert with reason or panic call and burn all the gas attached to the parent frame

Now consider https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L1792-L1847

            function ZKSYNC_NEAR_CALL_executeL1Tx(
                abi,
                txDataOffset,
                basePubdataSpent,
                gasPerPubdata,
            ) -> success {
                // Skipping the first word of the ABI encoding of the struct
                let innerTxDataOffset := add(txDataOffset, 32)
                let from := getFrom(innerTxDataOffset)
                let gasPrice := getMaxFeePerGas(innerTxDataOffset)

                debugLog("Executing L1 tx", 0)
                debugLog("from", from)
                debugLog("gasPrice", gasPrice)

                // We assume that addresses of smart contracts on zkSync and Ethereum
                // never overlap, so no need to check whether `from` is an EOA here.
                debugLog("setting tx origin", from)

                setTxOrigin(from)
                debugLog("setting gas price", gasPrice)

                setGasPrice(gasPrice)

                debugLog("execution itself", 0)

                let value := getValue(innerTxDataOffset)
                if value {
1820:                    mintEther(from, value, true)
                }

                success := executeL1Tx(innerTxDataOffset, from)

                debugLog("Executing L1 ret", success)

                // If the success is zero, we will revert in order
                // to revert the minting of ether to the user
                if iszero(success) {
                    nearCallPanic()
                }

                if isNotEnoughGasForPubdata(
                    basePubdataSpent,
                    gas(),
                    // Note, that for L1->L2 transactions the reserved gas is used to protect the operator from
                    // transactions that might accidentally cause to publish too many pubdata.
                    // Thus, even if there is some accidental `reservedGas` left, it should not be used to publish pubdata.
                    0,
                    gasPerPubdata,
                ) {
                    // If not enough gas for pubdata was provided, we revert all the state diffs / messages
                    // that caused the pubdata to be published
                    nearCallPanic()
                }
            }

This function is responsible for the execution of the L1->L2 transaction, case with this is that it hardcodes the useNearCallPanic to true while querying mintEther(), which then causes all the gas sent by the user for this transaction to be burnt, since it calls nearCallPanic() whose implementation can be seen here https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L1857-L1862

However using this search command [https://github.com/search?q=repo%3Acode-423n4%2F2024-03-zksync%20mintEther(&type=code](https://github.com/search?q=repo%3Acode-423n4%2F2024-03-zksync%20mintEther(&type=code) we can see that all other two instances of querying mintEther() in the bootloader passes the useNearCallPanic bool as false saving the user their gas

Impact

Medium since this is loss of monetary value (albeit in gas), but currently, all of the gas attached to the transaction while calling ZKSYNC_NEAR_CALL_executeL1Tx() could get unfairly burnt if there is a need of minting ether.

Recommended Mitigation Steps

Since the other 2 instances have their useNearCallPanic as false leading them to revert with reason, the same fate should meet the instance attached in report in order to not save the whole gas attached to the transaction

Assessed type

Context

c4-sponsor commented 8 months ago

saxenism (sponsor) disputed

saxenism commented 8 months ago

Not an issue. The only reason why mintEther can fail, while keeping enough gas to execute nearCallPanic() itself is if 63/64 of the gas of the parent frame was passed and it wasnt enough. We must use nearCallPanic to protect ourselves from malicious users supplying too few gas.

alex-ppg commented 7 months ago

The Warden denotes that gas will be forcibly burnt if an ether mint fails at the L2 level which may be considered unfair. Per the Sponsor's assessment and the code's implementation, the scenario under which an ether mint will fail can only be considered as "malicious" in the sense that the caller did not supply sufficient gas.

In such a case, burning any remaining gas is an acceptable error handling mechanism as the gas itself should be minuscule and the ether mint's failure indicates abnormal circumstances (i.e. significantly small gas allocated).

c4-judge commented 7 months ago

alex-ppg marked the issue as unsatisfactory: Invalid

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