diff --git a/README.md b/README.md index ccc6ed32..61400fd5 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,9 @@ 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 | -[iterators-over-indexing](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing) |Iterating with hardcoded indexes is slower than using an iterator. Also, if the index is out of bounds, it will panic. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing-1), | Enhancement | +| [iterators-over-indexing](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing) |Iterating with hardcoded indexes is slower than using an iterator. Also, if the index is out of bounds, it will panic. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing-1) | Enhancement | +| [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 | + ## Tests 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). 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 fec66f6e..b0f07783 100644 --- a/docs/docs/vulnerabilities/README.md +++ b/docs/docs/vulnerabilities/README.md @@ -200,8 +200,7 @@ definition of the `Result` type enum consists of two variants: Ok and Err. If 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. +We put this vulnerability under the [Validations and error handling category](#vulnerability-categories) with a Minor severity. ### Iterators-over-indexing @@ -211,3 +210,10 @@ This could lead to potential integer overflow vulnerabilities, which would trigg This vulnerability falls under the [Best practices](#vulnerability-categories) category and has an Enhancement 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. + +