Inaccurate Batch Stored Hash Retrieval in Getters Contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Getters.sol#L90

Vulnerability details

Impact

The storedBatchHash function's inability to distinguish between reverted and unreverted batches may mislead users and developers about batch statuses in zkSync.

Proof of Concept

You can retrieve the stored batch hash by invoking the storedBatchHash(uint256 _batchNumber) function found within the Getters contract.
https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Getters.sol#L90

It returns zero for uncommitted batch numbers, but it lacks the capability to distinguish between a reverted and an unreverted batch. For example, if batch number 100 has been reverted, and a user queries storedBatchHash(100), it returns a non-zero value, which may create the impression that the batch is unreverted. However, it should ideally return zero in such scenarios.

Tools Used

Recommended Mitigation Steps

The function should be revised as:

function storedBatchHash(uint256 _batchNumber) external view returns (bytes32) {
        if(_batchNumber > s.totalBatchesCommitted){
            return bytes32(0);
        }
        return s.storedBatchHashes[_batchNumber];
    }

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[miladpiri]

QA.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Agree with QA Refactoring

[HE1M]

@GalloDaSballo Thanks for your judgments.

This problem demonstrates a discrepancy with the documentation and an inaccurate implementation. Therefore, I propose categorizing it as having a medium severity level.

For better comprehension, let's examine a scenario involving two functions: getTotalBatchesCommitted and storedBatchHash:

Case 1:
If batch number 100 is committed, then:

• Invoking getTotalBatchesCommitted returns 100.
• Invoking storedBatchHash(100) returns the stored hash.

This aligns with the expected behavior outlined here.

Case 2:
If only batch 100 is reverted, then:

• Calling getTotalBatchesCommitted returns 99.
• Calling storedBatchHash(100) returns the same stored hash before being reverted, as reverting a batch does not clear the stored hash.

This deviates from the expected behavior documented here. The documentation asserts that for an uncommitted batch, storedBatchHash(_batchNumber) should return zero. Since getTotalBatchesCommitted returns 99, indicating that batch 100 is uncommitted, it is expected that storedBatchHash(100) returns zero. However, it incorrectly returns the same stored hash of batch 100 before being reverted.

Case 3:
If batch 100 is committed again, then:

• Invoking getTotalBatchesCommitted returns 100.
• Invoking storedBatchHash(100) returns a different stored hash.

This aligns with the anticipated behavior as detailed here, where the stored hash is expected to change for unexecuted batches.

This issue can directly impact third parties relying on the state of batches. Furthermore, I contend that the impact of this problem is nearly equivalent to issue #782, which is classified as medium severity. Therefore, it is fair for this issue to be treated similarly.

[GalloDaSballo]

After discussing with another judge, I am downgrading this type of "view findings" as QA