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

when contracts get deployed, code set their immutable values in the ImmutableSimulator contract and the default value for any immutable index is 0. function forceDeployOnAddress() is to be used only during an upgrade to set bytecodes on specific addresses but the code doesn't reset the immutable values for the upgrading contract. This will create state for the contract after deployment which can result in unexpected behavior.

Proof of Concept

This is forceDeployOnAddress() code:

    /// @notice The method that can be used to forcefully deploy a contract.
    /// @param _deployment Information about the forced deployment.
    /// @param _sender The `msg.sender` inside the constructor call.
    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 it calls _constructContract() which is:

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

which sets the deployed contract's immutables based on the return value of the constructor. Function forceDeployOnAddress() can be used to redeploy a new code to an address, and that address may have immutables set for it in previous deploys. As you can see in both _constructContract() and forceDeployOnAddress() there is no logic to reset the previously set immutables, This can cause problems in this two scenario:

the new code has fewer immutables but code access the higher index immutables, in the test cases those immutables has 0 value(the default value for non-existing immutables is 0) but in the deployed contract those immutables would have another values(which is set in the previous deploys).
even if the new code doesn't access the higher index immutables directly, the immutables for that address would show wrong values for the newly deployed code which has fewer immutables than older code. this may cause problem for any logic that depend on the immutable values of this contract.
if forceDeployOnAddress() get called with callConstructor as false, then the constructor of the new code won't get called and no new immutables would have been set for that address, but the immutables would have values from previous deploys. because the new code would be different than the previous code, so there is no guarantee that the value of the immutables are the same for them.
the address would have different immutable values if it was deployed newly or if it was upgraded which is different behavior for the same functionality in the system.

Tools Used

VIM

Recommended Mitigation Steps

code should have some value so deployer define that if he wants the immutables to be reset before deployment or not.

miladpiri commented 1 year ago

All the immutables of the contracts are reset during after the call to the consutrctor (it is expected that the compiler will set the needed immutables). I don’t think it is a security issue. The immutables should be rewriten.

Severity is QA.

c4-sponsor commented 1 year ago

miladpiri marked the issue as disagree with severity

GalloDaSballo commented 1 year ago

I have requested more info to the Warden, I would downgrade to QA in lack of additional proof.

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)