Merge pull request #239 from CoinFabrik/docs/new-detectors

New documentation for detectors

docs/docs/detectors/rust/assert-violation.md

@@ -0,0 +1,42 @@
+# Assert violation
+
+## Description
+
+- Category: `Validations and error handling`
+- Severity: `Enhancement`
+- Detectors: [assert-violation](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/assert-violation/src/lib.rs)
+
+The `assert!` macro is used in Rust to ensure that a certain condition holds true at a certain point in your code.
+
+### Why is this bad?
+
+The `assert!` macro can cause the contract to panic. It is recommended to avoid this, because it stops its execution, which might lead the contract to an inconsistent state if the panic occurs in the middle of state changes. Additionally, the panic could cause a transaction to fail.
+
+### Example
+
+Consider the following snippet code:
+
+```rust
+pub fn assert_if_greater_than_10(_env: Env, value: u128) -> bool {
+    assert!(value <= 10, "value should be less than 10");
+    true
+}
+```
+
+## Remediated example
+
+Consider using a proper error and return it:
+
+```rust
+pub fn assert_if_greater_than_10(_env: Env, value: u128) -> Result<bool, AVError> {
+    if value <= 10 {
+        Ok(true)
+    } else {
+        Err(AVError::GreaterThan10)
+    }
+}
+```
+
+## How is it detected?
+
+Checks for `assert!` macro usage.

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

@@ -0,0 +1,66 @@
+# Avoid panic error
+
+## Description
+
+- Category: `Validations and error handling`
+- Severity: `Enhancement`
+- Detector: [`avoid-panic-error`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/avoid-panic-error/src/lib.rs)
+
+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.
+
+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.
+
+This way, the caller can decide how to handle the error, although the state of the contract is always reverted on the callee.
+
+## Why is this bad?
+
+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
+
+Consider the following snippet code:
+
+```rust
+pub fn unsafe_check_value(origin: OriginFor<T>, threshold: u32) -> DispatchResult {
+            let who = ensure_signed(origin)?;
+
+            let stored_value = Value::<T>::get().unwrap_or_default();
+            if stored_value < threshold {
+                panic!("Value is too low!");
+            }
+
+            Self::deposit_event(Event::ValueChecked {
+                who,
+                value: stored_value,
+            });
+            Ok(())
+        }
+```
+
+This function panics if `stored_value` is less than `threshold`, disallowing the caller to handle the error in a different way, and completely stopping execution of the caller contract.
+The usage of panic! in this example, is not recommended because it will stop the execution of the caller contract.
+
+## Remediated example
+
+A possible remediation goes as follows:
+
+```rust
+pub fn unsafe_check_value(origin: OriginFor<T>, threshold: u32) -> DispatchResult {
+            let who = ensure_signed(origin)?;
+
+            let stored_value = Value::<T>::get().unwrap_or_default();
+            if stored_value < threshold {
+                return Err(Error::<T>::ValueTooLow.into());
+            }
+
+            Self::deposit_event(Event::ValueChecked {
+                who,
+                value: stored_value,
+            });
+            Ok(())
+        }
+```
+
+## How is it detected?
+
+Checks the use of the macro `panic!`.

docs/docs/detectors/rust/avoid-unsafe-block.md

@@ -0,0 +1,68 @@
+# Avoid unsafe block
+
+## Description
+
+- Category: `Validations and error handling`
+- Severity: `Critical`
+- Detector: [`avoid-unsafe-block`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/avoid-unsafe-block/src/lib.rs)
+
+The use of unsafe blocks in Rust is generally discouraged due to the potential risks it poses to the safety and reliability of the code. Rust's primary appeal lies in its ability to provide memory safety guarantees, which are largely enforced through its ownership and type systems. When you enter an unsafe block, you're effectively bypassing these safety checks. These blocks require the programmer to manually ensure that memory is correctly managed and accessed, which is prone to human error and can be challenging even for experienced developers. Therefore, unsafe blocks should only be used when absolutely necessary and when the safety of the operations within can be assured.
+
+## Why is this bad?
+
+`unsafe` blocks should not be used unless absolutely necessary. The use of unsafe blocks in Rust is discouraged because they bypass Rust's memory safety checks, potentially leading to issues like undefined behavior and security vulnerabilities.
+
+## Issue example
+
+Consider the following `Soroban` contract:
+
+```rust
+#[contractimpl]
+impl AvoidUnsafeBlock {
+    pub fn unsafe_function(n: u64) -> u64 {
+        unsafe {
+            let mut i = n as f64;
+            let mut y = i.to_bits();
+            y = 0x5fe6ec85e7de30da - (y >> 1);
+            i = f64::from_bits(y);
+            i *= 1.5 - 0.5 * n as f64 * i * i;
+            i *= 1.5 - 0.5 * n as f64 * i * i;
+
+            let result_ptr: *mut f64 = &mut i;
+
+            (*result_ptr).to_bits()
+        }
+    }
+}
+```
+
+In this example we can see that it creates a raw pointer named `result_ptr`. Then `(*result_ptr).to_bits()` dereferences the raw pointer. This directly accesses the memory location and calls the `to_bits` method on the value stored at that location.
+
+Raw pointers bypass Rust's type safety system and memory management features. If something goes wrong with the calculations or the value of n, dereferencing the pointer could lead to a memory access violations or undefined behavior.
+
+The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example).
+
+## Remediated example
+
+By removing the raw pointer, the following version eliminates the issue associated with dereferencing memory in an unsafe way. Rust's type safety checks ensure memory is accessed correctly, preventing the potential issues mentioned earlier.
+
+```rust
+ #[contractimpl]
+impl AvoidUnsafeBlock {
+    pub fn unsafe_function(n: u64) -> u64 {
+        let mut i = n as f64;
+        let mut y = i.to_bits();
+        y = 0x5fe6ec85e7de30da - (y >> 1);
+        i = f64::from_bits(y);
+        i *= 1.5 - 0.5 * n as f64 * i * i;
+        i *= 1.5 - 0.5 * n as f64 * i * i;
+        i.to_bits()
+    }
+}
+```
+
+The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example).
+
+## How is it detected?
+
+Checks for usage of `unsafe` blocks.

docs/docs/detectors/rust/divide-before-multiply.md

@@ -0,0 +1,43 @@
+# Divide before multiply
+
+## Description
+
+- Category: `Arithmetic`
+- Severity: `Medium`
+- Detectors: [`divide-before-multiply`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/divide-before-multiply/src/lib.rs)
+
+In Rust, the order of operations can influence the precision of the result, especially in integer arithmetic.
+
+## Why is this bad?
+
+Performing a division operation before a multiplication can lead to a loss of precision as division between integers might return zero.
+
+## Issue example
+
+Consider the following snippet code:
+
+```rust
+ pub fn split_profit(percentage: u64, total_profit: u64) -> u64 {
+    (percentage / 100) * total_profit
+}
+```
+
+In this contract, the `split_profit` function divides the `percentage` by `100` before multiplying it with `total_profit`. This could lead to a loss of precision if `percentage` is less than `100` as the division would return `0`. This could lead to incorrect calculations and potential financial loss in a real-world smart contract.
+
+## Remediated example
+
+Reverse the order of operations to ensure multiplication occurs before division.
+
+```rust
+pub fn split_profit(&self, percentage: u64, total_profit: u64) -> u64 {
+    (percentage * total_profit) / 100
+}
+```
+
+## How is it detected?
+
+Checks the existence of a division before a multiplication.
+
+## References
+
+[Rust documentation: `Integer Division`](https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators)

docs/docs/detectors/rust/empty-expect.md

@@ -0,0 +1,49 @@
+# Empty expect
+
+## Description
+
+- Category: `Best Practices`
+- Severity: `Medium`
+- Detectors: [`empty-expect`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/rust/empty-expect)
+
+An empty `.expect()` creates a panic without any explanatory message, leaving developers without information to diagnose the error or trace its origin. This lack of clarity can lead to longer resolution times, poor maintenance practices, and potentially even security issues if sensitive operations fail without explanation.
+
+## Issue example
+
+Consider the following function:
+
+```rust
+#[pallet::call_index(0)]
+pub fn unsafe_get_storage(origin: OriginFor<T>) -> DispatchResult {
+    let who = ensure_signed(origin)?;
+    let example_storage = ExampleStorage::<T>::get();
+    if example_storage.is_some() {
+        let value = example_storage.expect("");
+        Self::deposit_event(Event::UnsafeGetStorage { who, value });
+    }
+    Ok(())
+}
+```
+
+In the the `unsafe_get_storage` function, the line `example_storage.expect("")` uses an empty string in the `.expect()` method. This is problematic because it provides no context for the panic that occurs if the `Option` is `None`. If a panic is triggered, debugging the issue becomes significantly harder, as there is no information to explain what went wrong or why the code expected a value in the storage.
+
+The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/empty-expect/vulnerable/vulnerable-1).
+
+## Remediation
+
+Make the `.expect()` method include a descriptive message. This change ensures that if the `Option` is `None` and a panic occurs, the message clearly explains the problem.
+
+```rust
+#[pallet::call_index(0)]
+pub fn unsafe_get_storage(origin: OriginFor<T>) -> DispatchResult {
+    let who = ensure_signed(origin)?;
+    let example_storage = ExampleStorage::<T>::get();
+    if example_storage.is_some() {
+        let value = example_storage.expect("Storage is not initialized");
+        Self::deposit_event(Event::UnsafeGetStorage { who, value });
+    }
+    Ok(())
+}
+```
+
+The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/empty-expect/remediated/remediated-1).

docs/docs/detectors/rust/incorrect-exponentiation.md

@@ -0,0 +1,55 @@
+# Incorrect exponentiation
+
+## Description
+
+- Issue Category: `Arithmetic`
+- Issue Severity: `Critical`
+- Detectors: [`incorrect-exponentiation`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/incorrect-exponentiation/src/lib.rs)
+
+The operator `^` is not an exponential operator, it is a bitwise XOR. Make sure to use `pow()` instead for exponentiation. In case of performing a XOR operation, use `.bitxor()` for clarity.
+
+## Why is it bad?
+
+It can produce unexpected behaviour in the smart contract.
+
+## Issue example
+
+In the following example, the `^` operand is being used for exponentiation. But in Rust, `^` is the operand for an XOR operation. If misused, this could lead to unexpected behaviour in our contract.
+
+Consider the following `Substrate pallet`:
+
+```rust
+#[pallet::call_index(0)]
+        pub fn set_balance(origin: OriginFor<T>, new_value: u32) -> DispatchResult {
+            let who = ensure_signed(origin)?;
+            let calculated_value = new_value ^ 3;
+            Value::<T>::put(calculated_value);
+            Self::deposit_event(Event::BalanceSet {
+                who,
+                value: calculated_value,
+            });
+            Ok(())
+        }
+```
+
+## Remediated example
+
+A possible solution is to use the method `pow()`. But, if a XOR operation is wanted, `.bitxor()` method is recommended.
+
+```rust
+#[pallet::call_index(0)]
+pub fn set_balance(origin: OriginFor<T>, new_value: u32) -> DispatchResult {
+let who = ensure_signed(origin)?;
+let calculated_value = new_value.pow(3);
+Value::<T>::put(calculated_value);
+Self::deposit_event(Event::BalanceSet {
+who,
+value: calculated_value,
+});
+Ok(())
+}
+```
+
+## How is it detected?
+
+Warns about `^` being a `bit XOR` operation instead of an exponentiation.

docs/docs/detectors/rust/known-vulnerabilities.md

@@ -0,0 +1,18 @@
+# Known vulnerabilities
+
+## Description
+
+- Category: `Known Bugs`
+- Severity: `Medium`
+- Detectors: [`known-vulnerabilities`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/rust/known-vulnerabilities)
+
+Using libraries marked as risky is dangerous, as they contain vulnerabilities that pose significant risks to the security and stability of the codebase.
+
+## Remediation
+
+Updating to secure and supported versions of critical libraries or frameworks can help mitigate risks associated with outdated dependencies. Conducting routine security scans using tools such as cargo audit or similar ensures that potential vulnerabilities are identified and addressed promptly. Additionally, staying informed about security advisories and monitoring updates for patches or upgrades related to your dependencies will help maintain the long-term security of your application.
+
+## References
+
+- [RustSec](https://rustsec.org/)
+- [RustSec Github Repository](https://github.com/RustSec/advisory-db)