zkSync Era System Contracts contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

Force deployment of a contract keep the flag of isConstructor true, while in normal deployment, it is set to false at the end of the deployment.

Proof of Concept

When deploying a contract on zkSync, the flow is: createAccount/create2Account >> _nonSystemDeployOnAddress >> _performDeployOnAddress >> _storeConstructingByteCodeHashOnAddress _constructContract https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L281 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L289

In the _storeConstructingByteCodeHashOnAddress, the flag of of isConstructor will be set to true as well as the bytecodeHash will be stored in AccountCodeStorage. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L304 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L40

In the _constructContract, the msg.value will be transferred to the newly deployed contract, the constructor will be called, and finally the flag of isConstructor will be set to false, indicating that the contract is not in constructing mode anymore (it is already constructed). https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L326 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L52

The inconsistency is that if a contract is deployed through force deployment, the flow is: forceDeployOnAddresses >> forceDeployOnAddress >> _storeConstructingByteCodeHashOnAddress _constructContract (it may not be called) https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L214 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L223

It is similar to normal contract deployment until storing bytecode hash on the address, but if _deployment.callConstructor is false, the _constructContract will not be invoked. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L222

It means that forcing deployment of a contract if _deployment.callConstructor is false, will not mark the deployed contract as constructed (setting the flag isConstructor to false). So, this contract will always be considered as in constructing mode. In other words, unlike the normal deployment of contracts that at the end of deployment the flag isConstructor was set to false, the force deployment will keep the flag as true.

This means that to get extcodehash of this newly force-deployed contract, the result will be EMPTY_STRING_KECCAK. This not true, it should return the getRawCodeHash. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L92

Moreover, to get extcodesize of this newly force-deployed contract, the result will be 0. This is not true, it should return the real length of the code. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L119

Tools Used

Recommended Mitigation Steps

Even if during force deployment, the _deployment.callConstructor is false, the function markAccountCodeHashAsConstructed should be called, as recommended below:

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 {
            ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.markAccountCodeHashAsConstructed(_deployment.newAddress);
        }

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

Lines of code

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

Vulnerability details

Impact

Time-sensitive contracts will be impacted if deployed on zkSync.

Proof of Concept

Many contracts use block.number to measure the time as the miners were able to manipulate the timestamp (the timestamp could be easily gamed over short intervals). So, it was assumed that block.number is a safer and more accurate source of measuring time than timestamp.

For instance, if a defi project sets 144000 block interval to release the interest, it means approximately 144000 * 12 = 20 days. Please note that each block in Ethereum takes almost 12 second.

If the same defi project is deployed on zkSync, it will not operate as expected. Because the there is no time-bound for the blocks in zkSync (the interval may be 30 seconds or 1 week). So, the time to release the interest can be between 50 days to 2762 days.

Since, it is assumed that zkSync is Ethereum compatible, any deployed contracts on Ethereum may deploy their contract in zkSync without noting such big difference.

Even if the contracts use timestamp to measure the time, there will be another issue. In the contract SystemContext.sol, it is possible to set new block with the same timestamp as previous block, but with incremented block number. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/SystemContext.sol#L116

In other words, new blocks are created but their time is frozen. Please note that freezing time can not be lasted for a long time, because when committing block their timestamp will be validated against a defined boundary.

Tools Used

Recommended Mitigation Steps

It should be explicitly mentioned that block intervals in zkSync are not compatible with Ethereum. So, time-sensitive contracts will be noted.

Moreover, the equal sign should be removed in the following line: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/SystemContext.sol#L116

Lines of code

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

Vulnerability details

Impact

During force deployment, if the fund is not properly transferred to the to-be-force-deployed contract, the fund will remain in the contract ContractDeployer and can not easily be recovered.

Proof of Concept

The function forceDeployOnAddresses in contract ContractDeployer is used only during an upgrade to set bytecodes on specific addresses. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L232

The ETH sent to this function will be used to initialize to-be-force-deployed contracts. The ETH sent should be equal to the aggregated value needed for each contract. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L240

Then the function externally calls itself, and send the required value to itself. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L245

If any of this call is unsuccessful, the whole transaction will not revert, and the loop continues to deploy all the contract on the provided newAddress.

If for any reason, the deployment was not successful, the transferred ETH will remain in ContractDeployer, and can not be used for the next deployments (because the aggregated amount is compared with msg.value not the ETH balance of the contract). In other words, FORCE_DEPLOYER fund will be in ContractDeployer, and it can not be easily recoverred.

The possibility of unsuccessful deployment is not low:

It can happen if the bytecode is not known already. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L213 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L296

It can happen during storing constructing bytecode hash. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L214 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/AccountCodeStorage.sol#L36

It can happen during constructing contract and transferring the value. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L223

Tools Used

Recommended Mitigation Steps

By using try/catch, the fund can be transferred to an address that the governor has control to be used later.

function forceDeployOnAddresses(ForceDeployment[] calldata _deployments)
        external
        payable
    {
        // remaining of the code

        for (uint256 i = 0; i < deploymentsLength; ++i) {
            try
                this.forceDeployOnAddress{value: _deployments[i].value}(
                    _deployments[i],
                    msg.sender
                )
            {} catch {
                ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo(
                    address(this),
                    SomeAddress,
                    _deployments[i].value
                );
            }
        }
    }

Q1

In the document, it is noted that the gasLimit is equal to 2^32-1, but in the SystemContext the value is hardcoded to 1<<30. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/SystemContext.sol#L33 https://era.zksync.io/docs/dev/developer-guides/transactions/blocks.html#block-properties

Q2

The revert message is missing. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/SystemContractHelper.sol#L152 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L1Messenger.sol#L47 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/NonceHolder.sol#L135

Q3

The following check is not needed as there is casting to uint128 in line 60. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/MsgValueSimulator.sol#L48-L57 Using safeCastToU128 is recomended. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/Utils.sol#L19

Q4

The modifier onlySystemCall is not needed as it is forced that msg.sender == DEPLOYER_SYSTEM_CONTRACT. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/NonceHolder.sol#L134

Q5

msg.sender is forced to be FORCE_DEPLOYER contract address. So, no need to read msg.sender. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L233 It can be hardcoded, like: this.forceDeployOnAddress{value: _deployments[i].value}(_deployments[i], FORCE_DEPLOYER); https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L245

The same applies to line 212 as it only allows self call, so the _sender is always FORCE_DEPLOYER: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L212

Q6

If by chance the _newAddress during creating an account is in kernel space, the user can not deploy the contract. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L181 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L256

The probability is very very low, but since the kernel space is 2**16 -1, the chance of address collision is higher than on Ethereum.

In that case, the user should change the bytecode of to-be-deployed contract, or increase his deployment nonce by deploying just a dummy contract.

Q7

If bootlaoder calls markFactoryDeps, the parameter _l1PreimageHash is set zero as the parameter sent to _markBytecodeAsPublished. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/KnownCodesStorage.sol#L36

When BytecodeCompressor during publishing the compressed bytecode, calls markBytecodeAsPublished, parameter _shouldSendToL1 is false in _markBytecodeAsPublished, so the if clause will not be executed. https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L63 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/KnownCodesStorage.sol#L51 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/KnownCodesStorage.sol#L68

So, the parameter _l1PreimageHash is not accessed at all, and can be removed.

Q8

If the processFlags is something except 0x00 and 0x02, isETHCall is considered as false. So, it will be interpreted as normal mode (validate and execute). https://github.com/code-423n4/2023-03-zksync/blob/01379615bc20c2926d81c0a182f1abb6e922b93a/bootloader/bootloader.yul#L579 It is recommended to use switch/case to handle invalid cases.

Q9

In EVM, blockhash(block.number) = bytes32(0). In zKSync, when emulating the opcode blockhash, it is better to explicitly define the case that _block is equal to block.number will results in bytes32(0). https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/SystemContext.sol#L74

Although if _block == block.number, the else branch will be executed, and the mapping returns bytes32(0), but in case the blocks in L2 are reorged and the state of the blockhash is not set to zero, we may have blockHash[block.number] != 0.

The recommendation is using <= instead of <:

function getBlockHashEVM(uint256 _block) external view returns (bytes32 hash) {
        if (block.number <= _block || block.number - _block > 256) {
            hash = bytes32(0);
        } else {
            hash = blockHash[_block];
        }
    }

Or

function getBlockHashEVM(uint256 _block) external view returns (bytes32 hash) {
        if (block.number < _block || block.number - _block > 256) {
            hash = bytes32(0);
        } else {
            hash = blockHash[_block];
            if(_block == block.number){
                 assert(hash == bytes32(0));
            }
        }
    }