Cleanup Verifier.sol and make error handling consistent

[recmo]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (yarn compile) was run locally and any changes were pushed
• Lint (yarn lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Currently when provided with an invalid proof, the verifier will either:

• return false,
• execute the invalid() opcode,
• revert with one of a variety of error messages, like
  • verifier-gte-snark-scalar-field
  • pairing-add-failed,
  • etc..

This makes handling invalid proofs hard. Returning false is especially dangerous because it puts the burden on the caller.

What is the new behavior?

When provided with an invalid proof, the verifier will:

• revert with InvalidProof()

In the unlikely event that the caller desires different behaviour, the caller can use a Solidity try/catch statement.

Does this introduce a breaking change?

• Yes
• No

The revert error message for an invalid proof changes.

Other information

[recmo]

My next move would be to remove the return value from _isValidProof, but at that point the name should also change. I want to check in and get maintainers opinion first.

Personally I prefer to revert on invalid proofs. This prevents errors where the result is accidentally ignored. (See for example also safeTransfer and why it exists) I don't think there are cases where callers would want any thing else, but if so they can always intercept the using try/catch.

[kobigurk]

Are you sure EIP196 verifies input is in the correct range?

Specifically:

[cedoor]

Hi! After the next Semaphore ceremony we will have more verifiers (> 10). Since this code is regenerated every time from SnarkJS templates I think it is better to create a new verifier_groth16.sol.ejs template with your changes.

[recmo]

@kobigurk Looks like you are partially right: field elements are verified, but scalars are not considered field elements.

[kobigurk]

@kobigurk Looks like you are partially right: field elements are verified, but scalars are not considered field elements.

Ah yes, exactly the distinction I was looking for. Super dangerous :)

[recmo]

Ok, so let's be conservative here and reject proofs where the input is not reduced. I'll add a commit.

[kobigurk]

Ok, so let's be conservative here and reject proofs where the input is not reduced. I'll add a commit.

Thanks! Btw, it's not only conservative, it's a direct attack vector - a16z/zkdrops#2

[recmo]

I made all the functions in Pairing strict on the input. Note that before Pairing.negate(..) would accept an unreduced Y coordinate, so there was malleability in Proof.A.

I don't know if this removes all proof malleability for people other than the prover. (i.e. given a valid proof but not the private inputs, can I compute a different also valid proof)

[recmo]

@cedoor I suspected there was some template involved, despite the usual warning comment missing. Thanks for the link, I will submit a PR there too.

I think we can still merge it here, the only thing that changes with a new ceremony is the contents of the verifyingKey() function, so that should be easy to update. The only other thing that can possibly change (assuming same proof system) is the number of inputs, which is also trivially changed in verifyProof().

[kobigurk]

I made all the functions in Pairing strict on the input. Note that before Pairing.negate(..) would accept an unreduced Y coordinate, so there was malleability in Proof.A.

I don't know if this removes all proof malleability for people other than the prover. (i.e. given a valid proof but not the private inputs, can I compute a different also valid proof)

It removes "public input" malleability, which is the dangerous part. Proofs can still be modified, because Groth16 is randomizable.

[cedoor]

Can't we remove this line? Updating the extension contracts should be sufficient to make this change compatible

[recmo]

Happy to do so! I left it to limit the scope of changes.

[cedoor]

Seems you need to run yarn prettier:fix to pass tests.