[zk-sdk] Add unique domain separator for zk elgamal proof program for proofs#197
Conversation
joncinque
left a comment
There was a problem hiding this comment.
Looks great! I checked to make sure there are no more uses of Transcript::new anywhere (except in TranscriptProtocol)
| /// This string MUST be changed for any fork or separate deployment to prevent | ||
| /// cross-chain proof replay attacks. |
There was a problem hiding this comment.
nit: just so I'm sure I understand, we're not differentiating this for different networks, ie testnet vs devnet vs mainnet?
If we wanted to be pedantic, we may want to include the genesis hash in the domain, but this should be fine
There was a problem hiding this comment.
Yeah we encountered this issue for alpenglow BLS as well and I thought about this briefly. I think it adds a lot of extra complications if we differentiate between testnet, devnet, and mb. We probably need to add different features (in both solana-zk-sdk and agave) that has different hardcoded domain separator targeted at different networks and validators will need to deploy network specific builds of the zk-elgamal proof program.
I guess we can have the program directly look up the genesis hash inside the program and add it as an input to the verification function, but this probably adds another set of complications. I think keeping it uniform across different networks probably simplifies testing and maintenance.
There was a problem hiding this comment.
Yep that's fine, just wanted to make sure
|
Thanks for the review and let me merge this for now. If you feel strongly about anything related to the nit comment above, then I'll make sure to address it in a follow up 🙏 |
PaulLaux
left a comment
There was a problem hiding this comment.
This PR indeed resolves #b10 from the QEDIT audit.
Consider the following additions:
- The in-code documentation might not be visible enough. A mention in
README.mdorSECURITY.mdwill provide more visibility to this subject. - Consider personalization for testnet/devnet.
- Consider adding a WASM function to expose
TRANSCRIPT_DOMAIN.
Problem
All proof transcripts in the zk-sdk use hardcoded domain strings (e.g.,
b"ciphertext-ciphertext-equality-instruction",b"zero-ciphertext-instruction", etc.) without a shared chain-specific personalization prefix. Proofs generated on one blockchain can be replayed on any fork or chain using this code, as transcript domains are identical across all deploymentsSummary of Changes
I added a domain separation tag in the transcript by adding a concrete transcript constructor
Transcript::new_zk_elgamal_transcript(...)that first hashesb"solana-zk-elgamal-proof-program-v1"to the transcript before returning it. Then I updated all the instances ofTranscript::new(...)in the zk-sdk to useTranscript::new_zk_elgamal_transcript(...).The logic changes themselves should be pretty simple. But since we are adding new domain separations, I needed to update the hard-coded tests, which added on to the number of lines changed.