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. This function reset the deployed contract's nonceOrdering to Sequential always. but if contract has been deployed before and has Arbitrary nonce ordering, and Protocol wants to upgrade the contract's code, then code would set the nonce ordering to Sequential which would break the contract and transaction would revert because code can't set nonce as used as it's logic is based on Arbitrary ordering. This can cause any process and logics that are depending on that contract to fail and may result in fund loss too if the contract has been used in bridging assets and transactions(when there is no replay). The same issue is there for supportedAAVersion variable too. the deployed contract may support custom account version but code set the supporting value to None.

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 code sets the ordering to Sequential always and doesn't check that newAddress's current nonce ordering is Arbitrary or not. This would create issue when a contract has been deployed to that address before and set the ordering as a Arbitrary. When ordering is Arbitrary contract's may use NonceHolder.setValueUnderNonce() to mark arbitrary nonce as used, This is setValueUnderNonce() code:

    function setValueUnderNonce(uint256 _key, uint256 _value) public onlySystemCall {
        IContractDeployer.AccountInfo memory accountInfo = DEPLOYER_SYSTEM_CONTRACT.getAccountInfo(msg.sender);

        require(_value != 0, "Nonce value can not be set to 0");
        // If an account has sequential nonce ordering, we enforce that the previous
        // nonce has already been used.
        if (accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential && _key != 0) {
            require(isNonceUsed(msg.sender, _key - 1), "Previous nonce has not been used");
        }

        uint256 addressAsKey = uint256(uint160(msg.sender));

        nonceValues[addressAsKey][_key] = _value;

        emit ValueSetUnderNonce(msg.sender, _key, _value);
    }

As you can see when the nonce ordering is Sequential code checks that the key-1 has been used before. so a contract that the ordering is Arbitrary doesn't have to worry about that nonce are sequential and can mark any nonce as used but if all of a sudden this contract ordering set as Sequential after upgrading, then contract can't set nonce as used and calls to setValueUnderNonce() would revert and none of the transaction would get executed for this contract. Imagine this scenario:

ContractA has using Arbitrary type nonce ordering and it calls setValueUnderNonce() to mark nonce as used.
Protocol upgrade ContractA code by calling forceDeployOnAddress() and it would set the nonce ordering as Sequential.
then ContractA calls to setValueUnderNonce() would fail for arbitrary nonces and transactions would revert.

This can cause variety of impacts based on the usage of the upgraded contract, in all cases the contract would be in broken state and funds would be locked and it may result in funds loss too if the failed transactions won't be repayable (for example in the bridge).

There is another issue here too, contract set supportedAAVersion value as None by default, This can cause issue if already deployed contract support Abstract Accounts, then after new deployment the AccountInfo would show wrong information about contract.

Tools Used

VIM

Recommended Mitigation Steps

Function forceDeployOnAddress() shouldn't assumes that it's the first time code is deployed in that address and before setting the default values for AccountInfo, it should check that if it's the first time. if any code deployed to that address before, then it shouldn't set the default values for the AccountInfo of that address.

GalloDaSballo commented 1 year ago

Similar to #66 but keeping separate for now

miladpiri commented 1 year ago

Similar to all reports indicating forcing deployment on any address is risky. Anyway, It is allowed by design (intended) (a bad governor can do worse things via upgrade).

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

Per similar discussion to #66 and #141 am downgrading to QA

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

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