zkSync Era - Jeiwan's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/c857609bfdc41a0ee2c1b245217a785f66b42a56/code/system-contracts/contracts/Constants.sol#L35 https://github.com/code-423n4/2023-10-zksync/blob/c857609bfdc41a0ee2c1b245217a785f66b42a56/code/system-contracts/contracts/AccountCodeStorage.sol#L93-L95 https://github.com/code-423n4/2023-10-zksync/blob/c857609bfdc41a0ee2c1b245217a785f66b42a56/code/system-contracts/contracts/AccountCodeStorage.sol#L132

Vulnerability details

Impact

EXTCODEHASH and EXTCODESIZE return real values for ECADD and ECMUL precompiles, as well as for KECCAK256 and EVENT_WRITER precompiles. This breaks EIP-1052, which zkSync seeks to implement, and may cause incompatibility with the Ethereum VM. The severity is medium because zkSync tries to achieve the equivalence with EVM (where possible), and any unexpected discrepancy can cause incompatibility issues for projects running on zkSync.

Proof of Concept

The AccountCodeStorage.getCodeHash() function simulates the behavior of the extcodehash EVM opcode; the AccountCodeStorage.getCodeSize() function simulates the behavior of the extcodesize EVM opcode. As per their descriptions:

1. getCodeHash() returns:

   codeHash - hash of the bytecode according to the EIP-1052 specification.

2. getCodeSize():

   // NOTE: zero address and precompiles are a special case, they are contracts, but we // want to preserve EVM invariants (see EIP-1052 specification). That's why we automatically // return 0 length in the following cases: // - codehash(0) == 0 // - account is a precompile. // - account is currently being constructed

As per EIP-1052:

The EXTCODEHASH of an precompiled contract is either c5d246... or 0.

However, the two functions fail to correctly identify deployed precompiles. They both use the CURRENT_MAX_PRECOMPILE_ADDRESS constant, which is defined as (Constants.sol#L35):

uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));

However, there are new precompiles deployed after the SHA256 precompile (Constants.sol#L25-L28):

address constant ECRECOVER_SYSTEM_CONTRACT = address(0x01);
address constant SHA256_SYSTEM_CONTRACT = address(0x02);
address constant ECADD_SYSTEM_CONTRACT = address(0x06);
address constant ECMUL_SYSTEM_CONTRACT = address(0x07);

(Also, ECADD and ECMUL are said to be precompiles by the documentation.)

The CURRENT_MAX_PRECOMPILE_ADDRESS constant wasn't updated to include the new precompiles, although it has a note about that (Constants.sol#L30-L34):

/// @dev The current maximum deployed precompile address.
/// Note: currently only two precompiles are deployed:
/// 0x01 - ecrecover
/// 0x02 - sha256
/// Important! So the constant should be updated if more precompiles are deployed.

Here's a test case that demonstrates the issue:

// code/system-contracts/test/AccountCodeStorage.spec.ts
it.only('AUDIT undetected precompiles', async () => {
    await accountCodeStorage
        .connect(deployerAccount)
        .storeAccountConstructedCodeHash('0x0000000000000000000000000000000000000006', CONSTRUCTED_BYTECODE_HASH);
    await accountCodeStorage
        .connect(deployerAccount)
        .storeAccountConstructedCodeHash('0x0000000000000000000000000000000000000007', CONSTRUCTED_BYTECODE_HASH);

    // CORRECT:
    expect(await accountCodeStorage.getCodeSize('0x0000000000000000000000000000000000000001'))
        .to.be.eq(0);
    expect(await accountCodeStorage.getCodeSize('0x0000000000000000000000000000000000000002'))
        .to.be.eq(0);
    expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000001'))
        .to.be.eq("0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"); // hash of empty string
    expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000002'))
        .to.be.eq("0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"); // hash of empty string

    // WRONG:
    expect(await accountCodeStorage.getCodeSize('0x0000000000000000000000000000000000000006'))
        .to.be.eq(2097120);
    expect(await accountCodeStorage.getCodeSize('0x0000000000000000000000000000000000000007'))
        .to.be.eq(2097120);
    expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000006'))
        .to.be.eq("0x0100ffffdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
    expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000007'))
        .to.be.eq("0x0100ffffdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider updating the CURRENT_MAX_PRECOMPILE_ADDRESS constant to include the addresses of the ECADD and ECMUL precompiles.

The status of the KECCAK256 and EVENT_WRITER is not clear: while they're precompiles (they're stored in the "precompiles" folder), they're deployed in the system contracts address space (Constants.sol#L56, Constants.sol#L62). In case they are also treated as precompiles, consider improving the precompiles detection in the AccountCodeStorage.getCodeHash() and AccountCodeStorage.getCodeSize() functions so these addresses are also detected as precompiles (simply updating CURRENT_MAX_PRECOMPILE_ADDRESS might not work because there's a gap between the precompile addresses and the system contract addresses).

Assessed type

Other