fix: Ecdsa Malleability Bug

[suyash67]

Description

ECDSA has an inherent malleability because of the way $s$ in an ECDSA signature is computed. Read more about this here. This PR adds the constraint $s < |Fr|/2$ in ECDSA verification to fix the malleability. Additionally, in the signature construction, we make sure that $s < |Fr|/2$.

Acknowledgements: @Rumata888 corrected the constraint on $s$ in verification.

Checklist:

• I have reviewed my diff in github, line by line.
• Every change is related to the PR description.
• I have linked this pull request to the issue(s) that it resolves.
• There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
• There are no circuit changes, OR specifications in /markdown/specs have been updated.
• There are no circuit changes, OR a cryptographer has been assigned for review.
• I've updated any terraform that needs updating (e.g. environment variables) for deployment.
• The branch has been rebased against the head of its merge target.
• I'm happy for the PR to be merged at the reviewer's next convenience.
• New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
• If existing code has been modified, such documentation has been added or updated.

[Rumata888]

I've made two changes:

1. The function was actually less or equal.
2. Once I changed it to be less_than, the logic for constraining s became incorrect. You were checking that s < (Fr::modulus / 2). It is different from 2*s < Fr::modulus, because Fr::modulus is odd. Think mod 3. If you check that something is less than 3/2, then the only option is zero, while 1 should also be a legitimate value.

[kevaundray]

nit, but not a blocker: unit tests should ideally not contain random elements as they then become unreproducable

[Rumata888]

You are partially right. I'd add something like "random_element_for_test" into the field, so that it would automatically print it. But it is actually better to have random elements than static, because if there is a bug there is a chance that it triggers. And then you can stress the test to find it (that was the case with construct_addition_chains)

[kevaundray]

Looks good to me

[kevaundray]

Since this modifies the stdlib by adding extra constraints, I'm marking this a breaking change

[kevaundray]

Update: It was decided that changes to the stdlib will not be marked as breaking changes as this would include most commits

[suyash67]

I've made two changes:

1. The function was actually less or equal.
2. Once I changed it to be less_than, the logic for constraining s became incorrect. You were checking that s < (Fr::modulus / 2). It is different from 2*s < Fr::modulus, because Fr::modulus is odd. Think mod 3. If you check that something is less than 3/2, then the only option is zero, while 1 should also be a legitimate value.

Nice catch, thanks Kesha!