Range check #74
Range check #74
Conversation
| @@ -189,6 +191,10 @@ contract BaseMixer is MerkleTreeMiMC7, ERC223ReceivingContract { | |||
| digest_inputs[0] = primary_inputs[i]; // See the way the inputs are ordered in the extended proof | |||
| digest_inputs[1] = primary_inputs[i+1]; | |||
| bytes32 current_commitment = Bytes.sha256_digest_from_field_elements(digest_inputs); | |||
| require( | |||
| uint256(current_commitment) < r, | |||
There was a problem hiding this comment.
The double spending threat has to do with the nullifier, not the commitments.
Basically, N and N + MP lie in the same equiv. class mod P, thus you can we need to make sure that no one is able to use multiple elements from a same equiv class mod P to carry out double spendings. That's why we need to make sure that we only accept primary inputs that are < P (P being the scalar field prime here).
There was a problem hiding this comment.
Ah sorry I removed this check from the assemble function and forgot to add it again (c.f. last commit) (since the assemble function should disappear with the new packing policy).
It is written in the issue to check the primary inputS so I thought you also might want to check tall of them.
There was a problem hiding this comment.
Please make sure to double check the diffs of your PRs next time
There was a problem hiding this comment.
You were not checking the right thing here.
In the lines commented above you compare a 256-bit number with the scalar field characteristic r (encoded on 253 bits), I think you haven't ran the python tests. I haven't but I'd bet they would fail most of the time (nothing should prevent your nullifiers and commitments, both output of sha256 or blake2s to be smaller than r, why did you think they should?)
There was a problem hiding this comment.
Yeah I saw some tests failing.
I thought that these commitments were the field commitments already and not the blake2s digests.
There was a problem hiding this comment.
So do I understand correctly that you wanted to merge to develop a piece of code that:
- Does not fix the vulnerability
- Breaks the tests
??
It does not make any sense. If you are assigned to an issue, this is your responsibility to fix it. The goal is not to push an half-baked patch that breaks half of the tests, ask a review and move to the next one.
|
Fixed in #85 |
7923703 to
3445336
Compare
PR related to issue #38