Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QUANTITY fields check of Execution Payload is too relaxed #3844

Open
marioevz opened this issue Jul 6, 2022 · 2 comments
Open

QUANTITY fields check of Execution Payload is too relaxed #3844

marioevz opened this issue Jul 6, 2022 · 2 comments

Comments

@marioevz
Copy link

marioevz commented Jul 6, 2022

Describe the bug
The QUANTITY fields of the ExecutionPayloadV1 in the Engine API (https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md) must comply to the following definition:

Values of a field of QUANTITY type MUST be encoded as a hexadecimal string with a 0x prefix and the leading 0s stripped (except for the case of encoding the value 0) matching the regular expression ^0x(?:0|(?:[a-fA-F1-9][a-fA-F0-9]*))$.

Note: Byte order of encoded value having QUANTITY type is big-endian.

Currently Nimbus is allowing some Quantity types pass unchecked from the execution client in test invalid-quantity-fields of the eth2/engine simulator in hive.

The test verifies fields blockNumber (64 bitsize), gasLimit (64 bitsize), gasUsed (64 bitsize), timestamp (64 bitsize), baseFeePerGas (256 bitsize), with the following invalidations:

  • Overflow: Insert extra zeros left-padding in the hex string such that the bitsize overflows.
  • LeadingZero: Insert an extra zero so that the quantity hex string has an invalid leading zero (0x1 -> 0x01)
  • Empty: Send 0x string
  • NoPrefix: Strip the 0x from the valid hex string (0x1234 -> 1234)

To Reproduce
Use branch https://github.com/marioevz/hive/tree/all-tests-plus-cl-clients and run

./hive --client go-ethereum,nimbus-bn,nimbus-vc --sim eth2/engine --sim.loglevel 5
@tersec
Copy link
Contributor

tersec commented Jul 7, 2022

status-im/nim-web3#53 addresses this.

@tersec
Copy link
Contributor

tersec commented Jul 8, 2022

#3850 points out some spec points around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants