Reentrancy call via `ZKSYNC_NEAR_CALL_callPostOp` if transaction use Paymaster

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1425-L1447 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L2160-L2170

Vulnerability details

Impact

When transaction provide paymaster to provide gas payment, paymaster's ZKSYNC_NEAR_CALL_callPostOp can be used to do other operation/external call that modify storage and use more gas than it should be.

Proof of Concept

If L2 transaction is triggered and paymaster parameter provided, it will first make sure paymaster paid for gas via ensurePayment inside transaction validation.

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1286-L1305

            function ZKSYNC_NEAR_CALL_validateTx(
                abi,
                txDataOffset,
                gasPrice
            ) -> ret {
                // For the validation step we always use the bootloader as the tx.origin of the transaction
                setTxOrigin(BOOTLOADER_FORMAL_ADDR())
                setGasPrice(gasPrice)

                // Skipping the first 0x20 word of the ABI-encoding
                let innerTxDataOffset := add(txDataOffset, 32)
                debugLog("Starting validation", 0)

                accountValidateTx(txDataOffset)
                debugLog("Tx validation complete", 1)

>>>                ensurePayment(txDataOffset, gasPrice)

                ret := 1
            }

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L656-L732

            function ensurePayment(txDataOffset, gasPrice) {
                // Skipping the first 0x20 byte in the encoding of the transaction.
                let innerTxDataOffset := add(txDataOffset, 32)
                let from := getFrom(innerTxDataOffset)
                let requiredETH := safeMul(getGasLimit(innerTxDataOffset), gasPrice, "lal")

                let bootloaderBalanceETH := balance(BOOTLOADER_FORMAL_ADDR())
                let paymaster := getPaymaster(innerTxDataOffset)

                let payer := 0

                switch paymaster
                case 0 {
                    payer := from

                    // There is no paymaster, the user should pay for the execution.
                    // Calling for the `payForTransaction` method of the account.
                    setHook(VM_HOOK_ACCOUNT_VALIDATION_ENTERED())
                    let res := accountPayForTx(from, txDataOffset)
                    setHook(VM_HOOK_NO_VALIDATION_ENTERED())

                    if iszero(res) {
                        revertWithReason(
                            PAY_FOR_TX_FAILED_ERR_CODE(),
                            1
                        )
                    }
                }   
                default {
                    // There is some paymaster present.
                    payer := paymaster 

                    // Firstly, the `prepareForPaymaster` method of the user's account is called.
                    setHook(VM_HOOK_ACCOUNT_VALIDATION_ENTERED())
                    let userPrePaymasterResult := accountPrePaymaster(from, txDataOffset)
                    setHook(VM_HOOK_NO_VALIDATION_ENTERED())

                    if iszero(userPrePaymasterResult) {
                        revertWithReason(
                            PRE_PAYMASTER_PREPARATION_FAILED_ERR_CODE(),
                            1
                        )
                    }

                    // Then, the paymaster is called. The paymaster should pay us in this method.
                    setHook(VM_HOOK_PAYMASTER_VALIDATION_ENTERED())
>>>                    let paymasterPaymentSuccess := validateAndPayForPaymasterTransaction(paymaster, txDataOffset)
                    if iszero(paymasterPaymentSuccess) {
                        revertWithReason(
                            PAYMASTER_VALIDATION_FAILED_ERR_CODE(),
                            1
                        )
                    }

                    storePaymasterContextAndCheckMagic()
                    setHook(VM_HOOK_NO_VALIDATION_ENTERED())
                }

                let bootloaderReceivedFunds := safeSub(balance(BOOTLOADER_FORMAL_ADDR()), bootloaderBalanceETH, "qsx")

                // If the amount of funds provided to the bootloader is less than the minimum required one
                // then this transaction should be rejected.                
                if lt(bootloaderReceivedFunds, requiredETH)  {
                    revertWithReason(
                        FAILED_TO_CHARGE_FEE_ERR_CODE(),
                        0
                    )
                }

                let excessiveFunds := safeSub(bootloaderReceivedFunds, requiredETH, "llm") 

                if gt(excessiveFunds, 0) {
                    // Returning back the excessive funds taken.
                    directETHTransfer(excessiveFunds, payer)
                }
            }

Then after all transaction execution is done, it will trigger refundCurrentL2Transaction and trigger ZKSYNC_NEAR_CALL_callPostOp if gasLeft left is not 0 and paymaster provided.

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1401-L1476

            function refundCurrentL2Transaction(
                txDataOffset,
                transactionIndex,
                success, 
                gasLeft,
                gasPrice,
                reservedGas
            ) -> finalRefund {
                setTxOrigin(BOOTLOADER_FORMAL_ADDR())

                finalRefund := 0

                let innerTxDataOffset := add(txDataOffset, 32)

                let paymaster := getPaymaster(innerTxDataOffset)
                let refundRecipient := 0
                switch paymaster
                case 0 {
                    // No paymaster means that the sender should receive the refund
                    refundRecipient := getFrom(innerTxDataOffset)
                }
                default {
                    refundRecipient := paymaster

                    if gt(gasLeft, 0) {
                        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
                        }
                    } 
                }

                askOperatorForRefund(gasLeft)

                let operatorProvidedRefund := getOperatorRefundForTx(transactionIndex)

                // If the operator provides the value that is lower than the one suggested for 
                // the bootloader, we will use the one calculated by the bootloader.
                let refundInGas := max(operatorProvidedRefund, add(reservedGas, gasLeft))

                // The operator cannot refund more than the gasLimit for the transaction
                if gt(refundInGas, getGasLimit(innerTxDataOffset)) {
                    assertionError("refundInGas > gasLimit")
                }

                if iszero(validateUint32(refundInGas)) {
                    assertionError("refundInGas is not uint32")
                }

                let ethToRefund := safeMul(
                    refundInGas, 
                    gasPrice, 
                    "fdf"
                ) 

                directETHTransfer(ethToRefund, refundRecipient)

                finalRefund := refundInGas
            }

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L2087-L2171

            function ZKSYNC_NEAR_CALL_callPostOp(abi, paymaster, txDataOffset, txResult, maxRefundedGas) -> success {
                // The postOp method has the following signature:
                // function postTransaction(
                //     bytes calldata _context,
                //     Transaction calldata _transaction,
                //     bytes32 _txHash,
                //     bytes32 _suggestedSignedHash,
                //     ExecutionResult _txResult,
                //     uint256 _maxRefundedGas
                // ) external payable;
                // The encoding is the following:
                // 1. Offset to the _context's content. (32 bytes)
                // 2. Offset to the _transaction's content. (32 bytes)
                // 3. _txHash (32 bytes)
                // 4. _suggestedSignedHash (32 bytes)
                // 5. _txResult (32 bytes)
                // 6. _maxRefundedGas (32 bytes)
                // 7. _context (note, that the content must be padded to 32 bytes)
                // 8. _transaction

                let contextLen := mload(PAYMASTER_CONTEXT_BEGIN_BYTE())
                let paddedContextLen := lengthRoundedByWords(contextLen)
                // The length of selector + the first 7 fields (with context len) + context itself.
                let preTxLen := add(228, paddedContextLen)

                let innerTxDataOffset := add(txDataOffset, 32)
                let calldataPtr := sub(innerTxDataOffset, preTxLen)

                {
                    let ptr := calldataPtr

                    // Selector
                    mstore(ptr, {{RIGHT_PADDED_POST_TRANSACTION_SELECTOR}})
                    ptr := add(ptr, 4)

                    // context ptr
                    mstore(ptr, 192) // The context always starts at 32 * 6 position
                    ptr := add(ptr, 32)

                    // transaction ptr
                    mstore(ptr, sub(innerTxDataOffset, add(calldataPtr, 4)))
                    ptr := add(ptr, 32)

                    // tx hash
                    mstore(ptr, mload(CURRENT_L2_TX_HASHES_BEGIN_BYTE()))
                    ptr := add(ptr, 32)

                    // suggested signed hash
                    mstore(ptr, mload(add(CURRENT_L2_TX_HASHES_BEGIN_BYTE(), 32)))
                    ptr := add(ptr, 32)

                    // tx result
                    mstore(ptr, txResult)
                    ptr := add(ptr, 32)

                    // maximal refunded gas
                    mstore(ptr, maxRefundedGas)
                    ptr := add(ptr, 32)

                    // storing context itself
                    memCopy(PAYMASTER_CONTEXT_BEGIN_BYTE(), ptr, add(32, paddedContextLen))
                    ptr := add(ptr, add(32, paddedContextLen))

                    // At this point, the ptr should reach the innerTxDataOffset. 
                    // If not, we have done something wrong here.
                    if iszero(eq(ptr, innerTxDataOffset)) {
                        assertionError("postOp: ptr != innerTxDataOffset")
                    }

                    // no need to store the transaction as from the innerTxDataOffset starts
                    // valid encoding of the transaction
                }

                let calldataLen := safeAdd(preTxLen, getDataLength(innerTxDataOffset), "jiq")

>>>                success := call(
                    gas(),
                    paymaster,
                    0,
                    calldataPtr,
                    calldataLen,
                    0,
                    0
                )
            }

The problem is this ZKSYNC_NEAR_CALL_callPostOp is not protected with hook that prevent operation/external call that modify storage, this will allow postTransaction to do operation and use gas more than gasLeft and basically do more operation that what is paid.

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1438-L1447

...

                    if gt(gasLeft, 0) {
                        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
                        }
                    } 
...

as we can see, it will only set gasLeft to 0, even though gasSpentByPostOp can be much bigger than gasLeft.

Foundry PoC :

In the test scenario, we want to simulate postTransaction to do external call and modify the storage value on the target contract.

Modify the TestnetPaymaster contract inside code/contracts/zksync/contracts/TestnetPaymaster.sol to this :

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import "./interfaces/IPaymaster.sol";
import "./interfaces/IPaymasterFlow.sol";
import "./L2ContractHelper.sol";
import "hardhat/console.sol";
import "./MockChange.sol";

// This is a dummy paymaster. It expects the paymasterInput to contain its "signature" as well as the needed exchange rate.
// It supports only approval-based paymaster flow.
contract TestnetPaymaster is IPaymaster {

    MockChange public mockContract;

    function validateAndPayForPaymasterTransaction(
        bytes32,
        bytes32,
        Transaction calldata _transaction
    ) external payable returns (bytes4 magic, bytes memory context) {
        console.log("Goes here 1");
        // By default we consider the transaction as accepted.
        magic = PAYMASTER_VALIDATION_SUCCESS_MAGIC;

        require(
            msg.sender == BOOTLOADER_ADDRESS,
            "Only bootloader can call this contract"
        );
        require(
            _transaction.paymasterInput.length >= 4,
            "The standard paymaster input must be at least 4 bytes long"
        );

        bytes4 paymasterInputSelector = bytes4(
            _transaction.paymasterInput[0:4]
        );
        if (paymasterInputSelector == IPaymasterFlow.approvalBased.selector) {
            // While the actual data consists of address, uint256 and bytes data,
            // the data is not needed for the testnet paymaster
            (address token, uint256 amount, ) = abi.decode(
                _transaction.paymasterInput[4:],
                (address, uint256, bytes)
            );

            // Firstly, we verify that the user has provided enough allowance
            address userAddress = address(uint160(_transaction.from));
            address thisAddress = address(this);

            uint256 providedAllowance = IERC20(token).allowance(
                userAddress,
                thisAddress
            );
            require(
                providedAllowance >= amount,
                "The user did not provide enough allowance"
            );

            // The testnet paymaster exchanges X wei of the token to the X wei of ETH.
            uint256 requiredETH = _transaction.gasLimit *
                _transaction.maxFeePerGas;
            if (amount < requiredETH) {
                // Important note: while this clause definitely means that the user
                // has underpaid the paymaster and the transaction should not accepted,
                // we do not want the transaction to revert, because for fee estimation
                // we allow users to provide smaller amount of funds then necessary to preserve
                // the property that if using X gas the transaction success, then it will succeed with X+1 gas.
                magic = bytes4(0);
            }

            // Pulling all the tokens from the user
            try
                IERC20(token).transferFrom(userAddress, thisAddress, amount)
            {} catch (bytes memory revertReason) {
                // If the revert reason is empty or represented by just a function selector,
                // we replace the error with a more user-friendly message
                if (revertReason.length <= 4) {
                    revert("Failed to transferFrom from users' account");
                } else {
                    assembly {
                        revert(add(0x20, revertReason), mload(revertReason))
                    }
                }
            }

            // The bootloader never returns any data, so it can safely be ignored here.
            (bool success, ) = payable(BOOTLOADER_ADDRESS).call{
                value: requiredETH
            }("");
            require(success, "Failed to transfer funds to the bootloader");
        } else {
            revert("Unsupported paymaster flow");
        }
    }

    function setMockContract(address _addr) public {
        mockContract = MockChange(payable(_addr));
    }

    function postTransaction(
        bytes calldata _context,
        Transaction calldata _transaction,
        bytes32,
        bytes32,
        ExecutionResult _txResult,
        uint256 _maxRefundedGas
    ) external payable override {
        mockContract.setStateChange();
    }

    receive() external payable {}
}

Add MockChange contract to code/contracts/zksync/contracts/MockChange.sol :

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

contract MockChange {

    bool public stateChange;

    function setStateChange() public {
        stateChange = true;
    }

    receive() external payable {}
}

Add this test file to code/contracts/zksync/test/reentrancy.test.ts :

import { expect } from "chai";
import { Wallet, Provider } from "zksync-web3";
import * as hre from "hardhat";
import { ethers } from "ethers";
import { L2WethFactory } from "../typechain/L2WethFactory";
import { L2Weth} from "../typechain/L2Weth";
import { L2WethBridgeFactory } from "../typechain/L2WethBridgeFactory";
import { L2WethBridge } from "../typechain/L2WethBridge";
import { TestnetPaymaster } from "../typechain/TestnetPaymaster";
import { TestnetPaymasterFactory } from "../typechain/TestnetPaymasterFactory";
import { Deployer } from "@matterlabs/hardhat-zksync-deploy";
import { MockChange} from "../typechain/MockChange";
import { MockChangeFactory} from "../typechain/MockChangeFactory";

const richAccount =   {
    address: "0x36615Cf349d7F6344891B1e7CA7C72883F5dc049",
    privateKey: "0x7726827caac94a7f9e1b160f7ea819f172f7b6f9d2a97f992c38edeab82d4110",
};

const eth18 = ethers.utils.parseEther("18");

describe("Reentrancy paymaster", function () {
  let provider = new Provider(hre.config.networks.localhost.url);
  let wallet = new Wallet(richAccount.privateKey, provider);
  let wethToken: L2Weth;
  let wethBridge: L2WethBridge;
  let testnetPaymaster: TestnetPaymaster;
  let paymasterFlowInterface: ethers.utils.Interface;
  let mockChange: MockChange;
//   let mockERC20Approve: MockERC20Approve;

  before("Deploy token and paymaster", async function () {
      const deployer = new Deployer(hre, wallet);
      const wethTokenImpl = await deployer.deploy(await deployer.loadArtifact("L2Weth"));
      const wethBridgeImpl = await deployer.deploy(await deployer.loadArtifact("L2WethBridge"));
      const randomAddress = ethers.utils.hexlify(ethers.utils.randomBytes(20));

      const wethTokenProxy = await deployer.deploy(
          await deployer.loadArtifact("TransparentUpgradeableProxy"), 
          [wethTokenImpl.address, randomAddress, "0x"]
      );
      const wethBridgeProxy = await deployer.deploy(
          await deployer.loadArtifact("TransparentUpgradeableProxy"), 
          [wethBridgeImpl.address, randomAddress, "0x"]
      ) as any;

      const testnetPaymasterImpl = await deployer.deploy(await deployer.loadArtifact("TestnetPaymaster"));
      const mockChangeImpl = await deployer.deploy(await deployer.loadArtifact("MockChange"));  

      wethToken = L2WethFactory.connect(wethTokenProxy.address, wallet);
      wethBridge = L2WethBridgeFactory.connect(wethBridgeProxy.address, wallet);
      testnetPaymaster = TestnetPaymasterFactory.connect(testnetPaymasterImpl.address, wallet);
      mockChange = MockChangeFactory.connect(mockChangeImpl.address, wallet);
    //   mockERC20Approve = await deployer.deploy(await deployer.loadArtifact('MockERC20Approve')) as MockERC20Approve;

      let paymasterFlowInterfaceArtifact = await deployer.loadArtifact('IPaymasterFlow');
      paymasterFlowInterface = new ethers.utils.Interface(paymasterFlowInterfaceArtifact.abi);

    //   mockERC20Approve = MockERC20Approve__factory.connect(mockERC20ApproveImpl.address, wallet);

      await wethToken.initialize("Wrapped Ether", "WETH");
      await wethToken.initializeV2(wethBridge.address, randomAddress);

      await wethBridge.initialize(randomAddress, randomAddress, wethToken.address);
  });

  it("Test call using paymaster", async function () {
    await wethToken.deposit({ value: eth18 }).then(tx => tx.wait());
    await wethToken.approve(testnetPaymaster.address, eth18);
    await wallet.sendTransaction({
        to: testnetPaymaster.address,
        value: eth18
    }).then(tx => tx.wait());
    await testnetPaymaster.setMockContract(mockChange.address);
    const txResp = await wallet.sendTransaction({
        type: 113,
        to: wallet.address,
        data: '0x',
        gasLimit: 2_000_000,
        customData: {
            gasPerPubdata: 50000, // zksync.utils.DEFAULT_GAS_PER_PUBDATA_LIMIT
            paymasterParams: {
                paymaster: testnetPaymaster.address,
                paymasterInput: paymasterFlowInterface.encodeFunctionData('approvalBased', [
                    wethToken.address,
                    ethers.utils.parseUnits('1', 17),
                    '0x'
                ])
            }
        }
    });
    let txReceipt = await txResp.wait();
    console.log("Is external state changed? (bool)");
    console.log(await mockChange.stateChange());
});
});

Run the test :

cd code/contracts/zksync
era_test_node run > /dev/null 2>&1 & export TEST_NODE_PID=$!
npx hardhat test ./test/reentrancy.test.ts

Output :

  Reentrancy paymaster
Is external state changed? (bool)
true
    ✔ Test call using paymaster (613ms)

  1 passing (2s)

NOTE : The coded PoC verify that postTransaction allowed to do external call operation and modify storage. This can be leveraged to do expensive operation and use gas more than it should.

Tools Used

Manual review

Recommended Mitigation Steps

Restrict ZKSYNC_NEAR_CALL_callPostOp so it can't write to external storage and make sure not used gas more than what already paid.

Assessed type

Reentrancy

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[miladpiri]

It is invalid, because the paymaster can consume at most gasLeft because the nearCall forwards this amount to that frame. So, consuming this gas, will reduce the refund amount as well.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid