Function `l2TransactionBaseCost` in `Mailbox.sol` Lacks Range Checks for Parameters and Validation in Process Logic, Leading to Unreasonable Cost Calculation Results

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/0ac7f015e6d091eba444fe6e490a85e3ee4faf79/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L143-L175

Vulnerability details

The function l2TransactionBaseCost in Mailbox.sol is responsible for calculating the L2 Transaction Base Cost. The parameters _gasPrice, _l2GasLimit, and _l2GasPerPubdataByteLimit lack any value range checks, leading to occasionally large calculation results.

Moreover, there are issues with the test code for this function, and the correctness of its internal logic needs verification.

Impact

• Incorrect calculation of L2 Transaction Base Cost, affecting all related L2 fees.
• The possibility of extreme values for the L2 Transaction Base Cost, impacting the entire zkSync era.

Proof of Concept

1. The function l2TransactionBaseCost and the function _deriveL2GasPrice it calls do not perform any validation on external input parameters. https://github.com/code-423n4/2024-03-zksync/blob/0ac7f015e6d091eba444fe6e490a85e3ee4faf79/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L143-L175

    function l2TransactionBaseCost(
        uint256 _gasPrice,
        uint256 _l2GasLimit,
        uint256 _l2GasPerPubdataByteLimit
    ) public view returns (uint256) {
        uint256 l2GasPrice = _deriveL2GasPrice(_gasPrice, _l2GasPerPubdataByteLimit);
        return l2GasPrice * _l2GasLimit;
    }

    /// @notice Derives the price for L2 gas in ETH to be paid.
    /// @param _l1GasPrice The gas price on L1
    /// @param _gasPerPubdata The price for each pubdata byte in L2 gas
    /// @return The price of L2 gas in ETH
    function _deriveL2GasPrice(uint256 _l1GasPrice, uint256 _gasPerPubdata) internal view returns (uint256) {
        FeeParams memory feeParams = s.feeParams;
        require(s.baseTokenGasPriceMultiplierDenominator > 0, "Mailbox: baseTokenGasPriceDenominator not set");
        uint256 l1GasPriceConverted = (_l1GasPrice * s.baseTokenGasPriceMultiplierNominator) /
            s.baseTokenGasPriceMultiplierDenominator;
        uint256 pubdataPriceBaseToken;
        if (feeParams.pubdataPricingMode == PubdataPricingMode.Rollup) {
            pubdataPriceBaseToken = L1_GAS_PER_PUBDATA_BYTE * l1GasPriceConverted;
        }

        uint256 batchOverheadBaseToken = uint256(feeParams.batchOverheadL1Gas) * l1GasPriceConverted;
        uint256 fullPubdataPriceBaseToken = pubdataPriceBaseToken +
            batchOverheadBaseToken /
            uint256(feeParams.maxPubdataPerBatch);

        uint256 l2GasPrice = feeParams.minimalL2GasPrice + batchOverheadBaseToken / uint256(feeParams.maxL2GasPerBatch);
        uint256 minL2GasPriceBaseToken = (fullPubdataPriceBaseToken + _gasPerPubdata - 1) / _gasPerPubdata;

        return Math.max(l2GasPrice, minL2GasPriceBaseToken);
    }

2. The function test_l2TransactionBaseCost in the test code experimental_bridge.t.sol is used to indirectly test the same function in Mailbox.sol by testing bridgeHub.l2TransactionBaseCost. However, there are issues with this test code:

   1. Regardless of the input parameters, the test always passes.

         vm.mockCall(
          address(mockChainContract),
          abi.encodeWithSelector(
              mockChainContract.l2TransactionBaseCost.selector,
              mockGasPrice,
              mockL2GasLimit,
              mockL2GasPerPubdataByteLimit
          ),
          abi.encode(mockL2TxnCost)
      );

      This is because vm.mockCall has fixed the return value of l2TransactionBaseCost to be mockL2TxnCost, making the test tautological.

      assertTrue(
          bridgeHub.l2TransactionBaseCost(mockChainId, mockGasPrice, mockL2GasLimit, mockL2GasPerPubdataByteLimit) ==
              mockL2TxnCost
      );

   2. Using Fuzz Testing cannot verify the logic and correctness of the l2TransactionBaseCost function. The calculation of L2TxnCost depends on mockGasPrice, mockL2GasLimit, and mockL2GasPerPubdataByteLimit, so these four parameters, especially mockL2TxnCost, cannot be arbitrarily input. Manually inputting parameters and only changing the value of mockL2TxnCost still passes the test.

      
      Ran 1 test for test/foundry/unit/concrete/Bridgehub/l2transaction_base_cost.t.sol:ExperimentalBridgeTest
      [PASS] test_l2TransactionBaseCost() (gas: 203316)
      Logs:
      mockChainId 0
      mockGasPrice 10
      mockL2GasLimit 2000
      mockL2GasPerPubdataByteLimit 15
      mockL2TxnCost 35433243423423
      Bound Result 2
      calculatedL2TxnCost 35433243423423

   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.59ms

   Ran 1 test suite in 2.59ms: 1 tests passed, 0 failed, 0 skipped (1 total tests) (base) ➜ ethereum git:(main) ✗ forge test --match-path test/foundry/unit/concrete/Bridgehub/l2transaction_base_cost.t.sol -vv

   [⠔] Compiling... [⠘] Compiling 1 files with 0.8.20 [⠃] Solc 0.8.20 finished in 8.52s Compiler run successful!

   Ran 1 test for test/foundry/unit/concrete/Bridgehub/l2transaction_base_cost.t.sol:ExperimentalBridgeTest [PASS] test_l2TransactionBaseCost() (gas: 203316) Logs: mockChainId 0 mockGasPrice 10 mockL2GasLimit 2000 mockL2GasPerPubdataByteLimit 15 mockL2TxnCost 55 Bound Result 2 calculatedL2TxnCost 55

   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.45ms

   Ran 1 test suite in 2.45ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)

   
   ## Recommended Mitigation Steps
   - Review the logic of the `l2TransactionBaseCost` function.
   - Reconstruct the test data and test code for the `l2TransactionBaseCost` function.

Assessed type

Invalid Validation

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

We don't see the issue here, the warden can provide more concrete and statistical impact explanation if they want.

[alex-ppg]

The Warden attempts to articulate a flaw in the Mailbox::l2TransactionBaseCost function, however, no clear flaw is identified and I do not envision any apparent issue arising from the Warden's submission. As such, I consider it to have insufficient proof.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Insufficient proof