code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L181-L193 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L110-L121 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L249-L267

Vulnerability details

Impact

Function createAccount() Deploys a contract account with similar address derivation rules to the EVM's CREATE opcode. the deployed contract address is calculated based on sender deployed nonce. code uses _nonSystemDeployOnAddress() to deploy the contract to new address but it won't allow deploying to the kernel space addresses. This can create an issue for factory contracts that deploys new contracts and the calculated address is in kernel space, then those contracts transaction would revert always and they can't create new contract as deployment nonce won't increase and the calculated address would always be in kernel space.

Proof of Concept

This is _nonSystemDeployOnAddress() code:

    function _nonSystemDeployOnAddress(
        bytes32 _bytecodeHash,
        address _newAddress,
        AccountAbstractionVersion _aaVersion,
        bytes calldata _input
    ) internal {
        require(_bytecodeHash != bytes32(0x0), "BytecodeHash can not be zero");
        require(uint160(_newAddress) > MAX_SYSTEM_CONTRACT_ADDRESS, "Can not deploy contracts in kernel space");

        // We do not allow deploying twice on the same address.
        require(
            ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.getCodeHash(uint256(uint160(_newAddress))) == 0x0,
            "Code hash is non-zero"
        );
        // Do not allow deploying contracts to default accounts that have already executed transactions.
        require(NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(_newAddress) == 0x00, "Account is occupied");

        _performDeployOnAddress(_bytecodeHash, _newAddress, _aaVersion, _input);
    }

As you can see code checks that uint160(_newAddress) > MAX_SYSTEM_CONTRACT_ADDRESS and makes sure that the calculated address for deployed contract is not in kernel space. This is createAccount() code:

    function createAccount(
        bytes32, // salt
        bytes32 _bytecodeHash,
        bytes calldata _input,
        AccountAbstractionVersion _aaVersion
    ) public payable override onlySystemCall returns (address) {
        uint256 senderNonce = NONCE_HOLDER_SYSTEM_CONTRACT.incrementDeploymentNonce(msg.sender);
        address newAddress = getNewAddressCreate(msg.sender, senderNonce);

        _nonSystemDeployOnAddress(_bytecodeHash, newAddress, _aaVersion, _input);

        return newAddress;
    }

As you can see it uses getNewAddressCreate(msg.sender, senderNonce) to calculate address for deployed contract address and the calculated address only is based on the msg.sender and sender deployment nonce so the deployment address would be the same for an address if deployment nonce won't change. imagine this scenario:

there is FactoryA contract in a Protocol that deploys new Vault contract by calling ContractDeployer.createAccount(). the FactoryA is not upgradable.
after some time, the deployment Nonce for FactoryA reaches 10 and the calculates address for newly deployed address would be in kernel space. (the return value for getNewAddressCreate(FactoryA, 10) is lower than MAX_SYSTEM_CONTRACT_ADDRESS)
now FactoryA logics that creates new Vault won't work as it tries to create new contracts by calling to createAccount() which would revert because the calculated address is in kernel space. the deployment nonce for factoryA would stay in 10 forever.
because FactoryA is not upgradable and it's logics reverts so it can cause users of the FactoryA's protocol to lose access to their funds and their funds would be locked forever.

As you can see this can cause a contract to be broken. the chance that the issue happens is low but as the chance of the this check require(uint160(_newAddress) > MAX_SYSTEM_CONTRACT_ADDRESS, "Can not deploy contracts in kernel space"); in _nonSystemDeployOnAddress() is low too but it is implemented in the code so it's logical to assume that there may be a situation that this require check would cause a revert to deployment and it can cause a protocol that rely on deployment to be in broken state.

the issue is not serious for create2Account() as by changing the deployed contract bytecode it would be possible to deploy it because the calculated address would be different.

Tools Used

VIM

Recommended Mitigation Steps

function createAccount() should be more robust. code should check that if newAddress is in kernel space it should increase deployment nonce again. its like there is no deployment for that nonce and code continue with the next nonce. (to be full bulletproof it can be in a loop that code increase the nonce and check the calculated address to be sure that it's not in the kernel space). if they don't want to implement a solution for this, this risk should be mentioned and users should be know that using createAccount() is risky and may break the protocols logics.

GalloDaSballo commented 1 year ago

Very interesting finding, which can be a gotcha for factory deployed that rely on deterministic deployment (e.g. UniV3 factory)

miladpiri commented 1 year ago

The kernel space is addresses under ≤ 2**16, meaning that it would require to find preimage to hash with 20 - 2 = 18 zeroes. If someone can find this for keccak, we have bigger problems. Hash collision possibility is super low in this case. Moreover, this can happen also in Ethereum, if the factory is going to deploy on an address that is alreayd occupied, it will stuck forever if it is not upgradable.

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

I have to agree with the sponsor that the odds of finding such a hash are 1 in 1.7668471e+72

This is only 5 orders of magnitude less than 2^256, effectively making them comparable

I have considered QA, but am closing as invalid as I have to agree that if such a pre-image was found, we'd have bigger issues

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

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