From 72aae78938709c8761c492afc852b92576ffc53f Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Fri, 15 Jul 2022 12:37:49 -0700 Subject: [PATCH 1/3] added storage_var collision and dangerous import --- .../L1_to_L2_address_conversion/README.md | 2 +- not-so-smart-contracts/cairo/README.md | 4 ++ .../README.md | 54 +++++++++++++++++++ .../namespace_storage_var_collision/README.md | 52 ++++++++++++++++++ 4 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 not-so-smart-contracts/cairo/dangerous_public_imports_in_libraries/README.md create mode 100644 not-so-smart-contracts/cairo/namespace_storage_var_collision/README.md diff --git a/not-so-smart-contracts/cairo/L1_to_L2_address_conversion/README.md b/not-so-smart-contracts/cairo/L1_to_L2_address_conversion/README.md index f6b966f6..bbc467df 100644 --- a/not-so-smart-contracts/cairo/L1_to_L2_address_conversion/README.md +++ b/not-so-smart-contracts/cairo/L1_to_L2_address_conversion/README.md @@ -17,7 +17,7 @@ function badDepositToL2(uint256 to, uint256 amount) public returns (bool) { return true; } -function goodDepositToL2(uint256 to, uint256 amount) public returns (bool) { +function betterDepositToL2(uint256 to, uint256 amount) public returns (bool) { require(to !=0 && to < STARKNET_FIELD_PRIME, "invalid address"); //verifies 0 < to < P token.transferFrom(to, address(this),amount); emit Deposited(to,amount); // this message gets processed on L2 diff --git a/not-so-smart-contracts/cairo/README.md b/not-so-smart-contracts/cairo/README.md index 4353a4ef..19720c8b 100644 --- a/not-so-smart-contracts/cairo/README.md +++ b/not-so-smart-contracts/cairo/README.md @@ -21,6 +21,10 @@ Each _Not So Smart Contract_ includes a standard set of information: | [View state modifications](view_state) | View functions don't prevent state modifications | | [Arithmetic overflow](arithmetic_overflow) | Arithmetic in Cairo is not safe by default | | [Signature replays](replay_protection) | Account abstraction requires robust reuse protections | +| [L1 to L2 Address Conversion](l1_to_l2_address_conversion) | L1 to L2 messaging requires L2 address checks | +| [Incorrect Felt Comparison](incorrect_felt_comparison) | Unexpected results can occur during felt comparison | +| [Namespace Storage Var Collision](namespace_storage_var_collision) | Storage variables are not scoped by namespaces | +| [Dangerous Public Imports in Libraries](dangerous_public_imports_in_libraries) | Nonimported external functions can still be called | ## Credits 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 new file mode 100644 index 00000000..99c4f188 --- /dev/null +++ b/not-so-smart-contracts/cairo/dangerous_public_imports_in_libraries/README.md @@ -0,0 +1,54 @@ +# Dangerous Public Imports in Libraries + +When a library is imported in Cairo, all functions can be called even if some of them are not declared in the import statement. As a result, it is possible to call functions that a developer may think is unexposed, leading to unexpected behavior. + +# Example + +Consider the library `library.cairo`. Even though the `example.cairo` file imports only the `check_owner()` and the `_do_something()` function, the `bypass_owner_do_something()` function is still exposed and can thus be called, making it possible to circumvent the owner check. + +```cairo + +# library.cairo + +%lang starknet +@storage_var +func owner() -> (res: felt): +end + +func check_owner(): + 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 +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(): + check_owner() + do_something() + return () +end +``` + +# Mitigations + +Make sure to exercise caution when declaring external functions in a library. Recognize the possible state changes that can be made through the function and verify it is acceptable for anyone to call it. In addition, [Amarna](https://github.com/crytic/amarna) has a detector to uncover this issue. + + 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 new file mode 100644 index 00000000..300e55ce --- /dev/null +++ b/not-so-smart-contracts/cairo/namespace_storage_var_collision/README.md @@ -0,0 +1,52 @@ +# Namespace Storage Variable Collsion + +In Cairo, it is possible to 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 [here](https://gist.github.com/koloz193/18cb491167e844e9a28ac69825f68975). Suppose we have two different namespaces `A` and `B`, both with the same `balance` storage variable. In addition, 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 seperate storage variables have their balance increased respectively. 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 + +Make sure to not use the same storage variable name in the namespace (or change the return value's name, see [here](https://github.com/crytic/amarna/issues/10)). Also use [Amarna](https://github.com/crytic/amarna) to uncover this issue, since it has a detector for storage variable collisions. From 56444f629708b650855b057c7431c84ae1d6e0fe Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Fri, 15 Jul 2022 13:21:48 -0700 Subject: [PATCH 2/3] corrected errors in dangerous_public_imports_in_libraries --- .../cairo/dangerous_public_imports_in_libraries/README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 index 99c4f188..35ef4fbd 100644 --- 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 @@ -11,11 +11,13 @@ Consider the library `library.cairo`. Even though the `example.cairo` file impor # library.cairo %lang starknet +%builtins pedersen range_check +from starkware.cairo.common.cairo_builtins import HashBuiltin @storage_var func owner() -> (res: felt): end -func check_owner(): +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 @@ -38,9 +40,11 @@ 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(): +func check_owner_and_do_something{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr: felt*}(): check_owner() do_something() return () From 53b813cd5afdf6cc6f3e7715c2f2b927d3e7ec8f Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Fri, 15 Jul 2022 13:23:10 -0700 Subject: [PATCH 3/3] corrected errors in dangerous_public_imports_in_libraries --- .../cairo/dangerous_public_imports_in_libraries/README.md | 1 - 1 file changed, 1 deletion(-) 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 index 35ef4fbd..92feba4c 100644 --- 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 @@ -11,7 +11,6 @@ Consider the library `library.cairo`. Even though the `example.cairo` file impor # library.cairo %lang starknet -%builtins pedersen range_check from starkware.cairo.common.cairo_builtins import HashBuiltin @storage_var func owner() -> (res: felt):