zkSync Era System Contracts contest - minaminao's results

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L223

Vulnerability details

Impact

The fallback function in DefaultAccount is defined as fallback() external {...} , but it must be fallback() external payable {...} . This bug causes transfers with calldata to default accounts always to be reverted. On the other hand, they are not be reverted in Ethereum. Since DefaultAccount is required to behave like EOA, this fact is inconsistent with the specification.

In addition, in the ignoreNonBootloader and ignoreInDelegateCall modifiers, there is the following statement, but as mentioned above, DefaultAccount implements an empty fallback instead of an empty payable fallback, so it can be distinguished.

If all functions will use this modifier AND the contract will implement an empty payable fallback() then the contract will be indistinguishable from the EOA when called.

This bug makes it impossible to transfer Ether to any default account from any contract that implements transfers with calldata. In particular, if Ether deposited in a contract can only be withdrawn via a transfer with calldata, the assets will be permanently frozen.

For example, calling the returnEther function of the following contract has no risk in Ethereum. The Ether is simply transferred to Foo and returned. However, in zkSync Era, the above bug causes the Ether to not be returned, and it remains in Foo and is lost.

contract Foo {
    function returnEther() public payable {
        payable(msg.sender).call{value: msg.value}("00");
    }
}

Transfers with calldata are general, as seen in 2023-03-zksync/blob/main/contracts/openzeppelin/utils/Address.sol#L159 (this functionCallWithValue checks for success, so the asset is not lost and only reverted). Therefore, the possibility of asset loss in various protocols due to this vulnerability is very high.

Proof of Concept

The code causing this vulnerability: https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/DefaultAccount.sol#L223

The test below is a transfer with calldata to a default account address (0x100000); for Ethereum, this succeeds, but for zkSync Era, it fails. This is dangerous.

//! { "cases": [ {
//!     "name": "test",
//!     "inputs": [
//!         {
//!             "method": "test",
//!             "calldata": [
//!             ],
//!             "value": "1 ETH"
//!         }
//!     ],
//!     "expected": [
//!     ]
//! } ] }

// SPDX-License-Identifier: UNLICENSED 

pragma solidity >=0.4.16;

contract Test {
    function test() external payable {
        address defaultAccountAddress = address(0x100000);
        (bool success, ) = payable(defaultAccountAddress).call{value: msg.value}("00");
        require(success); // failed
    }
}

The above vulnerability can cause users' assets to be lost in a variety of scenarios. For example, the following test shows that the user's assets are locked to the Foo contract forever. Since it seems to be not simple to call transfer with calldata from DefaultAccount using era-compiler-tester, SimplifiedDefaultAccountForTest compatible with DefaultAccount is used. The result is the same when DefaultAccount is used.

//! { "cases": [ {
//!     "name": "test",
//!     "inputs": [
//!         {
//!             "method": "test",
//!             "calldata": [
//!             ],
//!             "value": "1 ETH"
//!         }
//!     ],
//!     "expected": [
//!     ]
//! } ] }

// SPDX-License-Identifier: UNLICENSED 

pragma solidity >=0.4.16;

contract Test {
    function test() external payable {
        // A simplified contract is used instead of a DefaultAccount contract, but the result is the same.
        SimplifiedDefaultAccountForTest account = new SimplifiedDefaultAccountForTest{value: 1 ether}();
        Foo foo = new Foo();
        account.callFooReturnEther(address(foo));
        require(address(account).balance == 1 ether); // failed
    }
}

uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000;
address payable constant BOOTLOADER_FORMAL_ADDRESS = payable(address(SYSTEM_CONTRACTS_OFFSET + 0x01));

contract SimplifiedDefaultAccountForTest {
    constructor() payable {} 

    // This process is implemented as a function here, but in reality, it is a transaction by an EOA.
    function callFooReturnEther(address fooAddress) external {
        Foo(fooAddress).returnEther{value: address(this).balance}();
    }

    fallback() external {
        // fallback of default account shouldn't be called by bootloader under no circumstances
        assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS);

        // If the contract is called directly, behave like an EOA
    }

    receive() external payable {
        // If the contract is called directly, behave like an EOA
    } 
}

contract Foo {
    function returnEther() public payable {
        payable(msg.sender).call{value: msg.value}("00");
    }
}

The above test will succeed by changing fallback() external to fallback() external payable.

Tools Used

None

Recommended Mitigation Steps

Change from fallback() external {...} to fallback() external payable {...} in DefaultAccount.

Lines of code

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

Vulnerability details

Impact

In https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/SystemContext.sol#L116 , require(_newTimestamp >= currentBlockTimestamp, "Timestamps should be incremental"); is incorrect. The correct condition is _newTimestamp > currentBlockTimestamp. This is as written in the Ethereum Yellow Paper as follows (this is also true in Ethereum when it was a PoW).

$H_s > P(H)_{H_S}$

($H_s$ is the timestamp of the current block and $P(H)_{H_s}$ is the timestamp of the parent block.)

Note that below that require statement is the following code.

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

However, the L1 contract does not verify the block timestamp condition as described below, so it must be verified in SystemContext.

// NOTE: We don't check that _newBlock.timestamp > _previousBlock.timestamp, it is checked inside the L2

In the specs and the bootloader code, some block parameters given by an operator (such as gas prices) are currently trusted, but the block timestamp is designed to be not. At the phase when operators are decentralized, this bug could result in unanticipated attacks on contracts deployed in zkSync Era and unexpected impacts on the zkSync ecosystem (including off-chain).

It can be said that allowing newTimestamp = currentBlockTimestamp can lead to a more severe timestamp dependency. Generally, contract developers should develop contracts cautiously, treating block.timestamp as an unreliable and unsafe source, as is well-known in here and here. However, they do not consider that "block.timestamp = previousBlockTimestamp can also occur when block.number > previouBlockNumber."

Therefore, applications/contracts that assume that block.timestamp > previousBlockTimestamp when block.number > previousBlockNumber will be broken. For example, it will not be possible to calculate something per unit of time. Executing foo / timeDiff with timeDiff = block.timestamp - previousBlockTimestamp will result in a division by zero error. The details are explained in Proof of Concept.

Proof of Concept

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/SystemContext.sol#L116 is incorrect. The following is a simple and impractical contract, but any DeFi and other contracts that accidentally contain such a mechanism will be attacked so that it cannot be executed forever.

contract Foo {
    uint256 prevBlockNumber;
    uint256 prevTimestamp;
    uint256 prev2Timestamp;

    function foo() public {
        require(prevBlockNumber < block.number);

        if (prevTimestamp > 0 && prev2Timestamp > 0) {
            uint256 timeDiff = prevTimestamp - prev2Timestamp;
            uint256 fooPerSecond = 1337 / timeDiff;

            // Something
        }

        prevBlockNumber = block.number;
        prev2Timestamp = prevTimestamp;
        prevTimestamp = block.timestamp;
    }
}

An attack scenario is as follows. This test uses the Foundry test because it seems to be not easy to set block numbers and block timestamps in the era-compiler-tester. However, this would also be true for zkSync Era.

pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Foo.sol";

contract FooAttackTest is Test {
    function test() public {
        Foo foo = new Foo();

        // normal operator
        vm.roll(1000);
        vm.warp(1000);
        foo.foo();

        // attacker
        vm.roll(1001);
        vm.warp(1000); 
        foo.foo();

        // normal operator
        vm.roll(1002);
        vm.warp(1020);
        foo.foo(); // divison by zero
    }
}

Tools Used

None

Recommended Mitigation Steps

Change the code as follows:

require(_newTimestamp > currentBlockTimestamp, "Timestamps should be incremental");

Table of Contents

• L-1: heapSize and auxHeapSize of ZkSyncMeta returned by getZkSyncMeta() are always 0
• L-2: Some variables are not cleaned up before call in inline assembler
• L-3: The amount of gas burned by precompileCall is slightly unfair depending on the amount of gasleft()
• L-4: No require in increaseMinNonce when nonceOrdering is Sequential
• NC-1: The baseFee description SystemContext is incorrect
• NC-2: The description of call flags in getCallFlags is incorrect
• NC-3: Unify whether to add override to interface functions
• NC-4: Whether or not _value > 0 should be checked in increaseMinNonce
• NC-5: Whether or not msg.value > 0 should be checked in withdraw of L2EthToken
• NC-6 (out of scope): CALL param 0 in extra_abi_data in VM specs is incorrect

L-1: heapSize and auxHeapSize of ZkSyncMeta returned by getZkSyncMeta() are always 0

In getZkSyncMeta() (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L198), heapSize and auxHeapSize are not getting.

    function getZkSyncMeta() internal view returns (ZkSyncMeta memory meta) {
        uint256 metaPacked = getZkSyncMetaBytes();
        meta.gasPerPubdataByte = getGasPerPubdataByteFromMeta(metaPacked);
        meta.shardId = getShardIdFromMeta(metaPacked);
        meta.callerShardId = getCallerShardIdFromMeta(metaPacked);
        meta.codeShardId = getCodeShardIdFromMeta(metaPacked);
    }

The following two lines must be added.

meta.heapSize = getHeapSizeFromMeta(metaPacked);
meta.auxHeapSize = getAuxHeapSizeFromMeta(metaPacked);

Fortunately, getZkSyncMeta is not used by any system contract and does not lead to direct attacks. In the future, it must be fixed as it may cause attacks or failures when getZkSyncMeta is used.

L-2: Some variables are not cleaned up before call in the inline assembler

The following cleanup is done for the function arguments that are less than 32 bytes. However, some variables are not cleaned up.

// Clearing values before usage in assembly, since Solidity doesn't do it by default
whoToMimic := and(whoToMimic, cleanupMask)

List of locations where cleanups appear:

• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L201
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L52
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L99
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L156
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L169

List of variables that are not cleaned up:

• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L131
  • _address is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L144
  • _address is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L159
  • _address is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L173
  • _address is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/EfficientCall.sol#L203
  • _address is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractsCaller.sol#L100
  • to is address type and not cleaned up.
• https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractsCaller.sol#L109
  • to is address type and not cleaned up.

However, in general, manual cleanup as above is not required for Ethereum because solc (not zksolc) cleans up function arguments that are stacked on the stack ([ref](https://docs.soliditylang.org/en/latest/ internals/variable_cleanup.html)). For example, if the following function with a variable of address type as an argument is compiled with solc and the mnemonics are checked, it can be confirm that it is cleaned up. The same is true for other types (uintN, bool, etc.).

contract A {
    function f(address x) public pure {}
}

The mnemonics:

...
0x050: PUSH20 0xffffffffffffffffffffffffffffffffffffffff
0x065: DUP3
0x066: AND
...

We can see that the mask is pushed onto the stack with the PUSH20 opcode and cleaned up with the AND opcode.

Hence, the cleanup processes present in the system contracts seem necessary for the zkEVM/zksolc specifications (registers, memory management, etc.). (If they are unnecessarily doing cleanup, they should be removed).

Therefore, depending on the zkEVM/zksolc specification, the variables listed earlier that are not cleaned up should be done. Also, if these do not need to be cleaned up, it would be better to clearly state in the comments why they do not need to be cleaned up.

L-3: The amount of gas burned by precompileCall is slightly unfair depending on the amount of gasleft()

In https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L152, it is more fair to put require(gasleft() >= _gasToBurn); after uint256 cleanupMask = UINT32_MASK;.

If gas is sufficient, the require statement consumes _gasToBurn + α no matter where it is placed. However, if gas is just barely enough to meet gasleft() >= _gasToBurn, then for example, if gasleft() is just _gasToBrun, it is lower because the execution cost of uint256 cleanupMask = UINT32_MASK; can be ignored.

L-4: No require in increaseMinNonce when nonceOrdering is Sequential

In setValueUnderNonce (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/NonceHolder.sol#L81), as follows, when nonceOrdering is sequential, the check exists to see if the previous nonce is used.

if (accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential && _key != 0) {
	require(isNonceUsed(msg.sender, _key - 1), "Previous nonce has not been used");
}

On the other hand, in increaseMinNonce (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/NonceHolder.sol#L64), since there is no check when nonceOrdering is sequential, it is possible to increase nonce by two or more despite sequential.

However, as the following statement on https://github.com/code-423n4/2023-03-zksync/blob/main/README.md?plain=1#L641 says, It might be better to remove the check on the setValueUnderNonce .

This ordering is not enforced in any way by system contracts, but it is more of a suggestion to the operator on how it should order the transactions in the mempool.

Anyway, it should be consistent whether nonceOrdering is checked or not.

NC-1: The baseFee description SystemContext is incorrect

At https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/SystemContext.sol#L43, they say that baseFee is a constant. On the other hand, https://github.com/code-423n4/2023-03-zksync/blob/main/README.md?plain=1#L327 says that baseFee is given by the operator and will be hard-coded (constant) in the future. In other words, it is currently dynamic, not constant.

Also, in bootloader.yul, it can be confirmed that baseFee is calculated dynamically and baseFee is not a constant, as shown below.

let baseFee, GAS_PRICE_PER_PUBDATA := getBaseFee(L1_GAS_PRICE, FAIR_L2_GAS_PRICE)

/// @dev Returns the baseFee for this block based on the
/// L1 gas price and the fair L2 gas price.
function getBaseFee(l1GasPrice, fairL2GasPrice) -> baseFee, gasPricePerPubdata {
	// By default, we want to provide the fair L2 gas price.
	// That it means that the operator controls
	// what the value of the baseFee will be. In the future, 
	// a better system, aided by EIP1559 should be added. 

	let pubdataBytePriceETH := safeMul(l1GasPrice, L1_GAS_PER_PUBDATA_BYTE(), "aoa")

	baseFee := max(
		fairL2GasPrice,
		ceilDiv(pubdataBytePriceETH, MAX_L2_GAS_PER_PUBDATA())
	)
	gasPricePerPubdata := ceilDiv(pubdataBytePriceETH, baseFee)
}

Therefore, the comment is incorrect and needs to be changed.

NC-2: The description of call flags in getCallFlags is incorrect

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L282 and https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/EventWriter.yul#L49 say Call flags are the value of the first register at the start of the call., but, as described in https://github.com/code-423n4/2023-03-zksync/blob/main/README.md?plain=1#L156 , call flags are in r2. Therefore the comment is incorrect and needs to be changed to the second register.

NC-3: Unify whether to add override to interface functions

Since Solidity 0.8.8, the override keyword is optional for interface functions (ref). The NonceHolder functions do not have it (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/NonceHolder.sol#L45), while the AccountCodeStorage functions do (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/AccountCodeStorage.sol#L34). It can be added or not, but it would be easier to understand if it is not added to all functions or added to all functions.

NC-4: Whether or not _value > 0 should be checked in increaseMinNonce

increaseMinNonce (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/NonceHolder.sol#L64) can be executed even if _value is 0. It should be clearly commented on whether it contains 0 or not.

NC-5: Whether or not msg.value > 0 should be checked in withdraw of L2EthToken

withdraw (https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L2EthToken.sol#L80 ) can be executed even if msg.value is 0. It should be clearly commented on whether it contains 0 or not.

NC-6 (out of scope): CALL param 0 in extra_abi_data in VM specs is incorrect

In https://github.com/code-423n4/2023-03-zksync/blob/main/docs/VM-specific_v1.3.0_opcodes_simulation.pdf, CALL param 0 (gas) in extra_abi_data is 0, but the correct value is an index.