Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update cairo #338

Merged
merged 5 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions not-so-smart-contracts/cairo/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,14 @@ Each _Not So Smart Contract_ consists of a standard set of information:

## Vulnerabilities

| Not So Smart Contract | Description |
| ------------------------------------------------------------------------------ | ------------------------------------------------------------ |
| [Improper access controls](access_controls) | Flawed access controls due to StarkNet account abstraction |
| [Integer division errors](integer_division) | Unforeseen results from division in a finite field |
| [View state modifications](view_state) | Lack of prevention for state modifications in view functions |
| [Arithmetic overflow](arithmetic_overflow) | Insecure arithmetic in Cairo by default |
| [Signature replays](replay_protection) | Necessary robust reuse protection due to account abstraction |
| [L1 to L2 Address Conversion](L1_to_L2_address_conversion) | Essential L2 address checks for L1 to L2 messaging |
| [Incorrect Felt Comparison](incorrect_felt_comparison) | Unexpected results from felt comparison |
| [Namespace Storage Var Collision](namespace_storage_var_collision) | Storage variables unscoped by namespaces |
| [Dangerous Public Imports in Libraries](dangerous_public_imports_in_libraries) | Ability to call nonimported external functions |
| Not So Smart Contract | Description |
| ---------------------------------------------------------------------------- | ------------------------------------------------------------ |
| [Arithmetic overflow](arithmetic_overflow) | Insecure arithmetic in Cairo for the felt252 type |
| [L1 to L2 Address Conversion](L1_to_L2_address_conversion) | Essential L2 address checks for L1 to L2 messaging |
| [L1 to L2 message failure](l1_to_l2_message_failure) | Messages sent from L1 may not be processed by the sequencer |
| [Overconstrained L1 <-> L2 interaction](overconstrained_l1_l2_interaction) | Asymmetrical checks on the L1 or L2 side can cause a DOS |
| [Signature replays](replay_protection) | Necessary robust reuse protection due to account abstraction |
| [Unchecked from address in L1 Handler](unchecked_from_address_in_l1_handler) | Access control issue when sending messages from L1 to L2 |

## Credits

Expand Down
42 changes: 0 additions & 42 deletions not-so-smart-contracts/cairo/access_controls/README.md

This file was deleted.

29 changes: 21 additions & 8 deletions not-so-smart-contracts/cairo/arithmetic_overflow/README.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,27 @@
# Arithmetic Overflow
# Arithmetic Overflow with Felt Type

The default primitive type, the field element (felt), behaves much like an integer in other languages, but there are 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 represents the prime used by Cairo, which is currently a 252-bit number. Arithmetic operations using felts are unchecked for overflow, which can lead to unexpected results if not properly accounted for. Since the range of values includes both negative and positive values, multiplying two positive numbers can result in a negative value and vice versa—multiplying two negative numbers does not always produce a positive result.
The default primitive type, the field element (felt), behaves much like an integer in other languages, but there are a few important differences to keep in mind. A felt can be interpreted as an unsigned integer in the range [0, P], where P, a 252 bit prime, represents the order of the field used by Cairo. Arithmetic operations using felts are unchecked for overflow or underflow, which can lead to unexpected results if not properly accounted for. Do note that Cairo's builtin primitives for unsigned integers are overflow/underflow safe and will revert.

StarkNet also provides the Uint256 module, offering developers a more typical 256-bit integer. However, the arithmetic provided by this module is also unchecked, so overflow is still a concern. For more robust integer support, consider using SafeUint256 from OpenZeppelin's Contracts for Cairo.
## Example

## Attack Scenarios
The following simplified code highlights the risk of felt underflow. The `check_balance` function is used to validate if a user has a large enough balance. However, the check is faulty because passing an amount such that `amt > balance` will underflow and the check will be true.

## Mitigations
```Cairo

struct Storage {
balances: LegacyMap<ContractAddress, felt252>
}

fn check_balance(self: @ContractState, amt: felt252) {
let caller = get_caller_address();
let balance = self.balances.read(caller);
assert(balance - amt >= 0);
}

- Always add checks for overflow when working with felts or Uint256s directly.
- Consider using the [OpenZeppelin Contracts for Cairo's SafeUint256 functions](https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/security/safemath/library.cairo) instead of performing arithmetic directly.
```

## Mitigations

## Examples
- Always add checks for overflow when working with felts directly.
- Use the default signed integer types unless a felt is explicitly required.
- Consider using Caracal, as it comes with a detector for checking potential overflows when doing felt252 arithmetic operaitons.

This file was deleted.

47 changes: 0 additions & 47 deletions not-so-smart-contracts/cairo/incorrect_felt_comparison/README.md

This file was deleted.

39 changes: 0 additions & 39 deletions not-so-smart-contracts/cairo/integer_division/README.md

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Consider L1 to L2 Message Failure
# L1 to L2 Message Failure

In Starknet, Ethereum contracts can send messages from L1 to L2 using a bridge. However, it is not guaranteed that the message will be processed by the sequencer. For instance, a message can fail to be processed if there is a sudden spike in the gas price and the value provided is too low. To address this issue, Starknet developers have provided an API to cancel ongoing messages.

Expand Down

This file was deleted.

Loading
Loading