From 57423dcce9e6bc5802f551c4a22403ed70961756 Mon Sep 17 00:00:00 2001 From: fpereira24 Date: Wed, 22 Jan 2025 12:08:50 -0300 Subject: [PATCH 1/3] add new detectors doc --- docs/docs/detectors/rust/assert-violation.md | 42 +++++++ docs/docs/detectors/rust/avoid-panic-error.md | 66 +++++++++++ .../docs/detectors/rust/avoid-unsafe-block.md | 68 +++++++++++ .../detectors/rust/divide-before-multiply.md | 43 +++++++ docs/docs/detectors/rust/empty-expect.md | 49 ++++++++ .../rust/incorrect-exponentiation.md | 55 +++++++++ .../detectors/rust/known-vulnerabilities.md | 18 +++ docs/docs/detectors/rust/overflow-check.md | 90 +++++++++++++++ docs/docs/detectors/rust/unsafe-expect.md | 41 +++++++ docs/docs/detectors/rust/unsafe-unwrap.md | 61 ++++++++++ .../detectors/substrate/assert-violation.md | 50 ++++++++ .../detectors/substrate/avoid-debug-info.md | 7 +- .../detectors/substrate/avoid-panic-error.md | 66 +++++++++++ .../detectors/substrate/avoid-unsafe-block.md | 63 ++++++++++ .../substrate/divide-before-multiply.md | 81 +++++++++++++ docs/docs/detectors/substrate/empty-expect.md | 8 +- .../detectors/substrate/equal-addresses.md | 3 +- .../substrate/incorrect-exponentiation.md | 55 +++++++++ .../integer-overflow-or-underflow.md | 59 ++++++++++ .../substrate/invalid-extrinsic-weight.md | 8 +- .../substrate/iterators-over-indexing.md | 68 +++++++++++ .../detectors/substrate/overflow-check.md | 109 ++++++++++++++++++ .../docs/detectors/substrate/unsafe-expect.md | 64 ++++++++++ .../docs/detectors/substrate/unsafe-unwrap.md | 61 ++++++++++ .../detectors/substrate/unsigned-extrinsic.md | 8 +- 25 files changed, 1225 insertions(+), 18 deletions(-) create mode 100644 docs/docs/detectors/rust/assert-violation.md create mode 100644 docs/docs/detectors/rust/avoid-panic-error.md create mode 100644 docs/docs/detectors/rust/avoid-unsafe-block.md create mode 100644 docs/docs/detectors/rust/divide-before-multiply.md create mode 100644 docs/docs/detectors/rust/empty-expect.md create mode 100644 docs/docs/detectors/rust/incorrect-exponentiation.md create mode 100644 docs/docs/detectors/rust/known-vulnerabilities.md create mode 100644 docs/docs/detectors/rust/overflow-check.md create mode 100644 docs/docs/detectors/rust/unsafe-expect.md create mode 100644 docs/docs/detectors/rust/unsafe-unwrap.md create mode 100644 docs/docs/detectors/substrate/assert-violation.md create mode 100644 docs/docs/detectors/substrate/avoid-panic-error.md create mode 100644 docs/docs/detectors/substrate/avoid-unsafe-block.md create mode 100644 docs/docs/detectors/substrate/divide-before-multiply.md create mode 100644 docs/docs/detectors/substrate/incorrect-exponentiation.md create mode 100644 docs/docs/detectors/substrate/integer-overflow-or-underflow.md create mode 100644 docs/docs/detectors/substrate/iterators-over-indexing.md create mode 100644 docs/docs/detectors/substrate/overflow-check.md create mode 100644 docs/docs/detectors/substrate/unsafe-expect.md create mode 100644 docs/docs/detectors/substrate/unsafe-unwrap.md diff --git a/docs/docs/detectors/rust/assert-violation.md b/docs/docs/detectors/rust/assert-violation.md new file mode 100644 index 00000000..d59956cc --- /dev/null +++ b/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 { + if value <= 10 { + Ok(true) + } else { + Err(AVError::GreaterThan10) + } +} +``` + +## How is it detected? + +Checks for `assert!` macro usage. diff --git a/docs/docs/detectors/rust/avoid-panic-error.md b/docs/docs/detectors/rust/avoid-panic-error.md new file mode 100644 index 00000000..c420ec7c --- /dev/null +++ b/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, threshold: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + + let stored_value = Value::::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, threshold: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + + let stored_value = Value::::get().unwrap_or_default(); + if stored_value < threshold { + return Err(Error::::ValueTooLow.into()); + } + + Self::deposit_event(Event::ValueChecked { + who, + value: stored_value, + }); + Ok(()) + } +``` + +## How is it detected? + +Checks the use of the macro `panic!`. diff --git a/docs/docs/detectors/rust/avoid-unsafe-block.md b/docs/docs/detectors/rust/avoid-unsafe-block.md new file mode 100644 index 00000000..02df6327 --- /dev/null +++ b/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. diff --git a/docs/docs/detectors/rust/divide-before-multiply.md b/docs/docs/detectors/rust/divide-before-multiply.md new file mode 100644 index 00000000..2009a915 --- /dev/null +++ b/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) diff --git a/docs/docs/detectors/rust/empty-expect.md b/docs/docs/detectors/rust/empty-expect.md new file mode 100644 index 00000000..f0ae0f03 --- /dev/null +++ b/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) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::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) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::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). diff --git a/docs/docs/detectors/rust/incorrect-exponentiation.md b/docs/docs/detectors/rust/incorrect-exponentiation.md new file mode 100644 index 00000000..1d864432 --- /dev/null +++ b/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, new_value: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + let calculated_value = new_value ^ 3; + Value::::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, new_value: u32) -> DispatchResult { +let who = ensure_signed(origin)?; +let calculated_value = new_value.pow(3); +Value::::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. diff --git a/docs/docs/detectors/rust/known-vulnerabilities.md b/docs/docs/detectors/rust/known-vulnerabilities.md new file mode 100644 index 00000000..d7130bbc --- /dev/null +++ b/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) diff --git a/docs/docs/detectors/rust/overflow-check.md b/docs/docs/detectors/rust/overflow-check.md new file mode 100644 index 00000000..84c54caf --- /dev/null +++ b/docs/docs/detectors/rust/overflow-check.md @@ -0,0 +1,90 @@ +# Overflow-check + +## Description + +- Category: `Arithmetic` +- Severity: `Critical` +- Detectors: [`overflow-check`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/overflow-check/src/lib.rs) + +Checks that `overflow-checks` is enabled in the `[profile.release]` section of the `Cargo.toml`. + +### Why is this bad? + +Integer overflow will trigger a panic in debug builds or will wrap in +release mode. Division by zero will cause a panic in either mode. In some applications one +wants explicitly checked, wrapping or saturating arithmetic. + +### Example + +```toml +[package] +name = "overflow-check-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[dev-dependencies] +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = false +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true +``` + +Use instead: + +```toml +[package] +name = "overflow-check-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[dev-dependencies] +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +overflow-checks = true +inherits = "release" +debug-assertions = true +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check). diff --git a/docs/docs/detectors/rust/unsafe-expect.md b/docs/docs/detectors/rust/unsafe-expect.md new file mode 100644 index 00000000..ba23361e --- /dev/null +++ b/docs/docs/detectors/rust/unsafe-expect.md @@ -0,0 +1,41 @@ +# Unsafe expect + +## Description + +- Category: `Validations and error handling` +- Severity: `Minor` +- Detectors: [`unsafe-expect`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/unsafe-expect/src/lib.rs) + +In Rust, the `expect` method is often used for error handling. It returns the contained `Ok` value for a `Result` or `Some` value for an `Option`. If an error occurs, it calls `panic!` with a provided error message. + +## Why is this bad? + +`.expect()` might panic if the result value is an error or `None`. It is recommended to avoid the panic of a contract 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. + +## Issue example + +Consider the following snippet code: + +```rust +pub fn balance_of(env: Env, owner: Address) -> i128 { + let state = Self::get_state(env); + state.balances.get(owner).expect("could not get balance") +} +``` + +In this contract, the `balance_of` function uses the expect method to retrieve the balance of an account. If there is no entry for this account in the balances mapping, the contract will panic and halt execution, which could be exploited maliciously to disrupt the contract's operation. + +## Remediated example + +Instead of using `expect`, use a safer method for error handling. In this case, if there is no entry for an account in the `balances` mapping, return a default value (like `0`). + +```rust +pub fn balance_of(env: Env, owner: Address) -> i128 { + let state = Self::get_state(env); + state.balances.get(owner).unwrap_or(0) +} +``` + +## How is it detected? + +Checks for usage of `.expect()`. diff --git a/docs/docs/detectors/rust/unsafe-unwrap.md b/docs/docs/detectors/rust/unsafe-unwrap.md new file mode 100644 index 00000000..5f9af415 --- /dev/null +++ b/docs/docs/detectors/rust/unsafe-unwrap.md @@ -0,0 +1,61 @@ +# Unsafe unwrap + +## Description + +- Category: `Validations and error handling` +- Severity: `Minor` +- Detectors: [`unsafe-unwrap`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/unsafe-unwrap/src/lib.rs) + +In Rust, the `unwrap` method is commonly used for error handling. It retrieves the inner value of an `Option` or `Result`. If an error or `None` occurs, it calls `panic!` without a custom error message. + +## Why is this bad? + +`.unwrap()` might panic if the result value is an error or `None`. It is recommended to avoid the panic of a pallet because it stops its execution, which might lead the pallets to an inconsistent state if the panic occurs in the middle of state changes. Additionally, the panic could cause a transaction to fail. + +## Issue example + +Consider the following `Substrate pallet`: + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let value = ExampleStorage::::get().unwrap(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +In this example, the `unsafe_get_storage` function uses the `unwrap` method to save the result of the `ExampleStorage` struct. If the function returns `Err`, the contract will panic and halt execution, potentially leading to malicious exploitation to disrupt the contract's operation. + +## Remediated example + +Instead of using `unwrap`, use a safer method for error handling like `unwrap_or_default`, or ensure that `.get()` is always `some` by adding a conditional. + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let value = ExampleStorage::::get().unwrap_or_default(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::get(); + if example_storage.is_none() { + return Err(Error::::NotInitialized.into()); + } + let value = example_storage.unwrap(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +## How is it detected? + +Checks for usage of .unwrap() diff --git a/docs/docs/detectors/substrate/assert-violation.md b/docs/docs/detectors/substrate/assert-violation.md new file mode 100644 index 00000000..effe975d --- /dev/null +++ b/docs/docs/detectors/substrate/assert-violation.md @@ -0,0 +1,50 @@ +# 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 substrate pallet: + +```rust +pub fn unsafe_check_balance(origin: OriginFor, amount: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + assert!( + BalanceStorage::::get().unwrap_or(0) >= amount, + "Insufficient balance" + ); + + Self::deposit_event(Event::BalanceChecked { who, amount }); + Ok(()) +} +``` + +## Remediated example + +Consider using a proper error and return it: + +```rust +pub fn unsafe_check_balance(origin: OriginFor, amount: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + if BalanceStorage::::get().unwrap_or(0) < amount { + return Err(Error::::InvalidBalance.into()); + } + + Self::deposit_event(Event::BalanceChecked { who, amount }); + Ok(()) +} +``` + +## How is it detected? + +Checks for `assert!` macro usage. diff --git a/docs/docs/detectors/substrate/avoid-debug-info.md b/docs/docs/detectors/substrate/avoid-debug-info.md index 3676e783..854256ae 100644 --- a/docs/docs/detectors/substrate/avoid-debug-info.md +++ b/docs/docs/detectors/substrate/avoid-debug-info.md @@ -4,8 +4,7 @@ - Category: `Best Practices` - Severity: `Minor` -- Detectors: [`avoid-debug-info`](https://github.com/CoinFabrik/scout-audit/tree/develop/detectors/substrate-pallets/avoid-debug-info) -- Test Cases: [`avoid-debug-info-1`](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/avoid-debug-info) +- Detectors: [`avoid-debug-info`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/substrate-pallets/avoid-debug-info) The use of debugging macros, such as `debug!()` and `info!()`, is useful during development and testing; however, these macros are not recommended for production and are considered a bad practice. Additionally, each operation that stores data in memory requires the virtual machine to perform additional work, which increases the gas costs needed for the transaction. Instead, consider using events emitting to log relevant data more efficiently and reduce unnecessary gas costs. @@ -36,7 +35,7 @@ pub fn unsafe_check_value(origin: OriginFor, threshold: u32) -> DispatchResul The `unsafe_check_value` function logs data and provides a suggestion using `debug!()` and `info!()` macros. These macros, while helpful during development, are inefficient in production because they increase gas costs due to unnecessary resource consumption. -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/avoid-debug-info/vulnerable/vulnerable-1). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/avoid-debug-info/vulnerable/vulnerable-1). ## Remediation @@ -64,4 +63,4 @@ pub fn unsafe_check_value(origin: OriginFor, threshold: u32) -> DispatchResul } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/avoid-debug-info/remediated/remediated-1). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/avoid-debug-info/remediated/remediated-1). diff --git a/docs/docs/detectors/substrate/avoid-panic-error.md b/docs/docs/detectors/substrate/avoid-panic-error.md new file mode 100644 index 00000000..67794d77 --- /dev/null +++ b/docs/docs/detectors/substrate/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 `Substrate pallet`: + +```rust +pub fn unsafe_check_value(origin: OriginFor, threshold: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + + let stored_value = Value::::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, threshold: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + + let stored_value = Value::::get().unwrap_or_default(); + if stored_value < threshold { + return Err(Error::::ValueTooLow.into()); + } + + Self::deposit_event(Event::ValueChecked { + who, + value: stored_value, + }); + Ok(()) + } +``` + +## How is it detected? + +Checks the use of the macro `panic!`. diff --git a/docs/docs/detectors/substrate/avoid-unsafe-block.md b/docs/docs/detectors/substrate/avoid-unsafe-block.md new file mode 100644 index 00000000..5b9bd196 --- /dev/null +++ b/docs/docs/detectors/substrate/avoid-unsafe-block.md @@ -0,0 +1,63 @@ +# 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 `substrate pallet`: + +```rust +#[pallet::call_index(0)] + pub fn process_data(origin: OriginFor, input: u8) -> DispatchResult { + let who = ensure_signed(origin)?; + + let result = unsafe { + let ptr: *const u8 = &input; + let value = *ptr; + value.rotate_left(2).wrapping_add(1) + }; + + DataStorage::::set(result); + + Self::deposit_event(Event::DataProcessed { who, value: result }); + + Ok(()) + } +``` + +In this example we can see that it creates a raw pointer named `ptr`. Then `value` dereferences the raw pointer. This directly accesses the memory location and calls the `rotate_left` 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 input, dereferencing the pointer could lead to a memory access violations or undefined behavior. + +## 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 +#[pallet::call_index(0)] + pub fn process_data(origin: OriginFor, input: u8) -> DispatchResult { + let who = ensure_signed(origin)?; + + let result = input.rotate_left(2).wrapping_add(1); + + DataStorage::::set(result); + + Self::deposit_event(Event::DataProcessed { who, value: result }); + + Ok(()) + } +``` + +## How is it detected? + +Checks for usage of `unsafe` blocks. diff --git a/docs/docs/detectors/substrate/divide-before-multiply.md b/docs/docs/detectors/substrate/divide-before-multiply.md new file mode 100644 index 00000000..2dfd0e20 --- /dev/null +++ b/docs/docs/detectors/substrate/divide-before-multiply.md @@ -0,0 +1,81 @@ +# 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 `Substrate pallet`: + +```rust +#[pallet::call_index(0)] + pub fn accumulate_dummy( + origin: OriginFor, + increase_by: T::Balance, + numerator: T::Balance, + denominator: T::Balance, + ) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + >::mutate(|dummy| { + let new_dummy = dummy.map_or(increase_by, |d| { + d.saturating_add(increase_by / denominator * numerator) + }); + *dummy = Some(new_dummy); + }); + + Self::deposit_event(Event::AccumulateDummy { + balance: increase_by, + }); + + Ok(()) + } +``` + +In this contract, the `accumulate_dummy` function creates a Dummy struct array by adding each element with the calculation of `increase_by / denominator * numerator`. This last calculation divides the `increase_by` by `denominator` before multiplying it with `numerator`. This could lead to a loss of precision if `increase_by` is less than `denominator` 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 +#[pallet::call_index(0)] + pub fn accumulate_dummy( + origin: OriginFor, + increase_by: T::Balance, + numerator: T::Balance, + denominator: T::Balance, + ) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + >::mutate(|dummy| { + let new_dummy = dummy.map_or(increase_by, |d| { + d.saturating_add(increase_by * numerator / denominator) + }); + *dummy = Some(new_dummy); + }); + + Self::deposit_event(Event::AccumulateDummy { + balance: increase_by, + }); + + Ok(()) + } +``` + +## 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) diff --git a/docs/docs/detectors/substrate/empty-expect.md b/docs/docs/detectors/substrate/empty-expect.md index 5a36f243..5346afc0 100644 --- a/docs/docs/detectors/substrate/empty-expect.md +++ b/docs/docs/detectors/substrate/empty-expect.md @@ -4,8 +4,8 @@ - Category: `Best Practices` - Severity: `Medium` -- Detectors: [`empty-expect`](https://github.com/CoinFabrik/scout-audit/tree/develop/detectors/rust/empty-expect) -- Test Cases: [`empty-expect-1`](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/empty-expect) +- Detectors: [`empty-expect`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/rust/empty-expect) +- Test Cases: [`empty-expect-1`](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/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. @@ -28,7 +28,7 @@ pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { 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/develop/test-cases/substrate-pallets/empty-expect/vulnerable/vulnerable-1). +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 @@ -47,4 +47,4 @@ pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/empty-expect/remediated/remediated-1). +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). diff --git a/docs/docs/detectors/substrate/equal-addresses.md b/docs/docs/detectors/substrate/equal-addresses.md index c4a847e4..c6023400 100644 --- a/docs/docs/detectors/substrate/equal-addresses.md +++ b/docs/docs/detectors/substrate/equal-addresses.md @@ -4,8 +4,7 @@ - Category: `Error Handling` - Severity: `Minor` -- Detectors: equal-addresses -- Test Cases: equal-addresses-1 equal-addresses-2 +- Detectors: [equal-addresses](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/equal-addresses/src/lib.rs) Functions that receive two addresses as parameters should include a check to ensure they are not the same. This ensures that the addresses represent distinct entities within the system diff --git a/docs/docs/detectors/substrate/incorrect-exponentiation.md b/docs/docs/detectors/substrate/incorrect-exponentiation.md new file mode 100644 index 00000000..1d864432 --- /dev/null +++ b/docs/docs/detectors/substrate/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, new_value: u32) -> DispatchResult { + let who = ensure_signed(origin)?; + let calculated_value = new_value ^ 3; + Value::::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, new_value: u32) -> DispatchResult { +let who = ensure_signed(origin)?; +let calculated_value = new_value.pow(3); +Value::::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. diff --git a/docs/docs/detectors/substrate/integer-overflow-or-underflow.md b/docs/docs/detectors/substrate/integer-overflow-or-underflow.md new file mode 100644 index 00000000..07b9db09 --- /dev/null +++ b/docs/docs/detectors/substrate/integer-overflow-or-underflow.md @@ -0,0 +1,59 @@ +# Integer overflow or underflow + +## Description + +- Category: `Arithmetic` +- Severity: `Critical` +- Detectors: [`integer-overflow-or-underflow`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/integer-overflow-or-underflow/src/lib.rs) + +In Rust, arithmetic operations can result in a value that falls outside the allowed numerical range for a given type. When the result exceeds the maximum value of the range, it's called an overflow, and when it falls below the minimum value of the range, it's called an underflow. + +## Why is this bad? + +If there are arithmetic operations with overflow or underflow problems, and if errors are not handled correctly, incorrect results will be generated, bringing potential problems for the contract. Additionally, these types of errors can allow attackers to drain a contract’s funds or manipulate its logic. + +## Issue example + +Consider the following `Substrate pallet`: + +```rust +#[pallet::call_index(0)] + pub fn accumulate_dummy(origin: OriginFor, increase_by: T::Balance) -> DispatchResult { + let _sender = ensure_signed(origin)?; + >::mutate(|dummy| { + let new_dummy = dummy.map_or(increase_by, |d| d + increase_by); + *dummy = Some(new_dummy); + }); + Self::deposit_event(Event::AccumulateDummy { + balance: increase_by, + }); + Ok(()) + } +``` + +In this example, an operation is performed on two u32 (`d` and `increase_by`) values without any safeguards against overflow if it occurs. + +## Remediated example + +Consider using safe operations to prevent an overflow + +```rust +#[pallet::call_index(0)] + pub fn accumulate_dummy(origin: OriginFor, increase_by: T::Balance) -> DispatchResult { + let _sender = ensure_signed(origin)?; + >::mutate(|dummy| { + let new_dummy = dummy.map_or(increase_by, |d| d.saturating_add(increase_by)); + *dummy = Some(new_dummy); + }); + Self::deposit_event(Event::AccumulateDummy { + balance: increase_by, + }); + Ok(()) + } +``` + +In this example, the `saturating_add` method is used to perform the addition. It returns the sum if no overflow occurs; otherwise, it returns `None`, with an OverflowError variant indicating that an overflow error has occurred. + +## How is it detected? + +Checks if there’s any numerical overflow or underflow. diff --git a/docs/docs/detectors/substrate/invalid-extrinsic-weight.md b/docs/docs/detectors/substrate/invalid-extrinsic-weight.md index b709b009..aa9a6c67 100644 --- a/docs/docs/detectors/substrate/invalid-extrinsic-weight.md +++ b/docs/docs/detectors/substrate/invalid-extrinsic-weight.md @@ -4,8 +4,8 @@ - Category: `Known Bugs` - Severity: `Enhancement` -- Detectors: [`invalid-extrinsic-weight`](https://github.com/CoinFabrik/scout-audit/tree/develop/detectors/substrate-pallets/invalid-extrinsic-weight) -- Test Cases: [`invalid-extrinsic-weight-1`](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/invalid-extrinsic-weight) +- Detectors: [`invalid-extrinsic-weight`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/substrate-pallets/invalid-extrinsic-weight) +- Test Cases: [`invalid-extrinsic-weight-1`](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/invalid-extrinsic-weight) The weight attribute is using a weight calculation function that doesn't match the extrinsic name. Each extrinsic must have its own dedicated weight calculation to accurately reflect its resource consumption. Reusing weight calculations from other functions can lead to incorrect resource estimation and potential issues in production. @@ -30,7 +30,7 @@ impl Pallet { In the provided implementation, `another_dummy_call` reuses the weight calculation function intended for another context. By not having a unique weight definition, this extrinsic introduces vulnerabilities into the system. Specifically, reusing weight functions may result in underestimating or overestimating resource consumption, leaving the network susceptible to Denial-of-Service (DoS) attacks. -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/invalid-extrinsic-weight/vulnerable/vulnerable-1). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/invalid-extrinsic-weight/vulnerable/vulnerable-1). ## Remediation @@ -52,4 +52,4 @@ impl Pallet { } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/invalid-extrinsic-weight/vulnerable/vulnerable-1). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/invalid-extrinsic-weight/vulnerable/vulnerable-1). diff --git a/docs/docs/detectors/substrate/iterators-over-indexing.md b/docs/docs/detectors/substrate/iterators-over-indexing.md new file mode 100644 index 00000000..fce62617 --- /dev/null +++ b/docs/docs/detectors/substrate/iterators-over-indexing.md @@ -0,0 +1,68 @@ +# Iterators over indexing + +## Description + +- Issue Category: `Arithmetic` +- Issue Severity: `Medium` +- Detectors: [`iterators-over-indexing`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/iterators-over-indexing/src/lib.rs) + +It warns if a `for` loop uses indexing instead of an iterator. If the indexing explicitly ends at `.len()`, the lint will not trigger. + +### Why is this bad? + +Accessing a vector by index is slower than using an iterator. This is because iterators are optimized for sequential access, avoiding repeated bounds-checking at runtime. Also, if the index is out of bounds it will cause the program to panic, potentially leading to runtime errors + +### Issue Example + +Consider the following substrate pallet: + +```rust +#[pallet::call_index(1)] +pub fn set_sum(origin: OriginFor) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + let mut new_sum = 0_u32; + + if let Some(v) = >::get() { + for i in 0..128 { + new_sum += v[i]; + } + } + + >::mutate(|sum| { + *sum = Some(new_sum); + }); + + Ok(()) +} +``` + +The provided function contains a for loop that uses indexing to access elements of the v vector. + +### Remediated example + +Consider using an iterator to iterate over the array + +```rust +pub fn set_sum(origin: OriginFor) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + let mut new_sum = 0; + + if let Some(v) = >::get() { + for i in v.iter() { + new_sum += i; + } + } + + >::mutate(|sum| { + *sum = Some(new_sum); + }); + + Ok(()) + } +``` + +## How is it detected? + +Find expressions inside the function that call some method that has te name `get` and checks how the for loop is used, if it's used without an iterator, thorws a warning diff --git a/docs/docs/detectors/substrate/overflow-check.md b/docs/docs/detectors/substrate/overflow-check.md new file mode 100644 index 00000000..97b8f03d --- /dev/null +++ b/docs/docs/detectors/substrate/overflow-check.md @@ -0,0 +1,109 @@ +# Overflow-check + +## Description + +- Category: `Arithmetic` +- Severity: `Critical` +- Detectors: [`overflow-check`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/rust/overflow-check/src/lib.rs) + +Checks that `overflow-checks` is enabled in the `[profile.release]` section of the `Cargo.toml`. + +### Why is this bad? + +Integer overflow will trigger a panic in debug builds or will wrap in +release mode. Division by zero will cause a panic in either mode. In some applications one +wants explicitly checked, wrapping or saturating arithmetic. + +### Example + +```toml + +[package] +edition = "2021" +name = "overflow-check-vulnerable-1" +version = "0.1.0" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +log = { workspace = true } +pallet-balances = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] + +``` + +Use instead: + +```toml + +[package] +edition = "2021" +name = "overflow-check-remediated-1" +version = "0.1.0" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +log = { workspace = true } +pallet-balances = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = false +panic = "abort" +strip = "symbols" +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check). diff --git a/docs/docs/detectors/substrate/unsafe-expect.md b/docs/docs/detectors/substrate/unsafe-expect.md new file mode 100644 index 00000000..059c96f1 --- /dev/null +++ b/docs/docs/detectors/substrate/unsafe-expect.md @@ -0,0 +1,64 @@ +# Unsafe expect + +## Description + +- Category: `Validations and error handling` +- Severity: `Minor` +- Detectors: [`unsafe-expect`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/unsafe-expect/src/lib.rs) + +In Rust, the `expect` method is often used for error handling. It returns the contained `Ok` value for a `Result` or `Some` value for an `Option`. If an error occurs, it calls `panic!` with a provided error message. + +## Why is this bad? + +`.expect()` might panic if the result value is an error or `None`. It is recommended to avoid the panic of a contract 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. + +## Issue example + +Consider the following `Substrate Pallet`: + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let value = ExampleStorage::::get().expect("Storage is not initialized"); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +In this contract, the `unsafe_get_storage` function uses the expect method to retrieve the storage. If the storage is not initialized, the contract will panic and halt execution, which could be exploited maliciously to disrupt the pallet's operation. + +## Remediated example + +Ensure that expect won't fail before using it. You can achieve this by calling expect inside an if statement that checks if `example_storage` is `Some`, or by returning an error if it is `None`. + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::get(); + if example_storage.is_some() { + let value = example_storage.expect("Storage is not initialized"); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + } + Ok(()) + } +``` + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::get(); + if example_storage.is_none() { + return Err(Error::::NotInitialized.into()); + } + let value = example_storage.expect("Storage is not initialized"); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +## How is it detected? + +Checks for usage of `.expect()`. diff --git a/docs/docs/detectors/substrate/unsafe-unwrap.md b/docs/docs/detectors/substrate/unsafe-unwrap.md new file mode 100644 index 00000000..5f9af415 --- /dev/null +++ b/docs/docs/detectors/substrate/unsafe-unwrap.md @@ -0,0 +1,61 @@ +# Unsafe unwrap + +## Description + +- Category: `Validations and error handling` +- Severity: `Minor` +- Detectors: [`unsafe-unwrap`](https://github.com/CoinFabrik/scout-audit/blob/main/detectors/substrate-pallets/unsafe-unwrap/src/lib.rs) + +In Rust, the `unwrap` method is commonly used for error handling. It retrieves the inner value of an `Option` or `Result`. If an error or `None` occurs, it calls `panic!` without a custom error message. + +## Why is this bad? + +`.unwrap()` might panic if the result value is an error or `None`. It is recommended to avoid the panic of a pallet because it stops its execution, which might lead the pallets to an inconsistent state if the panic occurs in the middle of state changes. Additionally, the panic could cause a transaction to fail. + +## Issue example + +Consider the following `Substrate pallet`: + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let value = ExampleStorage::::get().unwrap(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +In this example, the `unsafe_get_storage` function uses the `unwrap` method to save the result of the `ExampleStorage` struct. If the function returns `Err`, the contract will panic and halt execution, potentially leading to malicious exploitation to disrupt the contract's operation. + +## Remediated example + +Instead of using `unwrap`, use a safer method for error handling like `unwrap_or_default`, or ensure that `.get()` is always `some` by adding a conditional. + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let value = ExampleStorage::::get().unwrap_or_default(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +```rust +#[pallet::call_index(0)] + pub fn unsafe_get_storage(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let example_storage = ExampleStorage::::get(); + if example_storage.is_none() { + return Err(Error::::NotInitialized.into()); + } + let value = example_storage.unwrap(); + Self::deposit_event(Event::UnsafeGetStorage { who, value }); + Ok(()) + } +``` + +## How is it detected? + +Checks for usage of .unwrap() diff --git a/docs/docs/detectors/substrate/unsigned-extrinsic.md b/docs/docs/detectors/substrate/unsigned-extrinsic.md index a6063e6e..28c8694a 100644 --- a/docs/docs/detectors/substrate/unsigned-extrinsic.md +++ b/docs/docs/detectors/substrate/unsigned-extrinsic.md @@ -4,8 +4,8 @@ - Category: `DoS` - Severity: `Critical` -- Detectors: [`unsigned-extrinsic`](https://github.com/CoinFabrik/scout-audit/tree/develop/detectors/substrate-pallets/unsigned-extrinsic) -- Test Cases: [`unsigned-extrinsic-1`](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/unsigned-extrinsic) +- Detectors: [`unsigned-extrinsic`](https://github.com/CoinFabrik/scout-audit/tree/main/detectors/substrate-pallets/unsigned-extrinsic) +- Test Cases: [`unsigned-extrinsic-1`](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/unsigned-extrinsic) Unsigned extrinsics allow transactions to be submitted without any associated fees or signatures. This can be exploited by malicious actors to flood the network with transactions at no cost, potentially causing denial of service. Consider using signed extrinsics with appropriate fee mechanisms unless there's a specific security reason for allowing unsigned transactions. @@ -27,7 +27,7 @@ impl Pallet { The `unsafe_call` function uses `ensure_none(origin)?`, which allows only unsigned transactions (i.e., where the origin is `None`). While this might seem harmless for internal runtime logic, it becomes problematic if this function is inadvertently exposed or misused. -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1). ## Remediation @@ -45,4 +45,4 @@ impl Pallet { } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/develop/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-audit/tree/main/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1). From 48cc8ec46081d2b8308e2d673a00d5bc525ed258 Mon Sep 17 00:00:00 2001 From: Matias Cabello Date: Wed, 22 Jan 2025 14:30:18 -0300 Subject: [PATCH 2/3] added rust category in documentation sidebar --- docs/sidebars.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/sidebars.js b/docs/sidebars.js index d4c65f27..159c2f8d 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -65,6 +65,16 @@ const sidebars = { dirName: 'detectors/substrate' }, ] + }, + { + type: 'category', + label: 'Rust', + items: [ + { + type: 'autogenerated', + dirName: 'detectors/rust' + }, + ] } ] }, From 9444ae57bcf6c66bd760cf9a28e00be2f2807807 Mon Sep 17 00:00:00 2001 From: Matias Cabello Date: Wed, 22 Jan 2025 14:54:00 -0300 Subject: [PATCH 3/3] added auto generated categories index page --- docs/sidebars.js | 22 +++++++++++++++++++++- docs/src/css/custom.css | 4 ++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/sidebars.js b/docs/sidebars.js index 159c2f8d..2765a34f 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -38,7 +38,12 @@ const sidebars = { items: [ { type: 'category', - label: 'ink!', + label: 'Ink', + link: { + type: 'generated-index', + title: 'Ink detectors', + description: 'Issues and detectors for Polkadot\'s Ink smart contract language.' + }, items: [ { type: 'autogenerated', @@ -49,6 +54,11 @@ const sidebars = { { type: 'category', label: 'Soroban', + link: { + type: 'generated-index', + title: 'Soroban detectors', + description: 'Issues and detectors for Stellar\'s Soroban smart contract language.' + }, items: [ { type: 'autogenerated', @@ -59,6 +69,11 @@ const sidebars = { { type: 'category', label: 'Substrate Pallets', + link: { + type: 'generated-index', + title: 'Substrate detectors', + description: 'Issues and detectors for Polkadot\'s Substrate pallets.' + }, items: [ { type: 'autogenerated', @@ -69,6 +84,11 @@ const sidebars = { { type: 'category', label: 'Rust', + link: { + type: 'generated-index', + title: 'Rust detectors', + description: 'General Rust language issues and detectors.' + }, items: [ { type: 'autogenerated', diff --git a/docs/src/css/custom.css b/docs/src/css/custom.css index 240db0ed..630748be 100644 --- a/docs/src/css/custom.css +++ b/docs/src/css/custom.css @@ -28,3 +28,7 @@ --ifm-color-primary-lightest: #4fddbf; --docusaurus-highlighted-code-line-bg: rgba(0, 0, 0, 0.3); } + +.cardDescription_PWke { + display: none; +} \ No newline at end of file