zkSync Era System Contracts contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/ContractDeployer.sol#L212-L227 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/AccountCodeStorage.sol#L34-L58

Vulnerability details

Impact

In case if predeployed contract will be updated using ContractDeployer.forceDeployOnAddress without calling constructor, then this contract will be updated to construction state, which will make it behave like eoa, so it will be not possible to call functions of that contract anymore. Because of that some transactions on zksync network will be ddosed.

Proof of Concept

When it's needed to deploy new contract, FORCE_DEPLOYER can call ContractDeployer.forceDeployOnAddress function. This function can be called in order to deploy new contract, or just update existing one. In case if _deployment.callConstructor param is false, that means that we want to update existing contract's code, without calling it's constructor.

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/ContractDeployer.sol#L212-L227

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

The function forceDeployOnAddress first checks that bytecode is known, then it calls _storeConstructingByteCodeHashOnAddress, which will set account to construction state.

Later, in case if _constructContract is called, then after constructor is called, then state of account is set to constructed.

However, in case of predeploy update, _constructContract function will not be called and account will be in construction state forever.

In the docs we can see following: https://github.com/code-423n4/2023-03-zksync/tree/main#constructing-vs-non-constructing-code-hash To prevent contracts from being able to call a contract during its construction, we set the marker (i.e. second byte of the bytecode hash of the account) as 1. This way, the VM will ensure that whenever a contract is called without the is_constructor flag, the bytecode of the default account (i.e. EOA) will be substituted instead of the original bytecode.

That means, that after predeploy is updated, then it's marked as construction and calls to it will be substituted, so it will behave like eoa. That means that calls to predeployed contract's functions will return return(0, 0). Because of that some functionality on zksync will stop working, depending on the updated predeployed contract.

Tools Used

VsCode

Recommended Mitigation Steps

You can add else clause and set state to constructed

    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/main/contracts/SystemContext.sol#L116

Vulnerability details

Impact

When new block timestamp is set by bootloader, then it's allowed for it be same as previous block's timestamp. In case if protocols think that block can not have same timestamp it can be a problem for them.

Proof of Concept

SystemContext.setNewBlock is called by bootloader in order to provide new block and it's timestamp. https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/SystemContext.sol#L109-L128

    function setNewBlock(
        bytes32 _prevBlockHash,
        uint256 _newTimestamp,
        uint256 _expectedNewNumber,
        uint256 _baseFee
    ) external onlyBootloader {
        (uint256 currentBlockNumber, uint256 currentBlockTimestamp) = getBlockNumberAndTimestamp();
        require(_newTimestamp >= currentBlockTimestamp, "Timestamps should be incremental");
        require(currentBlockNumber + 1 == _expectedNewNumber, "The provided block number is not correct");


        blockHash[currentBlockNumber] = _prevBlockHash;


        // Setting new block number and timestamp
        currentBlockInfo = (currentBlockNumber + 1) * BLOCK_INFO_BLOCK_NUMBER_PART + _newTimestamp;


        baseFee = _baseFee;


        // The correctness of this block hash and the timestamp will be checked on L1:
        SystemContractHelper.toL1(false, bytes32(_newTimestamp), _prevBlockHash);
    }

As you can see, function checks that next block is same as previous + 1. Also it checks that timestamp is not less than previous block's timestamp.

So according to this code, it's possible for several blocks to have same block.timestamp. As zksync is l2 of ethereum, so developers can assume, that there can not be different blocks with same timestamp. So if their code somehow relies on that it can create problems for them.

Pls, note that i am not trying to say that operator can manipulate with timestamps here(of course he can), i am trying to say that it's possible that two zksync blocks can be created with same timestamp without any malicious actions.

I couldn't find any example of protocols that can be impacted now if they deploy same code they have on ethereum to zksync, but the protocol logic can be as following. Protocol can have some funds inside and it supposes to pay X amount anyone who will call specific function at timestamp Y. Functions also relies on maximum transactions count that can be sealed in the 1 block. They have calculated that maximum 30 their transactions can be called inside 1 block. So maximum they should pay is 30*X. But because of the fact that zksync can have blocks with same timestamp their logic will fail and they can pay more times.

The problem here for me is that developers can't just deploy same code to zksync that is deployed to ethereum because of that.

Tools Used

VsCode

Recommended Mitigation Steps

Make it not possible to have blocks with same timestamp.

KnownCodesStorage._sendBytecodeToL1 burns more gas, because BYTECODE_PUBLISHING_OVERHEAD constant is incorrect.

Description

KnownCodesStorage._sendBytecodeToL1 function uses BYTECODE_PUBLISHING_OVERHEAD constant as gas overhead, which is 100. But L1Messenger.sendToL1 uses 64 gas as overhead. As was disscussed with developer, KnownCodesStorage._sendBytecodeToL1 function should also use value 64 as gas overhead.

Recommendation

Change BYTECODE_PUBLISHING_OVERHEAD variable to value 64.

BytecodeCompressor.publishCompressedBytecode will publish already known bytecode lo l1 again.

Description

When BytecodeCompressor.publishCompressedBytecode is called, then bytecode is published lo l1. After that it is marked as published and known inside KnownCodesStorage contract. And in case if someone will call BytecodeCompressor.publishCompressedBytecode for same bytecode again, it will be sent to l1 again, however it is already saved. So user will pay for gas.

Recommendation

In case if bytecode is known by KnownCodesStorage then do not send it lo l1.

ContractDeployer.forceDeployOnAddress allows FORCE_DEPLOYER to change code for any account.

Description

ContractDeployer.forceDeployOnAddress function allows FORCE_DEPLOYER to deploy and change account not only for precompiles(system contracts). It allows to change code for any account.

Because of that, FORCE_DEPLOYER can do anything he wants and steal all funds from any account.

Recommendation

Allow to FORCE_DEPLOYER to deploy and change only system contracts.

NonceHolder.increaseMinNonce can silently overflow

Description

NonceHolder.increaseMinNonce function increases rawNonces for address. It's allowed to increase nonce by maximum MAXIMAL_MIN_NONCE_INCREMENT value. Later, rawNonces is increased inside unchecked block. Because of that it's possible that rawNonces for some account will overflow, in case if increaseMinNonce function will be caled too many times(more than 2^96 times) with value of MAXIMAL_MIN_NONCE_INCREMENT. After that nonces can be repeated for the account.

Recommendation

Add check for overflow.