-
Notifications
You must be signed in to change notification settings - Fork 237
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
fix(security): verifier contract range limit checks #43
Conversation
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.
Thanks for the fix! Glad to see the boundary checking in place. Could you add some tests to make sure we don't loose this in some refactoring/regression?
a3aadcf
to
2c60575
Compare
6ecfad5
to
5861b6d
Compare
46d0a47
to
3e1cdf4
Compare
test cases updated |
06317b8
to
34c90c1
Compare
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.
All tests pass for me
Description
Following an issue raised by @weijiekoh this PR checks for the range limits for proof and public inputs to the
verify()
inVerifier.sol
. Although there are parameter in theradish34/api/src/utils/crypto/ecc/babyjubjubparams.js
that are utilized in signature verification, there is currently no additional check against the inputs that any user could enter while interacting directly with the deployedVerifier
contractRelated Issue
#34
Motivation and Context
Test data for the
Verifier.test.js
is based on running/debugging the integration testsemaphore-protocol/semaphore#16
https://github.com/appliedzkp/semaphore/blob/master/contracts/sol/verifier.sol#L199
How Has This Been Tested
All tests at the root level as indicated in
npm run test
pass. New tests at radish34/contracts/ level also pass.Screenshots (if appropriate)
Types of changes
Checklist