search

clrfund / maci

Minimal anti collusion infrastructure
Other
5 stars 3 forks source link

Check verifier key from parameters match keys deployed on-chain #6

Closed auryn-macmillan closed 2 years ago

auryn-macmillan commented 2 years ago

Originally posted in upstream MACI repo, reposted here as the maintainers don't plan to fix issues with MACI versions prior to 1.0.

When running genProofs, it is possible to generate proofs using the wrong parameters, leading to a scenario where proveOnChain will fail. This leads to wasted time and resources computing results that ultimately must be re-computed using the correct parameters.

Ideally, we just use the correct parameters when we run genProofs. However, I believe it would be possible for genProofs to check that the verifier key in the parameters matches the verifier key on-chain prior to generating any proofs. If there is a mismatch, the process should stop and inform the user of the mismatch.

More concretely:

  1. The verifying key (in the zkey file) that is passed to setVks is stored on chain. This step happens very soon after the MACI contracts get deployed.
  2. genProofs should check whether the verifying key on chain matches the verifying key which can be derived from the parameters it uses. This will help the coordinator avoid making a mistake which could cost them lots of time and resources.
yuetloo commented 2 years ago

I've been looking into this a bit, one of the issues is that verifyingKey() is an internal function, can't access the function externally. I think even getStorageAt() doesn't work because it's a function, not a storage variable. And, this contract was generated using the zkutil tool by circom.

function verifyingKey() internal pure returns (VerifyingKey memory vk)

https://github.com/clrfund/monorepo/blob/feature/batch-64/contracts/contracts/snarkVerifiers/QuadVoteTallyVerifierBatch64.sol#L167

auryn-macmillan commented 2 years ago

Perhaps its worthwhile making that function public for future deployments, just so we don't need the verfied source to easily figure out the key.

For the current issue, we can know the key because you verfied the source code of the contract.

yuetloo commented 2 years ago

I've been checking the MACI 1.0 code and noticed there's checkVerifyingKey script in v1. https://github.com/privacy-scaling-explorations/maci/blob/v1/cli/ts/checkVerifyingKey.ts

I was thinking if we can confirm that v1 is ready for clr.fund, may be we should focus on migrating to v1 instead of dual maintaining the checkVerifyingKey script?

The other thing is that the gasEstimate check put in the genProofs script to validate the proof against smart contract, indirectly checks the VerifyingKey too. Is that good enough to close this issue? WDYT @auryn-macmillan ?

auryn-macmillan commented 2 years ago

@yuetloo, yeah I think those are both good points and I agree that the time is probably better spent migrating to MACI v1.0. I'll go ahead and close this issue.