Double spending threat if no range check is done on public inputs in verifier

[AntoineRondelet]

We need to make sure that we constrain the public inputs to be <p in the verifier contract.
See: semaphore-protocol/semaphore#16

[poma]

Looks like zeth is vulnerable, the check should be added here

zeth/zeth-contracts/contracts/Groth16Verifier.sol

Line 115 in e0b4ed4

inputValues[i] = primaryInputs[i];

[rrtoledo]

I propose to check the nullifiers inside BaseMixer.sol so that it is not proof dependent.

zeth/zeth-contracts/contracts/BaseMixer.sol

Line 113 in e0b4ed4

require(

Also why do we add the nullifiers before checking the proof passes ( "nullifiers[current_nullifier] = true;" ) ? This could lead to a DoS attack.

I do not think there are attacks on the range of the other primary inputs. That being said, I would rather have a dedicated function to assemble and check all inputs before doing any verification (since the assemble values are used in several places).

[AntoineRondelet]

Also why do we add the nullifiers before checking the proof passes ( "nullifiers[current_nullifier] = true;" ) ? This could lead to a DoS attack.

A state transition is atomic, so if the verifier rejects the proof, then all previous changes made to the contract's state are reversed. Theoretically it doesn't change anything to do the check before or after.
Now, it's always a good practice to exit as soon as possible. The reason why the nullifier check is done before the proof check is exactly for that.
In fact, for the transaction to be valid, you need:

1. The proof to verify correctly
2. The nullifiers to be valid (ie: Not disclosed/used in the past).

It is clear that the proof verification is the most expensive of these 2 checks (you do a pairing check with 3 pairings - in the case of Groth16 - and a linear (in the nb of pub inputs) number of scalar multiplications). Hence, you'd rather check the nullifiers before the proof since it aborts the execution faster than if you checked the proof first.

[AntoineRondelet]

I do not think there are attacks on the range of the other primary inputs. That being said, I would rather have a dedicated function to assemble and check all inputs before doing any verification (since the assemble values are used in several places).

I think only the nullifier needs to be checked indeed.
Moreover, the only function to re-assemble the primary inputs is sha256_digest_from_field_elements which, is defined in the Bytes library (https://github.com/clearmatics/zeth/blob/master/zeth-contracts/contracts/Bytes.sol#L4). I think this can be improved indeed, especially if the packing policy is changed (see: #52). Moreover, this would address #55

[AntoineRondelet]

Looks like zeth is vulnerable, the check should be added here

zeth/zeth-contracts/contracts/Groth16Verifier.sol

Line 115 in e0b4ed4

inputValues[i] = primaryInputs[i];

Thanks @poma !

[rrtoledo]

I think only the nullifier needs to be checked indeed.
Moreover, the only function to re-assemble the primary inputs is sha256_digest_from_field_elements which, is defined in the Bytes library (https://github.com/clearmatics/zeth/blob/master/zeth-contracts/contracts/Bytes.sol#L4). I think this can be improved indeed, especially if the packing policy is changed (see: #52). Moreover, this would address #55

There is indeed only one function which re-assemble any input, what I meant is how we use this function.

• We re-assemble the root and nullifiers in "assemble_root_and_nullifiers_and_append_to_state".
• We reassemble the commitments in "assemble_commitments_and_append_to_state".
• We re-assemble all primary inputs in "assemble_primary_inputs_and_hash" (non malleability update).
  As you can see the commitment, root and nullifiers are reassembled several times, what I proposed is to reassemble them once and for all (but this is unrelated to this issue).

[poma]

Why not check all the inputs in Groth16Verifier.sol? Like this https://github.com/iden3/snarkjs/blob/master/templates/verifier_groth.sol#L191

[rrtoledo]

@poma I proposed to do it outside of Groth16Verifier.sol simply because we have another proof verifiier (PGHR13Verifier.sol) and this check will be done regardless of the proof verifier used. :)

[AntoineRondelet]

This issue was fixed in #74
Some tests have been added. If you want to test the attack, you can checkout to commit 5c83092 that precedes any commit in #74 and run the tests introduced in #74. A double spending should occur at the end. This does not occur anymore with the checks added in the PR.

(btw: Thanks @poma :) )