c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/ContractDeployer.sol#L211-L227 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/ContractDeployer.sol#L232-L247

Vulnerability details

Impact

This statement suggests a vulnerability where a centralized team has the capability to deploy code to any destination or Ethereum address without restrictions. In other words, the team possesses the authority to choose where the code is deployed, which may pose concerns related to decentralization and control over smart contracts.

From zkSync's docs, it is mentioned that:

ZK Rollup inherits its security assurances directly from the underlying L1.

One of the core security assurances of Ethereum (which serves as zkSync's L1) is that smart contracts are, by default, unchangeable, and the protocol team lacks any special ability to modify existing smart contracts within the system. The fact that the zkSync protocol team possesses this ability represents a substantial risk of centralization.

This also implies that a legal authority or government entity can legally compel the protocol team to take actions against its users, such as: Freezing or confiscating the assets of any user, Replacing or disabling protocols like "mixers," such as Tornado Cash.

This particular characteristic renders zkSync no longer fully "trustless" in the manner that Ethereum is. Users will need to have confidence that the zkSync team will not engage in any misappropriation of their cryptocurrency holdings.

Proof of Concept

Due to the significant power of force-deployment, the zkSync protocol team could either autonomously or under the coercion of a government entity potentially execute the following actions:

Deploy a Custom ERC-20 Token: They could forcibly deploy an ERC-20 token to the address that corresponds to the DAI ERC-20. This new ERC-20 might feature a customized transfer function designed to consistently route tokens to a predetermined recipient address.
Deploy a Smart Contract for Asset Seizure: Another action they could take is force-deploying a smart contract to the address of a substantial Ethereum holder (an "ETH whale"). This smart contract might include a withdraw function designed to transfer all ETH assets to a specified destination. In essence, this could serve as an asset seizure mechanism.
Replace the L2ETHToken Contract: They may force-deploy code that replaces the L2ETHToken contract with an updated version containing functionality for blacklisting specific addresses. This alteration could be motivated by a government's desire to seize the assets of users of Tornado Cash on zkSync. The updated L2ETHToken contract could be programmed to automatically transfer ETH from certain addresses to a designated location whenever those addresses attempt to transfer or withdraw ETH

 function forceDeployOnAddresses(ForceDeployment[] calldata _deployments) external payable {
        require(msg.sender == FORCE_DEPLOYER, "Can only be called by FORCE_DEPLOYER_CONTRACT");

        uint256 deploymentsLength = _deployments.length;
        // We need to ensure that the `value` provided by the call is enough to provide `value`
        // for all of the deployments
        uint256 sumOfValues = 0;
        for (uint256 i = 0; i < deploymentsLength; ++i) {
            sumOfValues += _deployments[i].value;
        }
        require(msg.value == sumOfValues, "`value` provided is not equal to the combined `value`s of deployments");

        for (uint256 i = 0; i < deploymentsLength; ++i) {
            this.forceDeployOnAddress{value: _deployments[i].value}(_deployments[i], msg.sender);
        }
    }

Then this:

  /// @param _sender The `msg.sender` inside the constructor call.
    function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {
        _ensureBytecodeIsKnown(_deployment.bytecodeHash);
        _storeConstructingByteCodeHashOnAddress(_deployment.newAddress, _deployment.bytecodeHash);

        AccountInfo memory newAccountInfo;
        newAccountInfo.supportedAAVersion = AccountAbstractionVersion.None;
        // Accounts have sequential nonces by default.
        newAccountInfo.nonceOrdering = AccountNonceOrdering.Sequential;
        _storeAccountInfo(_deployment.newAddress, newAccountInfo);

        if (_deployment.callConstructor) {
            _constructContract(_sender, _deployment.newAddress, _deployment.input, false);
        }

        emit ContractDeployed(_sender, _deployment.bytecodeHash, _deployment.newAddress);
    }

The zkSync protocol team possesses the authority to forcefully deploy code to any address within zkSync, and unlike regular deployments, there are no checks in place when conducting a force-deployment. These checks, which are present during standard deployments in ContractDeployer._nonSystemDeployOnAddress, include:

Ensuring that the _bytecodeHash is not set to zero.
Confirming that the address being deployed to is not within the range of system contract addresses.
Prohibiting deployment to an address that has already executed transactions, thus preventing the overwrite of code on such addresses.

        require(_bytecodeHash != bytes32(0x0), "BytecodeHash can not be zero");
        require(uint160(_newAddress) > MAX_SYSTEM_CONTRACT_ADDRESS, "Can not deploy contracts in kernel space");

        // We do not allow deploying twice on the same address.
        require(
            ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.getCodeHash(uint256(uint160(_newAddress))) == 0x0,
            "Code hash is non-zero"
        );
        // Do not allow deploying contracts to default accounts that have already executed transactions.
        require(NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(_newAddress) == 0x00, "Account is occupied");

As a result, force-deployment can lead to the following consequences: The ability to overwrite the existing code on any address, even including addresses designated as system contracts, The potential to deploy a smart contract to default accounts that have previously executed transactions. This opens the door to the takeover of user accounts, as any account can be affected by this action.

In essence, this means that the zkSync protocol team has a significant level of control over the zkSync network, including the ability to modify existing code on various addresses, including system contract addresses, and potentially taking over user accounts by deploying smart contracts to previously active accounts.

Tools Used

Manual/VsCode

Recommended Mitigation Steps

Proposed strategies to address the issue:

Remove Force-Deploy Capability: The most straightforward approach is to completely eliminate the force-deploy functionality. By doing so, the ability to deploy code to any address, especially user accounts and system contracts dealing with balances, would be revoked entirely.
Restrict Force-Deploy to Specific Contracts: Another option is to limit the force-deploy functionality to only specific contracts, excluding the capability to force-deploy to user accounts and system contracts that handle account balances. By specifying which contracts are eligible for force-deployment, the potential for abuse or unintended consequences could be mitigated.
Eliminate Impersonation Functionality: To enhance security, the feature allowing impersonation of msg.sender should be removed. This would help prevent unauthorized access to contract functionality and ensure that transactions and actions are executed in a more secure and transparent manner.

Suggestions aim to address the identified issue of unrestricted force-deployment in zkSync by either eliminating the functionality altogether, limiting its scope, or enhancing security measures.