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.

Sign up for GitHub

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

Jump to bottom

Potential security bug with the zk-SNARK verifier #34

Closed
weijiekoh opened this issue Mar 21, 2020 · 2 comments · Fixed by #43
Closed

Potential security bug with the zk-SNARK verifier #34

weijiekoh opened this issue Mar 21, 2020 · 2 comments · Fixed by #43
Assignees
@skarred14
Labels
👮‍♀️ security Security issue or potential vulnerability

Comments

@weijiekoh
Copy link

Expected Behavior

The Verifier.verify() function, not the function that calls it (i.e. Shield.createMSA() and Shield.createPO(), should require that each public input to the snark is less than the scalar field:

21888242871839275222246405745257275088548364400416034343698204186575808495617

Actual Behavior

While the Shield.createMSA() and Shield.createPO() functions may not be vulnerable due to the way they hash some variables and check if the single public input matches the hash, other circuits in the future may be vulnerable if the developer does not do the required check.

To avoid this problem entirely, perform the check in the Verifier.verify() function. This bugfix should probably be done upstream in the Nightfall repository.

See also:

semaphore-protocol/semaphore#16

https://github.com/EYBlockchain/nightfall/pull/96
@skarred14 skarred14 added the 👮‍♀️ security Security issue or potential vulnerability label Mar 21, 2020
@skarred14
Copy link
Collaborator

skarred14 commented Mar 23, 2020 •
edited
Loading

@weijiekoh thanks for the issue. Going over the linked issues, it looks like one of the ways this could be addressed is by validating all inputs in inputs[] are checked against the zk snark scalar field, perhaps something like this in verificationCalculation function in Verifier.sol:
require(input[i] < 21888242871839275222246405745257275088548364400416034343698204186575808495617). Thoughts/comments?
To your point, it would be good to have this as an upstream change even for the export-verifier functionality in Zokrates to address this range limit

@weijiekoh
Copy link
Author

weijiekoh commented Mar 24, 2020 •
edited
Loading

Yes, sounds good!

I also recommend range-checking the proof elements as is done here:

https://github.com/appliedzkp/semaphore/blob/master/contracts/sol/verifier.sol#L199

@skarred14 skarred14 mentioned this issue Mar 25, 2020
Merged
8 tasks
@skarred14 skarred14 self-assigned this Mar 25, 2020
@skarred14 skarred14 linked a pull request Apr 1, 2020 that will close this issue
fix(security): verifier contract range limit checks #43
Merged
8 tasks
gitbook-com bot pushed a commit that referenced this issue Apr 6, 2022
@gitbook-bot
GitBook: [#34] No subject
aeec9af
fosgate29 pushed a commit that referenced this issue Jun 19, 2022
@markymark-26 @gitbook-bot @Kasshern
Docs Update (#472)
aeede4f 
* Initial commit

* GitBook: [#27] grammar updates

* GitBook: [#28] No subject

* GitBook: [#29] formatting change

* GitBook: [#30] grammar change

* GitBook: [#31] formatting change

* GitBook: [#32] complete page of open source community functions, removing all duplications

* GitBook: [#33] No subject

* GitBook: [#34] No subject

* GitBook: [#35] No subject

* GitBook: [#36] No subject

* GitBook: [#39] costmic fix

* GitBook: [#40] No subject

* Changes file name of original baseline example png and replaces original with new image

* Fixes radish34 explained link

* renames high level baselline architecture and replaces linked high level architecture with new image

* fixes italicized bolded phrase md

* renames bri2 stack pngs and updates bri-2 to point to those photos

* Comments API package under source coude because it is a duplicate of the baseline package (alread on npm)

* Changes persistence package md file name to identity - same change to summary.md file

* Removes the duplicated org registry section from CCSM package and leaves it in identity package

* Changes name of api package file to baseline (matching what shows on gitbook)

* Changes package file name from api-1 to api in order to match gitbook

* Changes file incorrectly name vaults to messaging to match its content in packages

* adds vaults section to packages in summary md  and correpsonding file

* Changes radish link from google sheets connector file

* testing radish link

* Changes radish link in connector to relative gitbook link

* Changes the baseline protocol link to a relative one in connectors files

* Added dynamics, sheets, and sequence-diagram pngs to docs folder and fixed their reference in the erp connectors files

* Changes the reference of image5 for the shuttle microservice container

* GitBook: [#42] delete general assembly page as it is not governed as a separate body

* GitBook: [#44] No subject

* GitBook: [#45] community leaders updated

* GitBook: [#46] No subject

* GitBook: [#48] formatting updates on governance, info added to this page from other pages that belonged here.

* GitBook: [#50] changes to community meeting docs

* GitBook: [#51] changes to community pages

* GitBook: [#52] formatting change

* GitBook: [#53] updated terms on organzation

* GitBook: [#55] No subject

* GitBook: [#56] updating contributing title

* GitBook: [#57] No subject

* GitBook: [#65] Remove asterisks from welcome

* GitBook: [#66] No subject

* GitBook: [#67] No subject

* GitBook: [#69] BASEIcs talk added

* GitBook: [#72] No subject

* GitBook: [#74] Misc. Grammar & Spelliing

* GitBook: [#76] No subject

* GitBook: [#78] Relinquishing Core Dev Status

Co-authored-by: gitbook-bot <ghost@gitbook.com>
Co-authored-by: sonal.patel <sonal.patel@mesh.xyz>
Co-authored-by: Kasshern <keith.salzman@protonmail.com>
Co-authored-by: mark.rymsza <mark.rymsza@mesh.xyz>
@veorq veorq mentioned this issue Mar 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@skarred14 skarred14

Labels
👮‍♀️ security Security issue or potential vulnerability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(security): verifier contract range limit checks ethereum-oasis-op/baseline
2 participants
@skarred14 @weijiekoh