Add failing overflow test.

[BlakeMScurr]

The following test fails by outputting 1 rather than 0. You should expect 218...616 < 10 to be true, but it's not due to an overflow error described here.

I would suggest using CompConstant to verify that the inputs are less than 2^252, but perhaps there's a more efficient way to do that in the context of the codebase.

[BlakeMScurr]

@demonsh @AndriianChestnykh Just for clarify, this unit test demonstrates an overflow vulnerability. A similar one was found in sismo, which prompted looking for one here (sismo's query code was inspired by iden3's).

[AndriianChestnykh]

@BlakeMScurr , thanks for letting us know. It looks reasonable and we'll be applying the fix soon.

[OBrezhniev]

Hello @BlakeMScurr!

Technically this is not a bug. p-1 is considered a negative number. See quote from the circom documentation:

The definition of relational operators < , > , <= , >= , == , != depends on the mathematical function val(x) which is defined as follows:

  val(z) = z-p  if p/2 +1 <= z < p

  val(z) = z,    otherwise.

z = p-1, which is greater than p/2+1, so it's evaluated as z-p = (p-1)-p = -1

We will add full support for negative numbers in the near future.

Thank you for your report! It helped us to spot a few other bugs related to negative number handling. Some of them are already fixed. For now I'm closing this PR.

[BlakeMScurr]

Hi @OBrezhniev!

That's a fair point for p-1, which is indeed a negative number. But I don't think the "breaking point" is at exactly p/2+1. When I found this issue originally I found the breaking point at lt.in[0] > p - 2^n + lt.in[1], which probably doesn't mean anything to you guys. But if you're OK with p-1, then maybe it doesn't matter for your use cases!

I should be able to change the PR and show that the breaking point doesn't exactly correspond to the negative numbers if you like, when I have some time.

[enricobottazzi]

Hi @BlakeMScurr, have you been able to determine what is the "breaking point"?

[BlakeMScurr]

Hi @enricobottazzi! So LessThan works by returning the (flipped) nth bit of 2^n + a - b = s (let's call that s for sum). There is a range check on s, so it has to be an n bit number, which means that a and b can't be too far apart, which helps to limit the confusing cases.

I would argue that it "fails" in the case discuss above by saying that p-1 < 10 - at least if the developer is thinking of the input numbers as being uints, which I think it pretty common.

But @OBrezhniev's argument is fine: if you want negative numbers then p-1 < 10 is totally fine. However, if you want negative numbers you should agree that p/2+1 < p/2, since that's the tipping point for negative numbers. But actually LessThan says the opposite, since p/2+1 is "one greater than p/2."

I kinda think of LessThan as a two part window that you can move around the integers. Divided roughly into:

• Fails if a-b > 2^n
• Returns 1 if 2^n > a-b > 0
• Returns 0 if 0 > a-b > -2^n
• Fails if -2^n > a-b

Does that make sense?