Merge pull request #55 from CoinFabrik/53-add-vulnerability-and-detec…

…tor-documentation-for-set-contract-storage

53 add vulnerability and detector documentation for set contract storage
Closes #53

README.md

@@ -44,8 +44,8 @@ cargo scout-audit
 | [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check)       | An arithmetic operation overflows or underflows the available memory allocated to the variable.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1)| Critical    |
 | [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values)       | Avoid using block attributes for random number generation to prevent manipulation.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)| Critical    |
 | [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm)       | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1)| Critical    |
-| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget)                 | The use of `core::mem::forget()` could lead to memory leaks and logic errors.                                                | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)                                                                                                                                              | Enhacement  |
-
+| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget)                 | The use of `core::mem::forget()` could lead to memory leaks and logic errors.                                                | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)        
+| [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage)                           | Insufficient access control on `env.storage()` method.                                                         | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3)                                                                                                                                                | Critical    |
 
 ## Tests
 

detectors/set-contract-storage/README.md

@@ -0,0 +1,34 @@
+# Set contract storage
+
+### What it does
+
+Checks for calls to `env.storage()` without a prior call to `env.require_auth()`.
+
+### Why is this bad?
+
+Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations.
+
+### Known problems
+
+Only check the function call, so false positives could result.
+
+### Example
+
+```rust
+fn set_contract_storage(env: Env) {
+  let _storage = env.storage().instance();
+}
+```
+
+Use instead:
+
+```rust
+fn set_contract_storage(env: Env, user: Address) {
+  user.require_auth();
+  let _storage = env.storage().instance();
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage).

test-cases/README.md

@@ -125,3 +125,24 @@ The `core::mem::forget` function is used to forget about a value without running
 
 We classified this issue, a deviation from best practices which could have
 security implications, under the [Best practices](#vulnerability-categories) category and assigned it an Enhancement severity.
+
+### Set contract storage
+
+Smart contracts can store important information in memory which changes through the contract's lifecycle. Changes happen via user interaction with the smart contract. An _unauthorized_ set contract storage vulnerability happens when a smart contract call allows a user to set or modify contract memory when they were not supposed to be authorized.
+
+Common practice is to have functions with the ability to change
+security-relevant values in memory to be only accessible to specific roles,
+e.g, only an admin can call the function `reset()` which resets auction values.
+When this does not happen, arbitrary users may alter memory which may impose
+great damage to the smart contract users.
+
+In `Soroban`, the method `env.storage()` can be used
+to modify the contract storage under a given key. When a smart contract uses
+this method, the contract needs to check if the caller should be able to
+alter this storage. If this does not happen, an arbitary caller may modify
+balances and other relevant contract storage.
+
+We classified this type of vulnerability under
+the [Authorization](#vulnerability-categories) category and assigned it a
+Critical severity.
+