zkSync Era - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

A very important modification of this update is that the GAS spent by pubdata is collected at the final step of the transaction.

But if there is a paymaster, when executing paymaster.postTransaction(_maxRefundedGas) _maxRefundedGas does not subtract the spentOnPubdata

bootloader.yul the code is as follow：

            function refundCurrentL2Transaction(
                txDataOffset,
                transactionIndex,
                success, 
                gasLeft,
                gasPrice,
                reservedGas,
                basePubdataSpent,
                gasPerPubdata
            ) -> 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) {
                        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"),
                            basePubdataSpent,
                            reservedGas,
                            gasPerPubdata
                        ))
                        let gasSpentByPostOp := sub(gasBeforePostOp, gas())

                        gasLeft := saturatingSub(gasLeft, gasSpentByPostOp)
                    } 
                }

                // It was expected that before this point various `isNotEnoughGasForPubdata` methods would ensure that the user 
                // has enough funds for pubdata. Now, we just subtract the leftovers from the user.
@>              let spentOnPubdata := getErgsSpentForPubdata(
                    basePubdataSpent,
                    gasPerPubdata
                )

                let totalRefund := saturatingSub(add(reservedGas, gasLeft), spentOnPubdata)

                askOperatorForRefund(
                    totalRefund,
                    spentOnPubdata,
                    gasPerPubdata
                )

                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, totalRefund)

                // 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
            }

paymaster's _maxRefundedGas = gasLeft + reservedGas, without subtracting spentOnPubdata.

This way _maxRefundedGas will be much larger than the correct value

paymaster will refund the used spentOnPubdata to the user

Impact

paymaster will refund the spentOnPubdata already used by the user

Recommended Mitigation

            function refundCurrentL2Transaction(
                txDataOffset,
                transactionIndex,
                success, 
                gasLeft,
                gasPrice,
                reservedGas,
                basePubdataSpent,
                gasPerPubdata
            ) -> 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
+                   let expectSpentOnPubdata := getErgsSpentForPubdata(
+                        basePubdataSpent,
+                        gasPerPubdata
+                    )                    
                    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"),
+                           saturatingSub(add(reservedGas, gasLeft), expectSpentOnPubdata),
                            basePubdataSpent,
                            reservedGas,
                            gasPerPubdata
                        ))
                        let gasSpentByPostOp := sub(gasBeforePostOp, gas())

                        gasLeft := saturatingSub(gasLeft, gasSpentByPostOp)
                    } 
                }

                // It was expected that before this point various `isNotEnoughGasForPubdata` methods would ensure that the user 
                // has enough funds for pubdata. Now, we just subtract the leftovers from the user.
                let spentOnPubdata := getErgsSpentForPubdata(
                    basePubdataSpent,
                    gasPerPubdata
                )

                let totalRefund := saturatingSub(add(reservedGas, gasLeft), spentOnPubdata)

                askOperatorForRefund(
                    totalRefund,
                    spentOnPubdata,
                    gasPerPubdata
                )

                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, totalRefund)

                // 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
            }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L166

Vulnerability details

Vulnerability details

StateTransitionManager.unfreezeChain() the code is as follows:

contract StateTransitionManager is IStateTransitionManager, ReentrancyGuard, Ownable2Step {
...
    function unfreezeChain(uint256 _chainId) external onlyOwner {
        IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
    }

This method is used for unfreeze, but it calls freezeDiamond(), which makes it impossible to unfreeze properly.

Impact

StateTransitionManager cannot unfreeze zkSync.

Although admin of zkSync can do unfreeze directly by zkSync.unfreezeDiamond()

However, admin is usually a multi-sign account, and to execute it would require a new wait for more time during which time zkSync would be fully freeze

Recommended Mitigation

    function unfreezeChain(uint256 _chainId) external onlyOwner {
-       IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
+       IZkSyncStateTransition(stateTransition[_chainId]).unfreezeDiamond();
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/zksync/contracts/bridge/L2SharedBridge.sol#L68

Vulnerability details

Vulnerability details

The migration steps for L1ERC20Bridge/L2ERC20Bridge are as follows:

https://github.com/code-423n4/2024-03-zksync/blob/main/docs/Protocol%20Section/Migration%20process.md

II. Upgrade L1ERC20Bridge contract

1. Upgrade L2 bridge

The new L2ERC20Bridge will upgraded to become the L2SharedBridge, and it will be backwards compatible with all messages from the old L1ERC20Bridge, so we upgrade that first as L1->L2 messages are much faster, and in the meantime we can upgrade the L1ERC20Bridge. The new L2SharedBridge can receive deposits from both the old L1ERC20Bridge and the new L1SharedBridge.

2. Upgrade L1ERC20Bridge

We upgrade the L1ERC20Bridge, and move all ERC20 tokens to the L1SharedBridge.

Since L2ERC20Bridge will be updated first, and then L1ERC20Bridge will be updated, L2SharedBridge needs to be compatible with the old L1ERC20Bridge before L1ERC20Bridge is updated.

So in L2SharedBridge.initialize() we need to set l1LegacyBridge = L1ERC20Bridge and finalizeDeposit() to allow l1LegacyBridge to execute.

But the current implementation doesn't set l1LegacyBridge, it's always address(0).

    function initialize(
        address _l1Bridge,
        address _l1LegecyBridge,
        bytes32 _l2TokenProxyBytecodeHash,
        address _aliasedOwner
    ) external reinitializer(2) {
        require(_l1Bridge != address(0), "bf");
        require(_l2TokenProxyBytecodeHash != bytes32(0), "df");
        require(_aliasedOwner != address(0), "sf");
        require(_l2TokenProxyBytecodeHash != bytes32(0), "df");

        l1Bridge = _l1Bridge;
        l2TokenProxyBytecodeHash = _l2TokenProxyBytecodeHash;

        if (block.chainid != ERA_CHAIN_ID) {
            address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}());
            l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken);
            l2TokenBeacon.transferOwnership(_aliasedOwner);
        } else {
            require(_l1LegecyBridge != address(0), "bf2");
@>          // Missing set l1LegecyBridge？
            // l2StandardToken and l2TokenBeacon are already deployed on ERA, and stored in the proxy
        }
    }

The above method just checks _l1LegecyBridge ! = address(0), and does not assign a value, l1LegacyBridge is always adddress(0).

This way any messages sent by the user before update L1ERC20Bridge will fail because finalizeDeposit() will not pass the validation

Impact

Until L1ERC20Bridge is updated, messages sent by the user will fail.

Recommended Mitigation

    function initialize(
        address _l1Bridge,
        address _l1LegecyBridge,
        bytes32 _l2TokenProxyBytecodeHash,
        address _aliasedOwner
    ) external reinitializer(2) {
...
        if (block.chainid != ERA_CHAIN_ID) {
            address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}());
            l2TokenBeacon = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken);
            l2TokenBeacon.transferOwnership(_aliasedOwner);
        } else {
            require(_l1LegecyBridge != address(0), "bf2");
+           l1LegacyBridge = _l1LegecyBridge
            // l2StandardToken and l2TokenBeacon are already deployed on ERA, and stored in the proxy
        }
    }

Assessed type

Context