Divergences in the Simulation of the extcodehash EVM Opcode

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L89

Vulnerability details

Impact

The divergences in the emulation of the extcodehash EVM opcode within zkSync Era carry several potential impacts, specially on Developers relying on zkSync Era's assurance that it emulates the extcodehash opcode as per EIP-1052 might encounter unexpected behavior in their smart contracts.

Proof of Concept

The getCodeHash function within the zkSync Era is designed to emulate the functionality of the extcodehash Ethereum Virtual Machine (EVM) opcode, as laid out in the Ethereum Improvement Proposal 1052 (EIP-1052).
https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L89

However, it's important to recognize that this function does not exhibit precisely identical behavior to the EVM's extcodehash opcode. There are specific discrepancies:

1. EIP-161 defines an account as "empty" when it satisfies certain criteria: no code, a nonce value of zero, and a balance of zero. According to EIP-1052, the extcodehash of an empty account should evaluate to bytes32(0). In the zkSync Era, the behavior aligns with this definition when codeHash is 0x00, getRawNonce(account) is 0, and isContractConstructing(codeHash) is false. If an account has nonzero balance, then based on the definition of EIP-161, it will not be considered as an empty account anymore. In this case, that account will be considered as an account with no code. So, extcodehash of such account will be keccak256("") in EVM.
   The issue is that, zkSync Era returns bytes32(0) regardless of the balance of the account. It only cares about the nonce and the code.

2. Based on EIP-1052, the extcodehash of an precompile contract is either keccak256("") or bytes32(0). For instance, the extcodehash of address(0x02)—representing the SHA-256 precompile contract in the EVM—should be keccak256("") because it has no code, a nonce of zero, and a nonzero balance. In contrast, the zkSync Era consistently returns keccak256("") for precompile contracts, regardless of their balances. The zkSync Era's behavior is based solely on whether the address is lower/equal to CURRENT_MAX_PRECOMPILE_ADDRESS.

These observed inconsistencies could potentially raise concerns for developers who rely on zkSync Era's assertion that it accurately simulates the extcodehash EVM opcode as dictated by the EIP-1051 specification.

Tools Used

Recommended Mitigation Steps

The following code is recommended to simulate the extcodehash EVM opcode precisely based on EIP-1052.

function getCodeHash(uint256 _input) external view override returns (bytes32) {

        address account = address(uint160(_input));
        if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS && account.balance != 0) {
            return EMPTY_STRING_KECCAK;
        } else if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS && address(account).balance == 0) {
            return bytes32(0);
        }

        bytes32 codeHash = getRawCodeHash(account);

        if (codeHash == 0x00 && NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(account) > 0) {
            codeHash = EMPTY_STRING_KECCAK;
        }
        else if (Utils.isContractConstructing(codeHash)) {
            codeHash = EMPTY_STRING_KECCAK;
        } else if (codeHash == 0x00 && NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(account) == 0 && address(account).balance != 0) {
            codeHash = EMPTY_STRING_KECCAK;
        }

        return codeHash;
    }

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L89

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[miladpiri]

The impact is medium, the probablity is low.

So, low severity can be fair.

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[GalloDaSballo]

While I agree with low impact, this seem to be an inconsistency in the implementation of the EVM, which makes the finding notable

[GalloDaSballo]

I would have a different perspective for an ERC or some implementation, however in this case this is an opcode that behaves differently (although in a edge case)

[GalloDaSballo]

At this time, I believe Medium Severity is most appropriate as the finding demonstrates an inconsistent behaviour of the EVM with the zkSyncVM

I agree with the impact, but believe that such finding belongs to the Medium Severity classification as it breaks a coding convention that goes beyond a best-practice

[c4-judge]

GalloDaSballo marked the issue as selected for report