inconsistency in batches and dimondCut handling

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/DiamondInit.sol#L66-L75 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/DiamondProxy.sol#L11-L16 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Admin.sol#L98-L101

Vulnerability details

Impact

all the diamondCut() actions that are performed in zkSync diamond proxy can only be done by Governance contract and operation of Governance contract is check by security council except the first dimonCut that is get executed when the diamond proxy deployed. This is an inconsistency in handling the actions of system. the first diamondCut can have issues like others upgrade and it should be checked by securityCouncil and done through Governance contract. actually the initialization diamondCut is more critical and complicated than others and it requires more care.

all the batch commitments in zkSync diamond is done by validator and batches are verified by Verifier contract and it would make sure that L2 execution is done right but this is not the case for genesis batch. the genesis batch is only provided from contract deployer and no verification is done for it (no proof is provided and no verifying proof is performed). this would undermine the whole security and verification of the protocol.

of course right now the zkSync is running and this logic is not going to be executed but when zkSync stack is implemented and published (https://era.zksync.io/docs/reference/concepts/hyperscaling.html) each hyperchain should be permissionless and trusted in some way, but not verifying the genesis batch would undermine that and one perform malicious operation in it.

I believe this has at least Medium severity. The unchecked initialization transaction that sets up the DiamondProxy with DiamondCut and set the genesis Batch should be treated as other DiamondCuts and batches and because this first action is not check and verified it undermine the whole security of the system. the contract deployer can set any ETH value in the genesis block for their address or do malicious actions, because the VM execution is not verified, so every malicious action is possible in the first batch(caller can set any values for ETH balance of accounts). other than malicious action there could be a bug in generating the batchHash and batchCommitment which by checking the onchain they can be avoided. overall all the batch commitments are verified by on-chain L1 contracts except the genesis, and it contradict the whole security of zkSystem that is receiving its security form Ethereum (there is a backdoor in security system of the zkSystem).

sponsor may say that this deployment action is done by trusted actors, but the other actions is done by trusted actors too but there are some checks on-chain to verify their actions but the initialization action is not verified on-chain unlike other actions.

Proof of Concept

This is where all the initialization happens: in DiamondInit contract:

    function initialize(InitializeData calldata _initalizeData) external reentrancyGuardInitializer returns (bytes32) {
        require(address(_initalizeData.verifier) != address(0), "vt");
        require(_initalizeData.governor != address(0), "vy");
        require(_initalizeData.admin != address(0), "hc");
        require(_initalizeData.priorityTxMaxGasLimit <= L2_TX_MAX_GAS_LIMIT, "vu");

        s.verifier = _initalizeData.verifier;
        s.governor = _initalizeData.governor;
        s.admin = _initalizeData.admin;

        // We need to initialize the state hash because it is used in the commitment of the next batch
        IExecutor.StoredBatchInfo memory storedBatchZero = IExecutor.StoredBatchInfo(
            0,
            _initalizeData.genesisBatchHash,
            _initalizeData.genesisIndexRepeatedStorageChanges,
            0,
            EMPTY_STRING_KECCAK,
            DEFAULT_L2_LOGS_TREE_ROOT_HASH,
            0,
            _initalizeData.genesisBatchCommitment
        );

        s.storedBatchHashes[0] = keccak256(abi.encode(storedBatchZero));
        s.allowList = _initalizeData.allowList;
        s.verifierParams = _initalizeData.verifierParams;
        s.zkPorterIsAvailable = _initalizeData.zkPorterIsAvailable;
        s.l2BootloaderBytecodeHash = _initalizeData.l2BootloaderBytecodeHash;
        s.l2DefaultAccountBytecodeHash = _initalizeData.l2DefaultAccountBytecodeHash;
        s.priorityTxMaxGasLimit = _initalizeData.priorityTxMaxGasLimit;

        // While this does not provide a protection in the production, it is needed for local testing
        // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
        assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32);

        return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE;
    }

As you can see all these values has been set into storage without security council approval and without checking and verifying genesis batch information with the Verifier contract.

other DiamondCuts need to be called by Governance contract and they are done by executeUpgrade() function of Admin Facet.

other batches need to be submitted and proved and executed by validator and their proof will be checked by Verifier contract.

so there is a inconsistency in handling the first DiamondCut and first Batch commitment and because of this the whole security of the system is undermined because if the whole protocol was secure but still no one can be sure about the first DiamondCut and Genesis Batch.

Tools Used

VIM

Recommended Mitigation Steps

like other DiamondCut allow the first one to be called from Governance contract(changing flow of calls). like other batches verify the genesis batch data with Verifier contract.

note:

I don't have backstage access and I can't comment during Post-Judging QA duration. so if you as judge or sponsor have any question regarding the issue please contact me directly.

Assessed type

Governance

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

Invalid. This is deployment/initialization step, anyone can deploy the contract, so the issue doesn’t make any sense.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

Disrispectful submission

[0xunforgiven]

Hi @GalloDaSballo, @miladpiri, @vladbochok. Thanks for reviewing my comment.

I believe there is a misunderstanding about the issue. This issue directly implicates zkSync's integrity and security, which is of paramount importance to the safety of user funds and the overall trust in the platform.

zkSync claims to be a zk-rollup, inheriting its security from the Ethereum blockchain by subjecting each finalized batch on zkSync to verification by on-chain L1 contracts. These contracts validate the integrity of the batch's zk-proof, ensuring the accuracy and legitimacy of the transactions included.

However, this report uncovers a critical flaw that undermines this fundamental security mechanism. One specific batch, namely the genesis batch(first batch), doesn't get verified by on-chain L1 verifier. Genesis batch information is included during the contract setups, but the code fails to engage the on-chain Verifier to validate the execution proof of the genesis batch. This poses a significant threat to zkSync's security and integrity, because genesis batch's "state root hash" can contain many malicious states (for example 100M ETH balance for an address).

I have personally corroborated this issue with @vladbochok, confirming that the genesis batch's proof does not undergo verification by on-chain L1 contracts. While it has been argued that "zkSync is a deployed protocol and the genesis batch-related logics won't be executed" this argument does not address the core concern. Our focus lies in evaluating the code itself, not vulnerabilities in an active system. The genesis batch handling code remains part of the codebase and is in scope.

Furthermore, @vladbochok has mentioned the hyperscaling/zkStack plans, It's a "zk-L2 Builder" or "zk-L2 Factory" that empowers anyone to create permissionless L2 blockchains using the current code. This vulnerability poses a grave risk to these chain's users, as malicious actors could generate compromised chains that zkSync L1 contracts would inadvertently accept. To compare, anyone can create (ETH, USDC) pool in Uniswap, but they can't mint unlimited number of token for themself while creating the pool.

As you can see in below photo, @vladbochok's points regarding the genesis block handling (I asked about the genesis block's timestamp in the photo, but the same concepts applies to other genesis block properties that is mentioned in this report which is on-chain validation of the execution zk-proof of genesis block):

overall I believe in the context of this contest, and because of impacts on zkSync's future plans (they were not aware of these risks) this issue has Medium severity.

[GalloDaSballo]

I disagree with the severity and the assessment, while consequences can be explained in an endless loop, the finding is a lack of validation at initialization which is not indicative of a further vulnerability nor risk, I maintain that such a submission is disingenous and no amount of polite talks which seems to be generated by chat GPT should change that per our rules on admin mistake as well as the known scope

[vladbochok]

My 5 cents here, while initialization should be enforced on each zkStack stack trustlessly (by a contract) I still don't see any immediate impact for this or other zkSync chains at the moment. The issue is very hypothetical and depends on not yet written/audited code that is out of scope for the contest. Due to the lack of impact, I don't think it should have medium severity.

Thanks to the judge for handling this.