Merge branch 'main' into events-detectors

docs/docs/detectors/6-unprotected-update-current-contract-wasm.md

@@ -1,17 +1,26 @@
-# Unprotected update of current contract wasm
+# Unprotected update current contract wasm
 
-### What it does
+## Description 
 
-It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller.
+- Category: `Authorization`
+- Severity: `Critical`
+- Detector: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm)
+- Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) 
+
+It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. 
 
-### Why is this bad?
+
+## Why is this bad? 
 
 If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it.
 
-### Example
+## Issue example 
+
+Consider the following `Soroban` contract:
 
 ```rust
-#[contractimpl]
+
+ #[contractimpl]
 impl UpgradeableContract {
     pub fn init(e: Env, admin: Address) {
         e.storage().instance().set(&DataKey::Admin, &admin);
@@ -22,17 +31,24 @@ impl UpgradeableContract {
     }
 
     pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) {
-        let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap();
-
         e.deployer().update_current_contract_wasm(new_wasm_hash);
     }
 }
-``` 
 
-Use instead:
+```
+
+This contract allows upgrades through the `update_current_contract_wasm` function. If just anyone can call this function, they could modify the contract behaviour.
+
+The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example).
+
+
+## Remediated example
+
+To prevent this, the function should be restricted to administrators or authorized users only.
 
 ```rust
-#[contractimpl]
+
+ #[contractimpl]
 impl UpgradeableContract {
     pub fn init(e: Env, admin: Address) {
         e.storage().instance().set(&DataKey::Admin, &admin);
@@ -51,6 +67,8 @@ impl UpgradeableContract {
 }
 ```
 
-### Implementation
+The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example).
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm)
+## How is it detected?
+
+It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller.

docs/docs/detectors/7-avoid-core-mem-forget.md

@@ -1,31 +1,45 @@
-# Avoid core mem forget usage
+# Avoid core::mem::forget usage
 
-### What it does
+## Description 
 
-Checks for `core::mem::forget` usage.
+- Category: `Best practices`
+- Severity: `Enhancement`
+- Detector:  [`avoid-core-mem-forget`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget)
+- Test Cases: [`avoid-core-mem-forget-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)
 
-### Why is this bad?
 
-This is a bad practice because it can lead to memory leaks, resource leaks and logic errors.
+The `core::mem::forget` function is used to forget about a value without running its destructor. 
 
-### Example
+## Why is this bad? 
 
-```rust
-pub fn forget_something(n: WithoutCopy) -> u64 {
-    core::mem::forget(n);
-    0
-}
-```
+Using this function is a bad practice because this can lead to memory leaks, resource leaks and logic errors.
+
+## Issue example 
 
-Use instead:
+Consider the following `Soroban` contract:
 
 ```rust
-pub fn forget_something(n: WithoutCopy) -> u64 {
-    let _ = n;
-    0
-}
+
+ pub fn forget_something(n: WithoutCopy) -> u64 {
+        core::mem::forget(n);
+        0
+    }	
 ```
 
-### Implementation
+The problem arises from the use of the `core::mem::forget` function. 
+
+The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1/vulnerable-example).
+
+
+## Remediated example
+
+Use the pattern `let _ = n;` or the `.drop()` method instead of `core::mem::forget(n);`.
+
+## How is it detected?
+
+Checks for `core::mem::forget` usage.
+
+## References
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget).
+- [Memory Management](https://docs.alephzero.org/aleph-zero/security-course-by-kudelski-security/ink-developers-security-guideline#memory-management)
+

docs/docs/detectors/9-avoid-panic-error.md

@@ -1,38 +1,54 @@
-# Panic error
+# Avoid panic error 
 
-### What it does
+## Description 
 
-The `panic!` macro is used to stop execution when a condition is not met.
-This is useful for testing and prototyping, but should be avoided in production code.
+- Category: `Validations and error handling`
+- Severity: `Enhancement`
+- Detector: [`avoid-panic-error`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error)
+- Test Cases: [`avoid-panic-error-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) 
 
-### Why is this bad?
+The panic! macro is used to stop execution when a condition is not met. This is useful for testing and prototyping, but should be avoided in production code.
 
-The usage of `panic!` is not recommended because it will stop the execution of the caller contract.
+Using `Result` as return type for functions that can fail is the idiomatic way to handle errors in Rust. The `Result` type is an enum that can be either `Ok` or `Err`. The `Err` variant can contain an error message. The `?` operator can be used to propagate the error message to the caller.
 
-### Known problems
+This way, the caller can decide how to handle the error, although the state of the contract is always reverted on the callee.
 
-While this linter detects explicit calls to `panic!`, there are some ways to raise a panic such as `unwrap()` or `expect()`.
+## Why is this bad? 
 
-### Example
+The usage of `panic!` is not recommended because it will stop the execution of the caller contract. This could lead the contract to an inconsistent state if the execution stops in the middle of state changes. Additionally, if execution stops, it could cause a transaction to fail.
+
+## Issue example 
+
+In the following example, the `panic!` command is being used to handle errors, disallowing the caller to handle the error in a different way, and completely stopping execution of the caller contract.
+
+Consider the following `Soroban` contract:
 
 ```rust
 pub fn add(env: Env, value: u32) -> u32 {
-    let storage = env.storage().instance();
-    let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
-    match count.checked_add(value) {
-        Some(value) => count = value,
-        None => panic!("Overflow error"),
-    }
-    storage.set(&COUNTER, &count);
-    storage.extend_ttl(100, 100);
-    count
-}
+        let storage = env.storage().instance();
+        let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+        match count.checked_add(value) {
+            Some(value) => count = value,
+            None => panic!("Overflow error"),
+        }
+        storage.set(&COUNTER, &count);
+        storage.extend_ttl(100, 100);
+        count
+    }	
 ```
+The add function takes a value as an argument and adds it to the value stored in the contract's storage. The function first checks if the addition will cause an overflow. If the addition will cause an overflow, the function will panic. If the addition will not cause an overflow, the function will add the value to the contract's storage.
 
-Use instead:
+The usage of panic! in this example, is not recommended because it will stop the execution of the caller contract. If the method was called by the user, then he will receive ContractTrapped as the only error message.
+
+The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example).
+
+
+## Remediated example
+
+A possible remediation goes as follows:
 
 ```rust
-pub fn add(env: Env, value: u32) -> Result<u32, Error> {
+ pub fn add(env: Env, value: u32) -> Result<u32, Error> {
     let storage = env.storage().instance();
     let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
     match count.checked_add(value) {
@@ -43,8 +59,24 @@ pub fn add(env: Env, value: u32) -> Result<u32, Error> {
     storage.extend_ttl(100, 100);
     Ok(count)
 }
+
 ```
+And adding the following Error enum:
+
+```rust
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum Error {
+    OverflowError = 1,
+}
+```
+By first defining the Error enum and then returning a Result<(), Error>, more information is added to the caller and, e.g. the caller contract could decide to revert the transaction or to continue execution.
+
+The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example).
+
+## How is it detected?
+
+Checks the use of the macro `panic!`.
 
-### Implementation
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error).
+