Losing fund during force deployment

[code423n4]

Lines of code

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

Vulnerability details

Impact

During force deployment, if the fund is not properly transferred to the to-be-force-deployed contract, the fund will remain in the contract ContractDeployer and can not easily be recovered.

Proof of Concept

The function forceDeployOnAddresses in contract ContractDeployer is used only during an upgrade to set bytecodes on specific addresses. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L232

The ETH sent to this function will be used to initialize to-be-force-deployed contracts. The ETH sent should be equal to the aggregated value needed for each contract. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L240

Then the function externally calls itself, and send the required value to itself. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L245

If any of this call is unsuccessful, the whole transaction will not revert, and the loop continues to deploy all the contract on the provided newAddress.

If for any reason, the deployment was not successful, the transferred ETH will remain in ContractDeployer, and can not be used for the next deployments (because the aggregated amount is compared with msg.value not the ETH balance of the contract). In other words, FORCE_DEPLOYER fund will be in ContractDeployer, and it can not be easily recoverred.

The possibility of unsuccessful deployment is not low:

It can happen if the bytecode is not known already. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L213 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L296

It can happen during storing constructing bytecode hash. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L214 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L36

It can happen during constructing contract and transferring the value. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L223

Tools Used

Recommended Mitigation Steps

By using try/catch, the fund can be transferred to an address that the governor has control to be used later.

function forceDeployOnAddresses(ForceDeployment[] calldata _deployments)
        external
        payable
    {
        // remaining of the code

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

[GalloDaSballo]

Loss of funds due to reverts, keeping separate for now

[GalloDaSballo]

See #95 for loss of value due to not calling constructor

[miladpiri]

The goal was to by force deploy multiple of contracts (especially system contracts during the upgrade). If any deployment was unsuccessful, the whole transaction should not be reverted. So, the fund reverted during the failed deployment should be transferred to a valid address (not stay in ContractDeployer) as suggested by the warden. The recommended mitigation is also good.

Severity is Medium.

[c4-sponsor]

miladpiri marked the issue as sponsor confirmed

[GalloDaSballo]

Per the Sponsors comment, the Warden has shown how, due to a lack of sweep on revert, funds sent in a sequence of multiple deployments can be lost when one of the deployment fails.

This is in contrast to having the entire deployment reverting

Because the behavior is unintended and funds can be lost conditionally on a revert, I believe Medium Severity to be appropriate

[c4-judge]

GalloDaSballo marked the issue as selected for report