Deployment Nonce Does not Increment For a Reverted Child Contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L344

Vulnerability details

Impact

There is an inconsistency between zkSync Era and Ethereum Virtual Machine (EVM) in handling deployment nonce leading to significant consequences. In EVM, the factory's nonce is increased at the beginning of child contract creation, and if the child contract's creation fails, the nonce is not rolled back. In zkSync Era, the deployment nonce increases during the deployment process, and if the child contract constructor reverts, the entire transaction, including the increased deployment nonce, also reverts.

Proof of Concept

In zkSync Era, when a factory contract deploys another contract using low-level creation operations (i.e. using CREATE or CREATE2 in assembly, instead of using new), it is expected that the deployment nonce of the factory will increment by one, akin to the behavior in the Ethereum Virtual Machine (EVM). However, a notable issue arises when the constructor of the child contract encounters a revert.

In the EVM, the nonce of the factory increases as soon as the CREATE operation begins, regardless of whether the child contract creation is ultimately successful. This can be seen in GETH codebase, where the nonce of the factory is increased at line 431. Then, the EVM captures a snapshot, at line 443, to manage any potential failures, allowing for a graceful revert, at line 492, in case of unsuccessful child contract creation. In such cases, the returned address is address(0), indicating a failed contract creation attempt. https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L431 https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L443 https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L492

However, zkSync Era differs in its approach. When the CREATE opcode is employed, it invokes the create or create2 function within the ContractDeployer contract. https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L146 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L130 This action results in the incremented deployment nonce of the factory, and, in the final stages of contract creation, it calls the constructor of the child contract. https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L189 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L344 In the event of a revert in the child contract's constructor, the entire transaction in the ContractDeployer is reverted. Consequently, the incremented deployment nonce of the factory is also rolled back, and the returned address signifies an unsuccessful contract creation attempt by being set to address(0).

This divergence in nonce management between zkSync Era and the EVM could have far-reaching implications, especially for factories that are deployed on zkSync Era and assume that the deployment nonce behaves in a manner consistent with the EVM.

PoC

Calling the function test triggers the internal function deploy three times. In each call, the child contract is deployed, and two values are returned: the predicted address of the to-be-deployed child and the address of the deployed child. The first and third deploy calls provide false as an argument to ensure that the constructor of the child doesn't revert. In the second deploy call, true is used as an argument, causing the constructor of the child to revert.

Comparing the predicted addresses with the deployed addresses reveals the following:

• In EVM: When the second child contract reverts, the factory's nonce increases. Consequently, the predicted address of the third child still matches its deployed address.
• In zkSync Era: When the second child contract reverts, the factory's nonce remains unchanged. As a result, the deployed address of the third child matches the predicted address of the second child.

It's worth noting that the initial value of the variable nonce is set to one for the EVM PoC, while it is set to zero for the zkSync Era PoC. This difference is unrelated to this report and has been explained in a separate report.

In EVM:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.17;

contract Factory {
    uint256 nonce = 1;

    address public predictedAddress1;
    address public predictedAddress2;
    address public predictedAddress3;

    address public deployedAddress1;
    address public deployedAddress2;
    address public deployedAddress3;

    function test() public {
        (predictedAddress1, deployedAddress1) = deploy(false);
        (predictedAddress2, deployedAddress2) = deploy(true);
        (predictedAddress3, deployedAddress3) = deploy(false);
    }

    function deploy(bool _shouldRevert) internal returns (address, address) {
        bytes memory bytecode = type(Child).creationCode;
        bytecode = abi.encodePacked(bytecode, abi.encode(_shouldRevert));

        address addr;
        assembly {
            addr := create(0, add(bytecode, 0x20), mload(bytecode))
        }

        return (predict(), addr);
    }

    function predict() public returns (address) {
        address predicted = address(
            uint160(
                uint256(
                    keccak256(
                        abi.encodePacked(
                            bytes1(0xd6),
                            bytes1(0x94),
                            address(this),
                            uint8(nonce++)
                        )
                    )
                )
            )
        );
        return predicted;
    }
}

contract Child {
    constructor(bool _shouldRevert) {
        if (_shouldRevert) {
            revert();
        }
    }
}

The result for EVM is:

• predictedAddress1: 0xbd5b354220B250DF257ed5e988Fe8FE81CdB6235

• deployedAddress1: 0xbd5b354220B250DF257ed5e988Fe8FE81CdB6235

• predictedAddress2: 0x02180dD815cA64898F6126f3911515B06D17acaD

• deployedAddress2: 0x0000000000000000000000000000000000000000

• predictedAddress3: 0x0A9C6D9d0AF27FC9F3F96196c3F8c89C79Df287D

• deployedAddress3: 0x0A9C6D9d0AF27FC9F3F96196c3F8c89C79Df287D

In zkSync Era:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.17;

contract Factory {
    uint256 nonce = 0;

    address public predictedAddress1;
    address public predictedAddress2;
    address public predictedAddress3;

    address public deployedAddress1;
    address public deployedAddress2;
    address public deployedAddress3;

    function test() public {
        (predictedAddress1, deployedAddress1) = deploy(false);
        (predictedAddress2, deployedAddress2) = deploy(true);
        (predictedAddress3, deployedAddress3) = deploy(false);
    }

    function deploy(bool _shouldRevert) internal returns (address, address) {
        bytes memory bytecode = type(Child).creationCode;
        bytecode = abi.encodePacked(bytecode, abi.encode(_shouldRevert));

        address addr;
        assembly {
            addr := create(0, add(bytecode, 0x20), mload(bytecode))
        }

        return (predict(), addr);
    }

    function predict() public returns (address newAddress) {
        bytes32 hash = keccak256(
            bytes.concat(
                keccak256("zksyncCreate"),
                bytes32(uint256(uint160(address(this)))),
                bytes32(nonce++)
            )
        );

        newAddress = address(uint160(uint256(hash)));
    }
}

contract Child {
    constructor(bool _shouldRevert) {
        if (_shouldRevert) {
            revert();
        }
    }
}

The result for zkSync Era is:

• predictedAddress1: 0xBADCfBd5E90eA6558580B8C6220b750E9438df7c

• deployedAddress1: 0xBADCfBd5E90eA6558580B8C6220b750E9438df7c

• predictedAddress2: 0xB7E8A1191E6Ef0F3A752eCaa050F0FF7BcaEAF51

• deployedAddress2: 0x0000000000000000000000000000000000000000

• predictedAddress3: 0xfBAE6B995fe81b48A5e389A81E8Af0ee2AaF7302

• deployedAddress3: 0xB7E8A1191E6Ef0F3A752eCaa050F0FF7BcaEAF51

As you see, the deployedAddress3 is equal to predictedAddress2 which is not correct.

Tools Used

Recommended Mitigation Steps

A potential solution might involve invoking the _constructContract function externally within a try/catch block. By doing so, if _constructContract reverts (as occurred with the child contract), the deployment nonce won't be rolled back. However, implementing this kind of modification would necessitate reviewing and changing other parts of the code as well.

function _performDeployOnAddress(
        bytes32 _bytecodeHash,
        address _newAddress,
        AccountAbstractionVersion _aaVersion,
        bytes calldata _input
    ) internal {
        // ...

        try this.constructContract(msg.sender, _newAddress, _bytecodeHash, _input, false, true){//...}
        catch{//...}

    }

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

miladpiri (sponsor) acknowledged

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[miladpiri]

This can have high impact (it depends on the context), and the probablity is medium to high (implementing factory contract is frequent). Medium severity can be fair.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-sponsor]

miladpiri (sponsor) acknowledged

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

At this time, I am marking the finding as Med for EVM non equivalence, please do raise concerns if you can show a specific way to cause loss of funds or broken behaviour for contracts that are widely used

[c4-judge]

GalloDaSballo marked the issue as selected for report

[HE1M]

@GalloDaSballo Thanks for your effort

This issue may pose a challenge when a factory contract, sharing the same address, is deployed on different chains and is intended to generate multiple proxy contracts with identical addresses (because the address of the created proxy contract relies on the factory's address and its nonce).

Consider a situation where a factory is deployed with the same address on both Ethereum and zkSync Era (its forks, or other chains). The address of the proxy contract generated by this factory is dependent on the factory's address and its nonce. If, for any reason, a proxy creation fails (for instance, encountering a REVERT during the execution of its initcode or running out of gas), the nonce of the factory will increase on Ethereum but not in the zkSync Era. Consequently, the nonces of the factory on both chains will no longer be synchronized, leading to divergent addresses for proxies created by this factory on the two chains going forward.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

contract Factory {

    function deploy(bytes[] memory _proxy) external {
        bytes memory bytecode;

        for (uint256 i = 0; i < _proxy.length; ++i) {
            bytecode = _proxy[i];
            assembly {
                pop(create(0, add(bytecode, 0x20), mload(bytecode)))
            }
        }
    }

    function proxyAddress(uint256 _nonce) public view returns (address) {
        address predicted = address(
            uint160(
                uint256(
                    keccak256(
                        abi.encodePacked(
                            bytes1(0xd6),
                            bytes1(0x94),
                            address(this),
                            uint8(_nonce)
                        )
                    )
                )
            )
        );
        return predicted;
    }
}

Given the frequent implementation of factory contracts (whether for ensuring the consistency of proxy contract addresses across different chains or for other use cases), the impact classification of this issue can be high.

[0xunforgiven]

Hey @HE1M, Beautiful finding as always.

Regarding your above comment, which says:

Consider a situation where a factory is deployed with the same address on both Ethereum and zkSync Era (its forks, or other chains).

I want to mention that according the to docs currently it's impossible to for Ethereum and zkSync contract to have sameaddress. docs:

For most of the rollups the address aliasing needs to prevent cross-chain exploits that would otherwise be possible if we simply reused the same L1 addresses as the L2 sender. In zkSync Era address derivation rule is different from the Ethereum, so cross-chain exploits are already impossible. However, zkSync Era may add full EVM support in the future, so applying address aliasing leaves room for future EVM compatibility.

[HE1M]

Hey @0xunforgiven , appreciate your point.

You are correct that the address derivation in zkSync Era differs from Ethereum. However, my key point is that developers considering the idea of using a factory to generate proxies with identical addresses (on zkSync Era, its forks, and etc.) will encounter the issue of nonce asynchronization.

[miladpiri]

Agree. This can be an issue.

[GalloDaSballo]

I believe that the finding is Valid and Medium, it can cause issues, in some specific scenarios, which are possible but not as common and they are implementation reliant The issues are not guaranteed, the behaviour is "sometimes" wrong Meaning that Medium Severity is the most appropriate

If a contract factory was in scope, I can see the argument for raising the severity, but a create2 or create3 deployer, as well as common factories with mappings for children, would work as intended, leading me to confirm Medium Severity as the most appropriate