From aa7ecaa63cb5722d9b4bd97526ac62b782cd3beb Mon Sep 17 00:00:00 2001 From: user Date: Tue, 16 Apr 2024 10:01:49 -0300 Subject: [PATCH 1/2] Add assert violation documentation in docusaurs and table --- README.md | 2 ++ docs/docs/detectors/15-assert-violation.md | 33 ++++++++++++++++++++++ docs/docs/vulnerabilities/README.md | 8 +++++- 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 docs/docs/detectors/15-assert-violation.md diff --git a/README.md b/README.md index f9d11bef..f240985e 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,8 @@ Visit [Scout's website](https://coinfabrik.github.io/scout-soroban/) to view the | [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation) | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3) | Medium | | [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) | Using an old version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) | Enhancement | | [unused-return-enum](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum) | Return enum from a function is not completely used. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) | Minor | +| [assert-violation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation) | Avoid the usage of the macro assert!, it can panic.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1) | Enhancement | + ## CLI Options diff --git a/docs/docs/detectors/15-assert-violation.md b/docs/docs/detectors/15-assert-violation.md new file mode 100644 index 00000000..09ca76c7 --- /dev/null +++ b/docs/docs/detectors/15-assert-violation.md @@ -0,0 +1,33 @@ +# Assert violation + +### What it does​ + +Checks for `assert!` macro usage. + +### Why is this bad?​ + +The `assert!` macro can cause the contract to panic. + +### Example​ + +```rust +pub fn assert_if_greater_than_10(_env: Env, value: u128) -> bool { + assert!(value <= 10, "value should be less than 10"); + true + } + +``` +Use instead: + +```rust +pub fn assert_if_greater_than_10(_env: Env, value: u128) -> Result { + if value <= 10 { + Ok(true) + } else { + Err(AVError::GreaterThan10) + } + } +``` +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation). diff --git a/docs/docs/vulnerabilities/README.md b/docs/docs/vulnerabilities/README.md index a5b1f014..77450ca8 100644 --- a/docs/docs/vulnerabilities/README.md +++ b/docs/docs/vulnerabilities/README.md @@ -201,4 +201,10 @@ any of the variants is not used, the code could be simplified or it could imply a bug. We put this vulnerability under the [Validations and error handling category](#vulnerability-categories) -with a Minor Severity. \ No newline at end of file +with a Minor Severity. + +### Assert violation + +The assert! macro is used in Rust to ensure that a certain condition holds true at a certain point in your code. The `assert!` macro can cause the contract to panic. Therefore, the detector suggests replacing `assert!` constructs with `Error` enum structures. + +This vulnerability falls under the category [Validations and error handling](#vulnerability-categories) and has an Enhancement Severity. From 4af7e2e476a3e981085eb8611353ee10dbace8da Mon Sep 17 00:00:00 2001 From: user Date: Tue, 16 Apr 2024 10:08:59 -0300 Subject: [PATCH 2/2] Clean misplaced detectors documentations --- detectors/assert-violation/README.md | 0 detectors/avoid-panic-error/src/README.md | 50 ---------------- .../unprotected-mapping-operation/README.md | 60 ------------------- 3 files changed, 110 deletions(-) delete mode 100644 detectors/assert-violation/README.md delete mode 100644 detectors/avoid-panic-error/src/README.md delete mode 100644 detectors/unprotected-mapping-operation/README.md diff --git a/detectors/assert-violation/README.md b/detectors/assert-violation/README.md deleted file mode 100644 index e69de29b..00000000 diff --git a/detectors/avoid-panic-error/src/README.md b/detectors/avoid-panic-error/src/README.md deleted file mode 100644 index e7076db6..00000000 --- a/detectors/avoid-panic-error/src/README.md +++ /dev/null @@ -1,50 +0,0 @@ -# Panic error - -### What it does - -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. - -### Why is this bad? - -The usage of `panic!` is not recommended because it will stop the execution of the caller contract. - -### Known problems - -While this linter detects explicit calls to `panic!`, there are some ways to raise a panic such as `unwrap()` or `expect()`. - -### Example - -```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 -} -``` - -Use instead: - -```rust -pub fn add(env: Env, value: u32) -> Result { - 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 => return Err(Error::OverflowError), - } - storage.set(&COUNTER, &count); - storage.extend_ttl(100, 100); - Ok(count) -} -``` - -### Implementation - -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error). diff --git a/detectors/unprotected-mapping-operation/README.md b/detectors/unprotected-mapping-operation/README.md deleted file mode 100644 index e06f9710..00000000 --- a/detectors/unprotected-mapping-operation/README.md +++ /dev/null @@ -1,60 +0,0 @@ -# Unprotected Mapping Operation - -### What it does - -It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field of the type `AccountId`. - -### Why is this bad? - -Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons: - -- Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitation are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author. - -- Data Corruption: Malicious users could intentionally provide keys that result in the corruption or manipulation of important data stored in the mapping. This could lead to incorrect calculations, unauthorized access, or other undesirable outcomes. - -- Denial-of-Service (DoS) Attacks: If users can set arbitrary keys, they may be able to create mappings with a large number of entries, potentially causing the contract to exceed its gas limit. This could lead to denial-of-service attacks, making the contract unusable for other users. - -### Known problems - -### Example - -```rust - pub fn set_balance(env: Env, address: Address, balance: i128) -> State { - // Get the current state. - let mut state = Self::get_state(env.clone()); - - // Set the new account to have total supply if it doesn't exist. - if !state.balances.contains_key(address.clone()) { - state.balances.set(address, balance); - // Save the state. - env.storage().persistent().set(&STATE, &state); - } - - state - } -``` - -Use instead: - -```rust - pub fn set_balance(env: Env, address: Address, balance: i128) -> State { - // Authenticate user - address.require_auth(); - - // Get the current state. - let mut state = Self::get_state(env.clone()); - - // Set the new account to have total supply if it doesn't exist. - if !state.balances.contains_key(address.clone()) { - state.balances.set(address, balance); - // Save the state. - env.storage().persistent().set(&STATE, &state); - } - - state - } -``` - -### Implementation - -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation).