Update Groth16Verifier.sol: fix snarkjs public signals check bug

[andrewcoder666]

Fix snarkjs public signals check bug.
see iden3/snarkjs#518

[nategraf]

Can you provide more detail showing this is indeed a bug, and not a required check. If I understand correctly, the impact here of this extra check is extra gas usage. The impact of removing the check when it's required is critical soundness failure.

[andrewcoder666]

Can you provide more detail showing this is indeed a bug, and not a required check. If I understand correctly, the impact here of this extra check is extra gas usage. The impact of removing the check when it's required is critical soundness failure.

hi @nategraf, according to the risc0 stark_verify.circom file, there are only 5 output signals ( public witness ) passed to the Groth16 verifier, therefore only first 5 elements should be checked before ecPairing check. The line deleted is meant to check the non-existing 6th element of the array parameter, which obviously is to access to the uninitialized memory since the array itself only contains 5 elements as shown in the code.

[Wollac]

This makes sense to me:
uint256[5] calldata _pubSignals has only 5 elements and thus doing calldataload(add(_pubSignals, 160)) reads the calldata one element after _pubSignals[4] , which can contain undefined data

[nategraf]

Looks like this is almost certainly correct. Thanks @andrewcoder666 for bringing this! I'm going to do a bit more due diligence before merging this.