Governor can alter any contract's bytecode (Outside of system contracts)

[code423n4]

Lines of code

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

Vulnerability details

Impact

forceDeployOnAddresses function is useful to allow governor to update system contracts on existing addresses in case of change in logic by the protocol team . Hence there is no check to ensure that a contract already exists at the address the contract is intended to be deployed.

We also know that System contracts have a kernel space , within which the system contracts are allowed. This is ensured using the MAX_SYSTEM_CONTRACT_ADDRESS constant which is equal to 0xffff.

Whenever a normal user deploys any contract we ensure they don't deploy in the kernel space.

However, when the governor uses the forceDeployOnAddresses function to deploy contracts, there is no check to ensure that he deploys the contract within the kernel space i.e. uint160(_newAddress) < MAX_SYSTEM_CONTRACT_ADDRESS

This is a dangerous power allowed to the admin . I understand the governor has emergency powers to steal funds of any users or ability to regenesis. But , the ability to change other contract's bytecode is not needed and challenges the ethos of immutable code on L2 just like Ethereum .

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

I recommend the following changes to the forceDeployOnAddress :

function forceDeployOnAddress(ForceDeployment calldata _deployment, address _sender) external payable onlySelf {
+      // deploy only if the address is in kernel space , else ignore 
+        if(uint160(_deployment.newAddress)  < MAX_SYSTEM_CONTRACT_ADDRESS){

        _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);
+   }
    }

[c4-judge]

GalloDaSballo marked the issue as duplicate of #141

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-b