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.

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

hotfix: workaround gnark 0.10.0 unsoundness bug #1756

Open
wants to merge 1 commit into
base: testnet
Choose a base branch
from

Conversation

Oppen
Copy link
Collaborator

@Oppen Oppen commented Jan 20, 2025

A lot of time passed since gnark v0.10.0 unsoundness bug0 was reported
and fixed. We posponed the upgrade because the fixed release, v0.11.0,
contains another vulnerability, an OOM1, for which a fix has been in
main since last November but no release appeared until now.

Our options here are limited, and none quite happy:

  • We can redirect to a commit in main;
  • We can disable groth16 verifiers from the network, which we currently
    use; or
  • We can enforce that proofs have only one commitment, as the
    unsoundness can only be triggered with multiple commitments per proof.

This implements the latter option, being the least invasive one.

Fixes #1749

Type of change

  • Bug fix

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

Testing

You'll need to compare the behavior of batcher and operators in both testnet and hotfix/gnark_workaround branches.

Download the attached file to your repo checkout and extract it with tar xf fake_proof.tgz. The contents are a witness, proof and verification key that should not be accepted by gnark but are, and they were generated with the following PoC. The PoC relies on a modified checkout of gnark, as explained in the document explaining the vulnerability.

Then apply this patch:

diff --git a/Makefile b/Makefile
index ad1ee3e7..4fc322dc 100644
--- a/Makefile
+++ b/Makefile
@@ -444,9 +444,9 @@ batcher_send_groth16_bn254_task: batcher/target/release/aligned
 	@echo "Sending Groth16Bn254 1!=0 task to Batcher..."
 	@cd batcher/aligned/ && cargo run --release -- submit \
 		--proving_system Groth16Bn254 \
-		--proof ../../scripts/test_files/gnark_groth16_bn254_infinite_script/infinite_proofs/ineq_1_groth16.proof \
-		--public_input ../../scripts/test_files/gnark_groth16_bn254_infinite_script/infinite_proofs/ineq_1_groth16.pub \
-		--vk ../../scripts/test_files/gnark_groth16_bn254_infinite_script/infinite_proofs/ineq_1_groth16.vk \
+		--proof ../../scripts/test_files/gnark_groth16_bn254_script/fake_groth16.proof \
+		--public_input ../../scripts/test_files/gnark_groth16_bn254_script/fake_groth16.pub \
+		--vk ../../scripts/test_files/gnark_groth16_bn254_script/fake_groth16.vk \
 		--proof_generator_addr 0x66f9664f97F2b50F62D13eA064982f936dE76657 \
 		--rpc_url $(RPC_URL) \
 		--network $(NETWORK)

Start a devnet and run:

make batcher_send_groth16_bn254_task

On testnet you should see a successful batch verification, as it's vulnerable to the unsoundness bug. On hotfix/gnark_workaround you should see both the batcher and operators (I suggest trying them separately with the rest of the stack running from testnet, to be able to test each of them) reject the proof, logging that it has too many commitments.

fake_proof.tgz

@Oppen Oppen force-pushed the hotfix/gnark_workaround branch from 84bca5e to 7b0dabc Compare January 20, 2025 17:24
@Oppen Oppen added audit cantina Audit report from Cantina labels Jan 20, 2025
A lot of time passed since gnark v0.10.0 unsoundness bug[0] was reported
and fixed. We posponed the upgrade because the fixed release, v0.11.0,
contains another vulnerability, an OOM[1], for which a fix has been in
main since last November but no release appeared until now.

Our options here are limited, and none quite happy:
- We can `redirect` to a commit in `main`;
- We can disable groth16 verifiers from the network, which we currently
  use; or
- We can enforce that proofs have only one commitment, as the
  unsoundness can only be triggered with multiple commitments per proof.

This implements the latter option, being the least invasive one.

[0]: https://www.zellic.io/blog/gnark-bug-groth16-commitments
[1]: GHSA-cph5-3pgr-c82g
@Oppen Oppen force-pushed the hotfix/gnark_workaround branch from 7b0dabc to 9d92813 Compare January 20, 2025 18:51
Copy link
Collaborator

@JuArce JuArce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is missing to specify that limitation in the docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit cantina Audit report from Cantina
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: unsoundness risk in gnark 0.10
3 participants