-
Notifications
You must be signed in to change notification settings - Fork 21
zkASM RLPAUTH #853
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
base: master
Are you sure you want to change the base?
zkASM RLPAUTH #853
Conversation
| ;; Divide SECP256K1N by 2 | ||
| var secp256k1n_divided_by_two u256 | ||
| var b u1 | ||
| secp256k1n_divided_by_two, b = SECP256K1N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing division operation for SECP256K1N comparison
The comment says "Divide SECP256K1N by 2" but the code secp256k1n_divided_by_two, b = SECP256K1N just assigns the full constant to the variable without any division. The variable name implies it should contain half the value, but it actually contains the full SECP256K1N constant. This causes the s-malleability check to use the wrong threshold.
| ;; Check that S <= SECP256K1N / 2 | ||
| var tmp u256 | ||
| b, tmp = S - secp256k1n_divided_by_two | ||
| if b == 1 goto failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inverted logic for S malleability check comparison
The check if b == 1 goto failure has inverted logic. When computing S - secp256k1n_divided_by_two, a borrow (b=1) indicates S < threshold, which per EIP-7702 should be the valid case (S must be at most N/2). The code incorrectly treats the valid case as failure and allows invalid signatures where S exceeds the threshold.
|
|
||
| ;; https://eips.ethereum.org/EIPS/eip-7702 | ||
|
|
||
| const SECP256K1N = 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrong constant used: field prime instead of curve order
The constant SECP256K1N is set to 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f which is the secp256k1 field prime (p), not the curve order (n). The correct curve order for signature malleability checks is 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141. Using the field prime instead of the curve order makes the s-malleability check ineffective as it allows larger S values than permitted by EIP-7702.
| ;; https://eips.ethereum.org/EIPS/eip-7702 | ||
|
|
||
| ;; TODO: here we can directly have secp256k1n_divided_by_two | ||
| const SECP256K1N = 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrong cryptographic constant used for signature validation
The SECP256K1N constant is set to the secp256k1 field prime p (0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F) instead of the curve order n (0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141). For EIP-7702 signature validation, the check S <= secp256k1n/2 must use the curve order n. Since p > n, using p makes the check too permissive and would incorrectly accept signatures with invalid S values between n/2 and p/2.
| ;; Divide SECP256K1N by 2 | ||
| var secp256k1n_divided_by_two u256 | ||
| var b u1 | ||
| secp256k1n_divided_by_two, b = SECP256K1N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing division operation for secp256k1n half calculation
The comment states "Divide SECP256K1N by 2" but the line secp256k1n_divided_by_two, b = SECP256K1N simply assigns the full constant without performing any division or right shift. The variable secp256k1n_divided_by_two will contain the full value, not half of it, causing the subsequent S value check to use an incorrect threshold.
| ;; Check that S <= SECP256K1N / 2 | ||
| var tmp u256 | ||
| b, tmp = S - secp256k1n_divided_by_two | ||
| if b == 1 goto failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inverted borrow check logic for S validation
The condition if b == 1 goto failure fails when the subtraction S - secp256k1n_divided_by_two causes a borrow (underflow), which happens when S < secp256k1n/2. However, the EIP-7702 requirement is that S <= secp256k1n/2, meaning the code should fail when S > secp256k1n/2 (no borrow). The logic is inverted and will reject valid signatures while accepting invalid ones.
| step_1: | ||
| if CHAIN_ID == 0 goto step_3 | ||
| if CHAIN_ID == 1 goto step_3 | ||
| fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hardcoded chain ID validation enables cross-chain replay
The chain ID validation hardcodes acceptance of CHAIN_ID == 1 (mainnet) regardless of which chain the code actually runs on. Per EIP-7702, authorizations with a non-zero chain_id must only be valid when chain_id matches the current execution chain. If this code runs on a non-mainnet chain, it would incorrectly accept mainnet authorizations (enabling cross-chain replay attacks) while rejecting valid authorizations for the actual chain. The function needs the current chain ID as a parameter to properly validate.
|
|
||
| ;; TODO: do we have an shared notation for constants, function arguments...? | ||
| ;; TODO: here we can directly have secp256k1n_divided_by_two | ||
| const SECP256K1N = 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrong secp256k1 constant uses field prime instead of curve order
The SECP256K1N constant is set to the secp256k1 field prime p (0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f = 2^256 - 2^32 - 977) instead of the curve order n (0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141). This is a critical cryptographic error because the s-value malleability check in ECDSA signatures requires comparison against the curve order n, not the field prime p. Using the wrong value would allow invalid signatures to pass validation.
| ;; Divide SECP256K1N by 2 | ||
| var secp256k1n_divided_by_two u256 | ||
| var b u1 | ||
| secp256k1n_divided_by_two, b = SECP256K1N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Division by two not actually performed
The comment states "Divide SECP256K1N by 2" but the code secp256k1n_divided_by_two, b = SECP256K1N does not perform division. In zkASM, this syntax performs type decomposition, not arithmetic. To properly compute N/2, a right-shift operation (like bit_shr256) would be needed. The variable secp256k1n_divided_by_two receives an incorrect value, causing the subsequent s <= N/2 malleability check to use the wrong threshold.
| ;; Check that s <= SECP256K1N / 2 | ||
| var tmp u256 | ||
| b, tmp = s - secp256k1n_divided_by_two | ||
| if b == 1 goto failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: S-value comparison logic is inverted
The s-value malleability check logic is inverted. The code b, tmp = s - secp256k1n_divided_by_two followed by if b == 1 goto failure rejects values where s < N/2 (borrow occurs) and accepts values where s >= N/2 (no borrow). This is backwards - EIP-7702 requires rejecting signatures where s > N/2 to prevent signature malleability. The condition should fail when b == 0 (no borrow, meaning s >= N/2) instead of when b == 1.
| ;; dummy values | ||
| authority = 0 | ||
| error = 0 | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Computed authority overwritten before returning
Control flow falls through to exit: unconditionally, where authority is reassigned to 0 just before return. This makes rlpauth always return a zero authority (and error = 0) even when earlier steps computed a nonzero result, breaking RLPAUTH behavior.
Note
Adds RLP auth zkASM module per EIP-7702 and integrates it into Prague and Osaka binaries, with basic helper funcs and tests.
rlpauth/rlpauth.zkasmimplementingrlpauth(...)flow (chainId check,s-range check, RLP build, magic-prefix concat,keccak,ecrecover), with placeholder helper functions.RLP_AUTH := rlpauth/rlpauth.zkasminMakefile.RLP_AUTHinZKEVM_MODULES_PRAGUEandZKEVM_MODULES_OSAKA.rlpauth/test/compute_rlp.jsonandrlpauth/test/compute_concat_magic_and_rlp.jsonfor helper functions.Written by Cursor Bugbot for commit a96f719. This will update automatically on new commits. Configure here.