forceDeployOnAddress() in ContractDeployer.sol` can deploy contract to any address

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L214-L258

Vulnerability details

Impact

During the force deployment, there's no additional check for the address that is going to be deployed on. This basically means, that it's possible to deploy to any address and overwrite existing contracts.
It's also possible to deploy to EOA addresses, which can lead to fund loss of that EOA.

The severity of this issue has been evaluated as High. However, since it affects function with onlySelf modifier - the severity has been decreased to one category down - to Medium, since it assume a scenario with malicious governance.

According to Code4rena Severity Categorization

Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

• Assets can be stolen/lost/compromised directly - High, thus we can deploy on any address, including EOA, or existing contracts
• hypothetical attack path with stated assumptions, but external requirements - Medium, thus attack path requires malicious governance (onlySelf) modifier

Moreover, having a function which such huge power may raise red flag to Investors and may be consider as rug-pull vector. We can, basically, overwrite any existing address. According to Code4rena severity classification - most of rug-pull vectors are considered as Medium:

• TIMELOCK_ROLE Has Absolute Power to Withdraw All FUND May Raise Red Flags for Investors 2021-11-malt-findings#125

According to zkSync docs

ZK Rollup strictly inherits the security guarantees of the underlying L1

However, ability to force deploy on any address does not fulfill those guarantees, since it's possible to modify existing smart contract on the system. It's crucial to disclose those privileges to users so that they can make informed decisions about using the protocol.

Proof of Concept

There are two ways of deploying new contract.
The first one is: _nonSystemDeployOnAddress() (it's called from create2Account(), createAccount()):

File: ContractDeployer.sol

    function _nonSystemDeployOnAddress(
        bytes32 _bytecodeHash,
        address _newAddress,
        AccountAbstractionVersion _aaVersion,
        bytes calldata _input
    ) internal {
        require(_bytecodeHash != bytes32(0x0), "BytecodeHash cannot 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");

And the second way: force deploy:

File: ContractDeployer.sol

 function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {
 (...)

  function forceDeployOnAddresses(ForceDeployment[] calldata _deployments) external payable {
  (...)

The code responsible for force deployment, does not provide require checks which exist in _nonSystemDeployOnAddress.
There's missing checks:

• require(uint160(_newAddress) > MAX_SYSTEM_CONTRACT_ADDRESS, "Can not deploy contracts in kernel space");

• require(ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.getCodeHash(uint256(uint160(_newAddress))) == 0x0, "Code hash is non-zero"

• require(NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(_newAddress) == 0x00, "Account is occupied");

This implies, that it's possible to use forceDeployOnAddress() to deploy to any address, including existing ones.
During the force deployment, there's no additional check for the address that is going to be deployed on. This basically means, that it's possible to deploy to any address and overwrite existing contracts

Tools Used

Manual code review

Recommended Mitigation Steps

There are multiple of ways to solve this issue. The first idea would be to remove force-deploy functionality, since there's already a way to deploy: _nonSystemDeployOnAddress.
If, for any reason this function needs to be kept - make sure to limit force-deploy to only certain contracts. You can provide a whitelist of allowed addresses on which force-deployment can be used.
If, for any reason, whitelist is not an option - make sure to not allow force-deploy to system contracts and user accounts.

Assessed type

Access Control

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

Force deploy can only be used during system upgrades, controled by governance. Governance can do any changes (including changing the verification keys) - so ability to overwrite any code doesn’t introduce new attack verctors.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Downgrading to QA for operative risks