Merge pull request #171 from crytic/clarify-signed-unsigned-felts

Cairo: Clarify signed/unsigned felts

Author: montyly

not-so-smart-contracts/cairo/arithmetic_overflow/README.md

@@ -1,6 +1,6 @@
 # Arithmetic overflow
 
-The default primitive type, the felt or field element, behaves a lot like an integer does in any other language but it has a few important differences to keep in mind. The range of valid felt values is (-P/2,P/2). P here is the prime used by Cairo, which is current a 252-bit number. Arithemtic using felts is unchecked for overflow and can lead to unexpected results if this isn't properly accounted for. And since the range of values spans both negative and positive values, things like multiplying two positive numbers can have a negative value as a result, and vice versa, multiplying a two negative numbers doesn't always have a positive result.
+The default primitive type, the felt or field element, behaves a lot like an integer does in any other language but it has a few important differences to keep in mind. A felt can be interpreted as a signed integer in the range (-P/2,P/2) or as an unsigned integer in the range (0, P]. P here is the prime used by Cairo, which is current a 252-bit number. Arithemtic using felts is unchecked for overflow and can lead to unexpected results if this isn't properly accounted for. And since the range of values spans both negative and positive values, things like multiplying two positive numbers can have a negative value as a result, and vice versa, multiplying a two negative numbers doesn't always have a positive result.
 
 StarkNet also provides the Uint256 module which offers a more typical 256-bit integer to developers. However, the arithmetic provided by this module is also unchecked so overflow is still something to keep in mind. For more robust integer support, consider using SafeUint256 from OpenZeppelin's Contracts for Cairo.
 

not-so-smart-contracts/cairo/incorrect_felt_comparison/README.md

@@ -1,6 +1,12 @@
 # Incorrect Felt Comparison
 
-In Cairo, there are two builtin methods for the less than or equal to comparison operator: `assert_le` and `assert_nn_le`. `assert_le` asserts that a number `a` is less than or equal to `b`, regardless of the size of `a`, while `assert_nn_le` additionally asserts that `a` is non-negative, i.e. not greater than or equal to the `RANGE_CHECK_BOUND` value of `2^128`: https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/common/math.cairo#L66-L67
+In Cairo, there are two methods for the less than or equal to comparison operator: `assert_le` and `assert_nn_le`:
+- `assert_le` asserts that a number `a` is less than or equal to `b`, regardless of the size of `a`
+- `assert_nn_le` additionally asserts that `a` is [non-negative](https://github.com/starkware-libs/cairo-lang/blob/9889fbd522edc5eff603356e1912e20642ae20af/src/starkware/cairo/common/math.cairo#L71), i.e. not greater than or equal to the `RANGE_CHECK_BOUND` value of `2^128`. 
+
+`assert_nn_le` works to compare unsigned integers but with a value less than `2^128` (e.g. an [Uint256](https://github.com/starkware-libs/cairo-lang/blob/9889fbd522edc5eff603356e1912e20642ae20af/src/starkware/cairo/common/uint256.cairo#L9-L14) field). To compare felts as unsigned integer over the entire range (0, P], `assert_le_felt` should be used. Note these functions exist also with the `is_` prefix where they return 1 (TRUE) or 0 (FALSE).
+
+Due to the complexity of these assertions, a common mistake is to use `assert_le` when `assert_nn_le` should be used.
 
 # Example
 
@@ -37,4 +43,6 @@ func better_comparison{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_c
 
 
 # Mitigations
-Review all felt comparisons closely. Determine what sort of behavior the comparison should have, and if `assert_nn_le` is more appropriate.
+- Review all felt comparisons closely. 
+- Determine what sort of behavior the comparison should have, and if `assert_nn_le` or `assert_le_felt` is more appropriate. 
+- Use `assert_le` if you explicitly want to make comparison between signed integers - otherwise explicitely document why it is used over `assert_nn_le`