Skip to content

fix: ECDSA verify should not panic when signature not reduced #1458

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

Merged
merged 3 commits into from
Mar 15, 2025

Conversation

jonathanpwang
Copy link
Contributor

Finding by @lispc

Description of Fix

  • (style) rename previous IntMod::assert_unique to IntMod::assert_reduced and move the implementation using iseqmod into macro since it's specific to the use of special intrinsic.
  • Add new is_reduced() -> bool that checks if an integer representation is less than the modulus. Does simple byte-wise comparison check.
  • Update ECDSA verify and recover functions so it returns Error when r or s are not in [1,n].

Closes INT-3517

Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-16 [-1.2%]) 1,269 334,136 17,905,035 - - -
fibonacci_program (-40 [-1.5%]) 2,651 1,500,096 51,485,167 - - -
regex_program (+11 [+0.1%]) 7,770 4,140,164 167,389,450 - - -
ecrecover_program (-7 [-0.5%]) 1,437 295,291 15,588,656 - - -
pairing (+29 [+0.6%]) 4,717 1,711,640 92,585,975 - - -

Commit: aae214a

Benchmark Workflow

@jonathanpwang jonathanpwang merged commit d1321b1 into main Mar 15, 2025
17 checks passed
@jonathanpwang jonathanpwang deleted the fix/ecdsa-lt-check branch March 15, 2025 20:17
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

Successfully merging this pull request may close these issues.

2 participants