diff --git a/not-so-smart-contracts/cairo/README.md b/not-so-smart-contracts/cairo/README.md index 806630c3..f8ebb03f 100644 --- a/not-so-smart-contracts/cairo/README.md +++ b/not-so-smart-contracts/cairo/README.md @@ -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 diff --git a/not-so-smart-contracts/cairo/access_controls/README.md b/not-so-smart-contracts/cairo/access_controls/README.md deleted file mode 100644 index 550ce6e1..00000000 --- a/not-so-smart-contracts/cairo/access_controls/README.md +++ /dev/null @@ -1,42 +0,0 @@ -# Access Controls and Account Abstraction - -_NOTE: The following was possible before StarkNet OS enforced the use of an account contract._ - -StarkNet employs an account abstraction model, which has some key distinctions compared to what Solidity developers may be accustomed to. In StarkNet, only contract addresses exist, and there are no EOA (Externally Owned Account) addresses. Typically, users deploy a contract that authenticates them and makes additional calls on their behalf, rather than interacting with contracts directly. The most basic form of this contract verifies whether the transaction is signed by the expected key, but it can also represent more elaborate structures like multisig or DAOs, or include complex logic for transaction allowances (e.g. separate contracts for deposits and withdrawals or prevention of unprofitable trades). - -Although direct contract interaction is still possible, the calling contract address will be set to 0 from the perspective of the contract being called. Since 0 is also the default value for uninitialized storage, it is possible to accidentally create access control checks that default to open access, as opposed to properly restricting access to intended users only. - -## Example - -Consider the two functions below, which both permit a user to claim a small number of tokens. The first function, without any checks, inadvertently sends tokens to the zero address, effectively eliminating them from the circulating supply. The second function prevents this from happening. - -```Cairo -@external -func bad_claim_tokens { syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr } (): - let (user) = get_caller_address() - - let (user_current_balance) = user_balances.read(sender_address) - user_balances.write(user_current_balance + 200) - - return () -end - -@external -func better_claim_tokens { syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr } (): - let (user) = get_caller_address() - assert_not_equal(user, 0) - - let (user_current_balance) = user_balances.read(sender_address) - user_balances.write(user, user_current_balance + 200) - - return () -end -``` - -## Mitigations - -- Include checks for the zero address. This will, however, prevent users from directly interacting with the contract. - -## External Examples - -- An [issue](https://github.com/OpenZeppelin/cairo-contracts/issues/148) found in the ERC721 implementation within a pre-0.1.0 version of OpenZeppelin's Cairo contract library, which allowed unauthorized users to transfer tokens. diff --git a/not-so-smart-contracts/cairo/arithmetic_overflow/README.md b/not-so-smart-contracts/cairo/arithmetic_overflow/README.md index 2d5e39a8..762190d7 100644 --- a/not-so-smart-contracts/cairo/arithmetic_overflow/README.md +++ b/not-so-smart-contracts/cairo/arithmetic_overflow/README.md @@ -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 +} + +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. diff --git a/not-so-smart-contracts/cairo/dangerous_public_imports_in_libraries/README.md b/not-so-smart-contracts/cairo/dangerous_public_imports_in_libraries/README.md deleted file mode 100644 index 7bc030f1..00000000 --- a/not-so-smart-contracts/cairo/dangerous_public_imports_in_libraries/README.md +++ /dev/null @@ -1,54 +0,0 @@ -# Dangerous Public Imports in Libraries - -NOTE: The following issue was present until cairo-lang 0.10.0. - -When a library is imported in Cairo, all functions become callable even if they are not explicitly declared in the import statement. Consequently, developers may unintentionally expose functions that lead to unexpected behavior. - -# Example - -Consider the library `library.cairo`. Although the `example.cairo` file imports only the `check_owner()` and `do_something()` functions, the `bypass_owner_do_something()` function is still exposed and can be called, allowing the owner check to be circumvented. - -```cairo -# library.cairo - -%lang starknet -from starkware.cairo.common.cairo_builtins import HashBuiltin -@storage_var -func owner() -> (res: felt): -end - -func check_owner{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr: felt*}(): - let caller = get_caller_address() - let owner = owner.read() - assert caller = owner - return () -end - -func do_something(): - # do something potentially dangerous that only the owner can do - return () -end - -# for testing purposes only -@external -func bypass_owner_do_something(): - do_something() - return () -end - -# example.cairo -%lang starknet -%builtins pedersen range_check -from starkware.cairo.common.cairo_builtins import HashBuiltin -from library import check_owner(), do_something() -# Even though we just import check_owner() and do_something(), we can still call bypass_owner_do_something()! -func check_owner_and_do_something{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr: felt*}(): - check_owner() - do_something() - return () -end -``` - -# Mitigations - -Exercise caution when declaring external functions in a library. Evaluate the potential state changes that can be made through the function and ensure it is safe for any user to call. Additionally, [Amarna](https://github.com/crytic/amarna) includes a detector to help identify this issue. diff --git a/not-so-smart-contracts/cairo/incorrect_felt_comparison/README.md b/not-so-smart-contracts/cairo/incorrect_felt_comparison/README.md deleted file mode 100644 index 40c24dc7..00000000 --- a/not-so-smart-contracts/cairo/incorrect_felt_comparison/README.md +++ /dev/null @@ -1,47 +0,0 @@ -# Incorrect Felt Comparison - -In Cairo, the less than or equal to comparison operator has two methods: `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), essentially meaning it should be less than or equal to the `RANGE_CHECK_BOUND` value of `2^128`. - -`assert_nn_le` is suitable for comparing unsigned integers with values smaller 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 integers over the entire range (0, P], use `assert_le_felt`. These functions also exist with the `is_` prefix, where they return 1 (TRUE) or 0 (FALSE). - -One common mistake resulting from the complexity of these assertions is using `assert_le` instead of `assert_nn_le`. - -# Example - -Consider the example of a codebase that uses the following checks concerning a hypothetical ERC20 token. The first function may incorrectly pass the assertion even if the `value` is greater than `max_supply`, because the function does not verify that `value >= 0`. The second function, however, asserts that `0 <= value <= max_supply`, which will correctly prevent an incorrect `value` from passing the assertion. - -```cairo -@storage_var -func max_supply() -> (res: felt) { -} - -@external -func bad_comparison{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}() { - let (value: felt) = ERC20.total_supply(); - assert_le{range_check_ptr=range_check_ptr}(value, max_supply.read()); - - // do something... - - return (); -} - -@external -func better_comparison{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}() { - let (value: felt) = ERC20.total_supply(); - assert_nn_le{range_check_ptr=range_check_ptr}(value, max_supply.read()); - - // do something... - - return (); - -} -``` - -# Mitigations - -- Carefully review all felt comparisons. -- Determine the desired behavior of the comparison and decide if `assert_nn_le` or `assert_le_felt` is more appropriate. -- Use `assert_le` if you explicitly want to compare signed integers. Otherwise, clearly document why it is used over `assert_nn_le`. diff --git a/not-so-smart-contracts/cairo/integer_division/README.md b/not-so-smart-contracts/cairo/integer_division/README.md deleted file mode 100644 index 13e8d9e3..00000000 --- a/not-so-smart-contracts/cairo/integer_division/README.md +++ /dev/null @@ -1,39 +0,0 @@ -# Integer Division - -Math in Cairo is done in a finite field, which explains why the numeric type is called `felt` for field elements. In most cases, addition, subtraction, and multiplication will behave like standard integer operations when writing Cairo code. However, developers need to pay extra attention when performing division. Unlike in Solidity, where division is carried out as if the values were real numbers and anything after the decimal place is truncated, in Cairo, it's more intuitive to think of division as the inverse of multiplication. When a number divides a whole number of times into another number, the result is what we would expect, such as 30/6=5. However, if we try to divide numbers that don't quite match up so perfectly, like 30/9, the result might be surprising, such as 1206167596222043737899107594365023368541035738443865566657697352045290673497. That's because 120...97 \* 9 = 30 (modulo the [252-bit prime used by StarkNet](https://docs.starkware.co/starkex-v4/crypto/stark-curve)). - -## Example - -Consider the following functions that normalize a user's token balance to a human-readable value for a token with 10^18 decimals. In the first function, it will provide meaningful values only when a user has a whole number of tokens and will return nonsensical values in every other case. The better version stores these values as Uint256s and employs more traditional integer division. - -```cairo -@external -func bad_normalize_tokens{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}() -> ( - normalized_balance: felt -) { - let (user) = get_caller_address(); - - let (user_current_balance) = user_balances.read(user); - let (normalized_balance) = user_current_balance / 10 ** 18; - - return (normalized_balance,); -} - -@external -func better_normalize_tokens{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}() -> ( - normalized_balance: Uint256 -) { - let (user) = get_caller_address(); - - let (user_current_balance) = user_balances.read(user); - let (normalized_balance, _) = uint256_unsigned_div_rem(user_current_balance, 10 ** 18); - - return (normalized_balance,); -} -``` - -## Mitigations - -- Review the most appropriate numeric type for your use case. Especially if your programs rely on division, consider using [the uint256 module](https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/common/uint256.cairo) instead of the felt primitive type. - -## External Examples diff --git a/not-so-smart-contracts/cairo/consider_L1_to_L2_message_failure.md b/not-so-smart-contracts/cairo/l1_to_l2_message_failure/README.md similarity index 97% rename from not-so-smart-contracts/cairo/consider_L1_to_L2_message_failure.md rename to not-so-smart-contracts/cairo/l1_to_l2_message_failure/README.md index dbef0658..671ecee9 100644 --- a/not-so-smart-contracts/cairo/consider_L1_to_L2_message_failure.md +++ b/not-so-smart-contracts/cairo/l1_to_l2_message_failure/README.md @@ -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. diff --git a/not-so-smart-contracts/cairo/namespace_storage_var_collision/README.md b/not-so-smart-contracts/cairo/namespace_storage_var_collision/README.md deleted file mode 100644 index f8d3c9d5..00000000 --- a/not-so-smart-contracts/cairo/namespace_storage_var_collision/README.md +++ /dev/null @@ -1,54 +0,0 @@ -# Namespace Storage Variable Collision - -**Note:** The issues described below were possible until Cairo-lang 0.10.0. - -In Cairo, you can use namespaces to scope functions under an identifier. However, storage variables are not scoped by these namespaces. If a developer accidentally uses the same variable name in two different namespaces, it could lead to a storage collision. - -# Example - -The following example has been copied from [this Gist](https://gist.github.com/koloz193/18cb491167e844e9a28ac69825f68975). Suppose we have two different namespaces `A` and `B`, both with the same `balance` storage variable. Additionally, both namespaces have respective functions `increase_balance()` and `get_balance()` to increment the storage variable and retrieve it respectively. When either `increase_balance_a()` or `increase_balance_b()` is called, the expected behavior would be to have two separate storage variables increase their balances. However, because storage variables are not scoped by namespaces, there will be one `balance` variable updated twice: - -```cairo -%lang starknet - -from starkware.cairo.common.cairo_builtins import HashBuiltin - -from openzeppelin.a import A -from openzeppelin.b import B - -@external -func increase_balance_a { - syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, - range_check_ptr}(amount : felt): - A.increase_balance(amount) - return () -end - -@external -func increase_balance_b { - syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, - range_check_ptr}(amount : felt): - B.increase_balance(amount) - return () -end - -@view -func get_balance_a { - syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, - range_check_ptr}() -> (res : felt): - let (res) = A.get_balance() - return (res) -end - -@view -func get_balance_b { - syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, - range_check_ptr}() -> (res : felt): - let (res) = B.get_balance() - return (res) -end -``` - -# Mitigations - -To avoid this issue, ensure that you do not use the same storage variable name in different namespaces (or change the return value's name, as explained [here](https://github.com/crytic/amarna/issues/10)). You can also use [Amarna](https://github.com/crytic/amarna) to detect this problem, as it has a built-in detector for storage variable collisions. diff --git a/not-so-smart-contracts/cairo/overconstrained_l1_l2_interaction/README.md b/not-so-smart-contracts/cairo/overconstrained_l1_l2_interaction/README.md new file mode 100644 index 00000000..01a23de4 --- /dev/null +++ b/not-so-smart-contracts/cairo/overconstrained_l1_l2_interaction/README.md @@ -0,0 +1,59 @@ +# Overconstrained L1 <-> L2 interaction + +When interacting with contracts that are designed to interact with both L1 and L2, care must be taken that the checks and validations on both sides are symmetrical. If one side has more validations than the other, this could create a situation where a user performs an action on one side, but is unable to perform the corresponding action on the other side, leading to a loss of funds or a denial of service. + +## Example + +The following Starknet bridge contract allows for permissionless deposit to any address on L1 via the `deposit_to_L1` function. In particular, someone can deposit tokens to the `BAD_ADDRESS`. However, in that case the tokens will be lost forever, because the tokens are burned on L2 and the L1 contract's `depositFromL2` function prevents `BAD_ADDRESS` from being the recipient. + +```Cairo +#[storage] +struct Storage { + l1_bridge: EthAddress, + balances: LegacyMap +} + +#[derive(Serde)] +struct Deposit { + recipient: EthAddress, + token: EthAddress, + amount: u256 +} + +fn deposit_to_l1(ref self: ContractState, deposit: Deposit) { + let caller = get_caller_address(); + //burn the tokens on the L2 side + self.balances.write(caller, self.balances.read(caller) - deposit.amount); + let payload = ArrayTrait::new(); + starknet::send_message_to_l1_syscall(self.l1_bridge.read(), deposit.serialize(ref payload)).unwrap(); +} +``` + +```solidity + +address public immutable MESSENGER_CONTRACT; +address public immutable L2_TOKEN_BRIDGE; +address public constant BAD_ADDRESS = address(0xdead); + +constructor(address _messenger, address _bridge) { + MESSENGER_CONTRACT = _messenger; + L2_TOKEN_BRIDGE = _bridge; +} + +function depositFromL2(address recipient, address token, uint256 amount) external { + require(recipient != BAD_ADDRESS, "blacklisted"); + uint256[] memory payload = _buildPayload(recipient,token,amount); + MESSENGER_CONTRACT.consumeMessageFromL2(L2_TOKEN_BRIDGE,payload); + //deposit logic + [...] +} + +function _buildPayload(address recipient, address token, uint256 amount) internal returns (uint256[] memory) { + //payload building logic for Starknet message + [...] +} +``` + +## Mitigations + +- Make sure to validate that the checks on both the L1 and L2 side are similar enough to prevent unexpected behavior. Ensure that any unsymmetric validations on either side cannot lead to a tokens being trapped or any other denial of service. diff --git a/not-so-smart-contracts/cairo/replay_protection/README.md b/not-so-smart-contracts/cairo/replay_protection/README.md index 718fbb35..9c99306a 100644 --- a/not-so-smart-contracts/cairo/replay_protection/README.md +++ b/not-so-smart-contracts/cairo/replay_protection/README.md @@ -4,14 +4,29 @@ The StarkNet account abstraction model enables offloading many authentication de ## Example -Consider the following function that validates a signature for EIP712-style permit functionality. The first version lacks both a nonce and a method for identifying the specific chain a signature is designed for. Consequently, this signature schema would allow signatures to be replayed both on the same chain and across different chains, such as between a testnet and mainnet. +Consider the following function that validates a signature for EIP712-style permit functionality. Notice that the contract lacks a way of keeping track of nonces. As a result, the same signature can be replayed over and over again. In addition, there is no method for identifying the specific chain a signature is designed for. Consequently, this signature schema would allow signatures to be replayed both on the same chain and across different chains, such as between a testnet and mainnet. ```cairo - # TODO + #[storage] + struct Storage { + authorized_pubkey: felt252 + } + + #[derive(Hash)] + struct Signature { + sig_r: felt252, + sig_s: felt252, + amount: u256, + recipient: ContractAddress + } + + fn bad_is_valid_signature(self: @ContractState, sig: Signature) { + let hasher = PoseidonTrait::new(); + let hash = hasher.update_with(sig).finalize(); + ecdsa::check_ecdsa_signature(hash,authorized_pubkey,sig.r,sig.s); + } ``` ## Mitigations - Consider using the [OpenZeppelin Contracts for Cairo Account contract](https://github.com/OpenZeppelin/cairo-contracts/blob/main/docs/modules/ROOT/pages/accounts.adoc) or another existing account contract implementation. - -## External Examples diff --git a/not-so-smart-contracts/cairo/unchecked_from_address_in_l1_handler/README.md b/not-so-smart-contracts/cairo/unchecked_from_address_in_l1_handler/README.md new file mode 100644 index 00000000..8c198cc7 --- /dev/null +++ b/not-so-smart-contracts/cairo/unchecked_from_address_in_l1_handler/README.md @@ -0,0 +1,42 @@ +# Unchecked from address in L1 Handler function + +A function with the `l1_handler` annotation is intended to be called from L1. The first parameter of the `l1_handler` function is always `from`, which represents the `msg.sender` of the L1 transaction that attempted to invoke the function on Starknet. If the `l1_handler` function is designed to be invoked from a specific address on mainnet, not checking the from address may allow anyone to call the function, opening up access control vulnerabilities. + +## Example + +The following Starknet bridge contract's owner, specified in the `uint256[] calldata payload` array, is designed to be called only from the `setOwnerOnL2()` function. Even though the owner is checked on the solidity side, the lack of validation of the `from_address` parameter allows anyone to call the function from an arbitrary L1 contract, becoming the owner of the bridge on L2. + +```solidity +address public immutable OWNER; +address public immutable MESSENGER_CONTRACT; +address public immutable L2_BRIDGE_ADDRESS; +constructor(address _owner, address _messenger, address _bridge) { + OWNER = _owner; + MESSENGER_CONTRACT = _messenger; + L2_BRIDGE_ADDRESS = _bridge; + +} + +function setOwnerOnL2(uint256[] calldata payload, uint256 selector) external { + require(owner == msg.sender, "not owner"); + IStarknetMessaging(MESSENGER_CONTRACT).sendMessageToL2(L2_BRIDGE_ADDRESS, selector, payload); +} +``` + +```Cairo +#[storage] +struct Storage { + owner: ContractAddress +} + +#[l1_handler] +fn set_owner_from_l1(ref self: ContractState, from_address: felt252, new_owner: ContractAddress) { + self.owner.write(new_owner); +} + +``` + +## Mitigations + +- Make sure to validate the `from_address`, otherwise any L1 contract can invoke the annotated Starknet function. +- Consider using Caracal, as it comes with a detector for verifying if the `from_address` is unchecked in an `l1_handler` function. diff --git a/not-so-smart-contracts/cairo/view_state/README.md b/not-so-smart-contracts/cairo/view_state/README.md deleted file mode 100644 index 5eb8c895..00000000 --- a/not-so-smart-contracts/cairo/view_state/README.md +++ /dev/null @@ -1,26 +0,0 @@ -# State Modifications in View Functions - -StarkNet uses the @view decorator to indicate that a function should not modify the state. However, this restriction is [not currently enforced by the compiler](https://www.cairo-lang.org/docs/hello_starknet/intro.html). Developers should exercise caution when creating view functions and when calling functions in other contracts, as there may be unintended consequences if they accidentally include state modifications. - -## Example - -Consider the following function that is declared as a `@view`. It might have originally been intended solely as a view function, but was later repurposed to fetch a nonce _and increment it in the process_ to ensure that a nonce is never repeated when creating a signature. - -```cairo -@view -func bad_get_nonce{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}() -> ( - nonce: felt -) { - let (user) = get_caller_address(); - let (nonce) = user_nonces.read(user); - user_nonces.write(user, nonce + 1); - - return (nonce); -} -``` - -## Mitigations - -- Thoroughly review all `@view` functions, including those in third-party contracts, to make sure they don't unintentionally modify the state. - -## External Examples