Incorrect max precompile address

mediumCode4rena

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Constants.sol#L27-L28 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Constants.sol#L35 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/AccountCodeStorage.sol#L89-L95

Vulnerability details

Summary

Two new precompiles have been introduced (ECADD and ECMUL) without updating the pointer that indicates the current maximum precompile address (CURRENT_MAX_PRECOMPILE_ADDRESS), causing these new precompiles to not be considered as precompiles in the system.

Impact

The updated revision of ZkSync Era has introduced two new precompiles, ECADD and ECMUL at addresses 0x06 and 0x07, respectively.

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Constants.sol#L27-L28

solidity📋
27: address constant ECADD_SYSTEM_CONTRACT = address(0x06);
28: address constant ECMUL_SYSTEM_CONTRACT = address(0x07);

Their implementation is available in code/system-contracts/contracts/precompiles/EcAdd.yul and code/system-contracts/contracts/precompiles/EcMul.yul.

Precompile addresses are tracked by a constant named CURRENT_MAX_PRECOMPILE_ADDRESS, which points to the maximum address that is a precompile. The intention here is to have a way to determine if an address is a precompile, i.e. precompile <=> address <= CURRENT_MAX_PRECOMPILE_ADDRESS.

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Constants.sol#L35

solidity📋
35: uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));

Note that the implementation still points to the old maximum precompile, which was SHA256 at address 0x02.

This means that both ECADD and ECMUL won't be considered as precompiles, this is because their address are greater than the CURRENT_MAX_PRECOMPILE_ADDRESS, breaking the invariant.

Particularly, this impacts the getCodeHash() function in AccountCodeStorage. The function follows the assumption that a precompile is under the CURRENT_MAX_PRECOMPILE_ADDRESS constant in order to return the hash of the empty string (EMPTY_STRING_KECCAK).

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

solidity📋
89:     function getCodeHash(uint256 _input) external view override returns (bytes32) {
90:         // We consider the account bytecode hash of the last 20 bytes of the input, because
91:         // according to the spec "If EXTCODEHASH of A is X, then EXTCODEHASH of A + 2**160 is X".
92:         address account = address(uint160(_input));
93:         if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS) {
94:             return EMPTY_STRING_KECCAK;
95:         }
...

As both ECADD and ECMUL are greater than CURRENT_MAX_PRECOMPILE_ADDRESS, the implementation of getCodeHash() will return zero instead of the expected EMPTY_STRING_KECCAK constant, as other precompiles do.

Proof of Concept

The following test asks the AccountCodeStorage contract for the code hash of the new precompiles. The expected value should be the keccak hash of the empty string.

typescript📋
describe('AccountCodeStorage', function() {
    it('fails to return correct hash for ECADD precompile', async () => {
        expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000006')).to.be.eq(
            EMPTY_STRING_KECCAK
        );
    });
    
    it('fails to return correct hash for ECMUL precompile', async () => {
        expect(await accountCodeStorage.getCodeHash('0x0000000000000000000000000000000000000007')).to.be.eq(
            EMPTY_STRING_KECCAK
        );
    });
});

The expected results from these tests fail:

  Audit tests
    AccountCodeStorage
      1) fails to return correct hash for ECADD precompile
      2) fails to return correct hash for ECMUL precompile


  0 passing (256ms)
  2 failing

  1) Audit tests
       AccountCodeStorage
         fails to return correct hash for ECADD precompile:

      AssertionError: expected '0x00000000000000000000000000000000000…' to equal '0xc5d2460186f7233c927e7db2dcc703c0e50…'
      + expected - actual

      -0x0000000000000000000000000000000000000000000000000000000000000000
      +0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470

      at Context.<anonymous> (test/Audit.spec.ts:33:110)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

  2) Audit tests
       AccountCodeStorage
         fails to return correct hash for ECMUL precompile:

      AssertionError: expected '0x00000000000000000000000000000000000…' to equal '0xc5d2460186f7233c927e7db2dcc703c0e50…'
      + expected - actual

      -0x0000000000000000000000000000000000000000000000000000000000000000
      +0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470

      at Context.<anonymous> (test/Audit.spec.ts:39:110)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

Recommendation

Update the CURRENT_MAX_PRECOMPILE_ADDRESS pointer to include the new precompile addresses.

diff📋
-    uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));
+    uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(ECMUL_SYSTEM_CONTRACT));

Assessed type

Other