forceDeployOnAddress() may lose value

[code423n4]

Lines of code

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

Vulnerability details

Impact

if not nedd callConstructor , value will be ignored, may lose value

Proof of Concept

FORCE_DEPLOYER call forceDeployOnAddresses () can pass _deployments.value The current implementation, if need callConstructor, will pass the value to newAddress, and set the msg.value for Constructor If callConstructor is not needed, it is directly ignored.

    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); //<------will transfer value to newAddress in _constructContract()
        }
        //<------but if don't need callConstructor value will stay in this contract

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

    function _constructContract(address _sender, address _newAddress, bytes calldata _input, bool _isSystem) internal {
        // Transfer the balance to the new address on the constructor call.
        uint256 value = msg.value;
        if (value > 0) {
            ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo(address(this), _newAddress, value);//<------will transfer value to newAddress
            // Safe to cast value, because `msg.value` <= `uint128.max` due to `MessageValueSimulator` invariant
            SystemContractHelper.setValueForNextFarCall(uint128(value));
        }

        bytes memory returnData = EfficientCall.mimicCall(gasleft(), _newAddress, _input, _sender, true, _isSystem);
        ImmutableData[] memory immutables = abi.decode(returnData, (ImmutableData[]));
        IMMUTABLE_SIMULATOR_SYSTEM_CONTRACT.setImmutables(_newAddress, immutables);
        ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.markAccountCodeHashAsConstructed(_newAddress);
    }    

So if have pass value, although don't need callConstructor,also need to transfer value to newAddress

Tools Used

Recommended Mitigation Steps

    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);        
-       }
+       }else{
+             if (msg.value > 0) {
+               ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo(address(this), _deployment.newAddress, msg.value);
+             }
+       }

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

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Slightly more concise but basically both reports are as good

[miladpiri]

IMHO Low, but it is something that it is possible to forget in the future upgrades. The impact is also very limited.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[GalloDaSballo]

I believe the finding to be valid, but reliant on user mistake: https://github.com/code-423n4/org/issues/53

Downgrading to Low

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c