`Executor.sol` don't respect the EIP-4844

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol#L605-L619

Vulnerability details

Impact

Function Executor.sol#_pointEvaluationPrecompile() does not work as expected due to missing constraint checking for the returned result.

Proof of Concept

• According to EIP-4844, we can see that the returned result is 64 bytes, which is the sum of the first 32 bytes are Bytes(U256(FIELD_ELEMENTS_PER_BLOB).to_be_bytes32() and the last 32 bytes are U256(BLS_MODULUS).to_be_bytes32().

  def point_evaluation_precompile(input: Bytes) -> Bytes:
  ----SNIP----
  # Return FIELD_ELEMENTS_PER_BLOB and BLS_MODULUS as padded 32 byte big endian values
  return Bytes(U256(FIELD_ELEMENTS_PER_BLOB).to_be_bytes32() + U256(BLS_MODULUS).to_be_bytes32())

• Take a look at Executor.sol#_pointEvaluationPrecompile() : here

  File: Executor.sol
  601    /// @notice Calls the point evaluation precompile and verifies the output
  602    /// Verify p(z) = y given commitment that corresponds to the polynomial p(x) and a KZG proof.
  603    /// Also verify that the provided commitment matches the provided versioned_hash.
  604    ///
  605    function _pointEvaluationPrecompile(
  606        bytes32 _versionedHash,
  607        bytes32 _openingPoint,
  608        bytes calldata _openingValueCommitmentProof
  609    ) internal view {
  610        bytes memory precompileInput = abi.encodePacked(_versionedHash, _openingPoint, _openingValueCommitmentProof);
  611
  612        (bool success, bytes memory data) = POINT_EVALUATION_PRECOMPILE_ADDR.staticcall(precompileInput);
  613
  614        // We verify that the point evaluation precompile call was successful by testing the latter 32 bytes of the
  615        // response is equal to BLS_MODULUS as defined in https://eips.ethereum.org/EIPS/eip-4844#point-evaluation-precompile
  616        require(success, "failed to call point evaluation precompile");
  617        (, uint256 result) = abi.decode(data, (uint256, uint256));
  618        require(result == BLS_MODULUS, "precompile unexpected output");
  619    }

• On L612, after the data value is calculated there is no checking for the length of data = 64 .
• On L618, the value of result is only checked with BLS_MODULUS instead of being checked with FIELD_ELEMENTS_PER_BLOB and BLS_MODULUS .

• This can lead to unexpected precompilation results .

  Tools Used

  Manual review and https://eips.ethereum.org/EIPS/eip-4844#point-evaluation-precompile

  Recommended Mitigation Steps

  Consider change to :

  File: Executor.sol
  function _pointEvaluationPrecompile(
      bytes32 _versionedHash,
      bytes32 _openingPoint,
      bytes calldata _openingValueCommitmentProof
  ) internal view {
      bytes memory precompileInput = abi.encodePacked(_versionedHash, _openingPoint, _openingValueCommitmentProof);
  
      (bool success, bytes memory data) = POINT_EVALUATION_PRECOMPILE_ADDR.staticcall(precompileInput);
  
      // We verify that the point evaluation precompile call was successful by testing the latter 32 bytes of the
      // response is equal to BLS_MODULUS as defined in https://eips.ethereum.org/EIPS/eip-4844#point-evaluation-precompile
      require(success, "failed to call point evaluation precompile");
  +++        if (data.length != 64) revert INVALID_DATA ;
  ---        (, uint256 result) = abi.decode(data, (uint256, uint256));
  ---        require(result == BLS_MODULUS, "precompile unexpected output");
  +++        bytes32 first;
  +++        bytes32 second;
  +++        assembly {
  +++            first := mload(add(data, 32))
  +++            second := mload(add(data, 64))
  +++        }
  +++        if (uint256(first) != FIELD_ELEMENTS_PER_BLOB || uint256(second) != BLS_MODULUS) {
  +++            revert UNEXPECTED_OUTPUT();
      }
  }

• According to docs uint32 FIELD_ELEMENTS_PER_BLOB = 4096 .

Assessed type

Other

[c4-sponsor]

razzorsec (sponsor) disputed

[razzorsec]

We consider this issue invalid, since the precompile implementation in Geth fails on error or returns blobPrecompileReturnValue on success which is a 64bytes array of FIELD_ELEMENTS_PER_BLOB & BLS_MODULUS. A similar pattern was observed in other nodes.

[alex-ppg]

The Warden specifies that the EIP-4844 precompile invoked by the Executor::_pointEvaluationPrecompile function is insecurely done so due to not validating the output's length or both of the outputs expected.

The EIP-4844 definition itself does not instruct on how to validate calls to the precompile, it simply denotes how the precompiles should behave. As such, how a call to the precompile is validated is up to integrators.

The only return outlined in the precompile is the following:

# Return FIELD_ELEMENTS_PER_BLOB and BLS_MODULUS as padded 32 byte big endian values
return Bytes(U256(FIELD_ELEMENTS_PER_BLOB).to_be_bytes32() + U256(BLS_MODULUS).to_be_bytes32())

This represents a return of two constant variables that are 64 bytes in length total. By parsing the BLS_MODULUS argument as part of an ABI decode call as presently performed in the code (abi.decode(data, (uint256, uint256))), we are implicitly validating that the length is at least 64 bytes.

Whether both the FIELD_ELEMENTS_PER_BLOB and the BLS_MODULUS variables should be validated can be argued, and in this circumstance, I do not believe any flaw or vulnerability can actively arise from validating only the latter of the two.

I appreciate the Warden's due diligence, but can only consider the exhibit as a QA recommendation at this point based on the EIP's description.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c