TransactionValidator checks intrinsic costs against wrong value

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L41

Vulnerability details

Impact

An incorrect check allows an L1->L2 transaction to be sent that does not cover the sum of overhead and intrinsic costs for the operator.

Proof of Concept

The gasLimit required for a transaction consists of (see docs):

$$ \text{totalGasLimit} = \text{overhead + actualGasLimit} = \text{overhead + (intrinisicCosts + executionCosts)} $$

The function TransactionValidator.getMinimalPriorityTransactionGasLimit calculates the intrinsic costs that will be incurred for the processing of a transaction on L2. The calculated value is checked in TransactionValidator.validateL1ToL2Transaction like this:

require(
    getMinimalPriorityTransactionGasLimit(
        _encoded.length,
        _transaction.factoryDeps.length,
        _transaction.gasPerPubdataByteLimit
    ) <= _transaction.gasLimit, 
    "up"
);

The issue is that _transaction.gasLimit is the total gasLimit including the overhead for the operator. The overhead costs are already checked before that in TransactionValidator.getTransactionBodyGasLimit

function getTransactionBodyGasLimit(
    uint256 _totalGasLimit,
    uint256 _gasPricePerPubdata,
    uint256 _encodingLength
) internal pure returns (uint256 txBodyGasLimit) {
    uint256 overhead = getOverheadForTransaction(_totalGasLimit, _gasPricePerPubdata, _encodingLength);

    require(_totalGasLimit >= overhead, "my"); // provided gas limit doesn't cover transaction overhead
    unchecked {
        txBodyGasLimit = _totalGasLimit - overhead;
    }
}

The value returned by this function is the actualGasLimit (see formula above) that will be available for the processing of the transaction and which should be used to check if the intrinsic costs are covered.

In the current implementation the totalGasLimit is checked twice if its greater than overhead and intrinsic costs (separately, not the sum). The bootloader does things correctly by subtracting the overhead from the totalGasLimit first and then checking if the rest covers the intrinsic costs (also called "preparation" costs):

function processL1Tx(...){
    ...
    //gasLimitForTx is total - overhead (and some other intrinsic costs)
    let gasLimitForTx, reservedGas := getGasLimitForTx(...)
    ...
    canonicalL1TxHash, gasUsedOnPreparation := l1TxPreparation(txDataOffset)
    ...
}   if gt(gasLimitForTx, gasUsedOnPreparation) {
        ...
        potentialRefund, success := getExecuteL1TxAndGetRefund(txDataOffset, sub(gasLimitForTx, gasUsedOnPreparation))

That means users will not be able to execute transactions for cheap. However, since L1->L2 transaction have to be processed (due to the way the priority queue works), it would be possible to grief the operator by submitting transactions that only cover $\text{max(overhead, intrinsicCosts)}$. Those transactions would not have enough gas to be executed on L2, but the overhead and intrinsic costs would still be incurred.

Tools Used

Manual Review

Recommended Mitigation Steps

In TransactionValidator.validateL1ToL2Transaction change the check like this:

uint256 l2GasForTxBody = getTransactionBodyGasLimit(...)
...
require(
    getMinimalPriorityTransactionGasLimit(
        _encoded.length,
        _transaction.factoryDeps.length,
        _transaction.gasPerPubdataByteLimit
    ) <= l2GasForTxBody, // <---
    "up"
);

l2GasForTxBody is what remains after subtracting the overhead from the totalGasLimit.

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #975

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as selected for report