added storage_var collision and dangerous import (#119)

* added storage_var collision and dangerous import

* corrected errors in dangerous_public_imports_in_libraries

* corrected errors in dangerous_public_imports_in_libraries

Author: technovision99

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

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

@@ -23,6 +23,8 @@ Each _Not So Smart Contract_ includes a standard set of information:
 | [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
 

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

@@ -0,0 +1,57 @@
+# 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
+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
+
+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. 
+
+

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.