code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Function forceDeployOnAddress() is to be used only during an upgrade to set bytecodes on specific addresses. by setting callConstructor as false, code won't call constructor after deployment and then the msg.value would stuck in the ContractDeployer address for ever. when callConstructor is false, either code should send the msg.value to the deployed address or it should make sure that msg.value is zero. for issue to happen deployer should send ETH while setting callConstructor as false, As there is no warning about this in the documentation so it would be considered a valid interaction with ContractDeployer, because when deploying a contract by sending ETH, the ETH is supposed to be transferred to the deployed address.

Proof of Concept

This is forceDeployOnAddress() code:

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

As you can see the function in payable and by calling _constructContract() code sends the msg.value to the deployed contract address. but when callConstructor is false then code would not call the constructor and won't transfer the ETH to the deployed contract address and there is not check that msg.value is 0 and so funds would stuck in the ContractDeployer address.

Tools Used

VIM

Recommended Mitigation Steps

Either send the msg.value ETH to the deployed contract address directly when constructor is not called, or check that msg.value is 0 when callConstructor is false.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #95

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

L

code-423n4 / 2023-03-zksync-findings