From 692d1a0960f5434d693f597ecd8d988df8d3b591 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 08:12:30 -0300 Subject: [PATCH 01/13] Add avoid-panic-error --- test-cases/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index 0cc8c7bc..d229d5b2 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -146,3 +146,12 @@ We classified this type of vulnerability under the [Authorization](#vulnerability-categories) category and assigned it a Critical severity. +### Avoid panic error + +The use of the `panic!` macro to stop execution when a condition is not met is +useful for testing and prototyping but should be avoided in production code. +Using `Result` as the return type for functions that can fail is the idiomatic +way to handle errors in Rust. + +We classified this issue, a deviation from best practices which could have +security implications, under the [Validations and error handling](#vulnerability-categories) category and assigned it an Enhancement severity. From 5b1a1859f56a51ee7127d7ef42769b0bf0185817 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 08:58:02 -0300 Subject: [PATCH 02/13] Add avoid-panic-error --- detectors/avoid-panic-error/src/README.md | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 detectors/avoid-panic-error/src/README.md diff --git a/detectors/avoid-panic-error/src/README.md b/detectors/avoid-panic-error/src/README.md new file mode 100644 index 00000000..89ed51fb --- /dev/null +++ b/detectors/avoid-panic-error/src/README.md @@ -0,0 +1,50 @@ +# 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/panic-error). From de64fb63edab4cd6e9e6c86ffad9924a60915268 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:05:36 -0300 Subject: [PATCH 03/13] Add avoid-panic-error and severity for avoid-core-mem-forget --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d7fe6efe..41e1db22 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,9 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check) | An arithmetic operation overflows or underflows the available memory allocated to the variable. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1)| Critical | | [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values) | Avoid using block attributes for random number generation to prevent manipulation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)| Critical | | [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1)| Critical | -| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) +| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) | Enhancement | | [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | +| [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | ## CLI Options From e6c4bb539c8e2cb73be5475de11d257cdf039dbe Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:13:34 -0300 Subject: [PATCH 04/13] Add avoid-unsafe-block --- test-cases/README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index d229d5b2..6720a0f9 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -155,3 +155,11 @@ way to handle errors in Rust. We classified this issue, a deviation from best practices which could have security implications, under the [Validations and error handling](#vulnerability-categories) category and assigned it an Enhancement severity. + +### Avoid unsafe block + +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. This can lead to various issues, such as undefined behavior, memory leaks, or security vulnerabilities. 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. + +We classified this issue, a deviation from best practices which could have +security implications, under the [Validations and error handling](#vulnerability-categories) category and assigned it a Critical severity. + From ac2b8f4da7747c6f29fbf95c991618f9de07760c Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:21:28 -0300 Subject: [PATCH 05/13] Update link to detector --- detectors/avoid-panic-error/src/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detectors/avoid-panic-error/src/README.md b/detectors/avoid-panic-error/src/README.md index 89ed51fb..e7076db6 100644 --- a/detectors/avoid-panic-error/src/README.md +++ b/detectors/avoid-panic-error/src/README.md @@ -47,4 +47,4 @@ pub fn add(env: Env, value: u32) -> Result { ### Implementation -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/panic-error). +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error). From b4048960d8bb03c739336d72970408f9c57624cb Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:22:04 -0300 Subject: [PATCH 06/13] Add avoid-unsafe-block --- detectors/avoid-unsafe-block/README.md | 47 ++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 detectors/avoid-unsafe-block/README.md diff --git a/detectors/avoid-unsafe-block/README.md b/detectors/avoid-unsafe-block/README.md new file mode 100644 index 00000000..f7fb1b3b --- /dev/null +++ b/detectors/avoid-unsafe-block/README.md @@ -0,0 +1,47 @@ +# Avoid unsafe block + +### What it does + +Checks for usage of `unsafe` blocks. + +### 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. + +### Example + + ```rust +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; + let result = *result_ptr; + + result.to_bits() + } +} +``` + +Use instead: + + ```rust +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; + result.to_bits() +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block). From e86b9df48a49ec5f9542dc2595f1a4fadcd6c845 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:39:35 -0300 Subject: [PATCH 07/13] Add avoid-unsafe-block --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 41e1db22..e7494f33 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) | Enhancement | | [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | | [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | +| [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | ## CLI Options From 958325c9c6ab975f293c0e919ae4d0af01ca76ed Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 09:44:33 -0300 Subject: [PATCH 08/13] Add dos-unbounded-operation --- test-cases/README.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index 6720a0f9..d83a4a21 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -163,3 +163,24 @@ The use of `unsafe` blocks in Rust is generally discouraged due to the potential We classified this issue, a deviation from best practices which could have security implications, under the [Validations and error handling](#vulnerability-categories) category and assigned it a Critical severity. +### DoS unbounded operation + +Each block in Soroban Stellar has an upper bound on the amount of gas +that can be spent, and thus the amount of computation that can be done. This +is the Block Gas Limit. If the gas spent by a function call on a Soroban smart +contract exceeds this limit, the transaction will fail. Sometimes it is the +case that the contract logic allows a malicious user to modify conditions +so that other users are forced to exhaust gas on standard function calls. + +In order to prevent a single transaction from consuming all the gas in a block, +unbounded operations must be avoided. This includes loops that do not have a +bounded number of iterations, and recursive calls. + +A denial of service vulnerability allows the exploiter to hamper the +availability of a service rendered by the smart contract. In the context +of Soroban smart contracts, it can be caused by the exhaustion of gas, +storage space, or other failures in the contract's logic. + +We classified this type of vulnerability under +the [Denial of Service](#vulnerability-categories) category and assigned it a +Medium severity. From c13b35c5deb7afead6a6b9e666bc0b7692edfea4 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 10:02:24 -0300 Subject: [PATCH 09/13] Add dos-unbounded-opereation --- detectors/dos-unbounded-operation/README.md | 43 +++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 detectors/dos-unbounded-operation/README.md diff --git a/detectors/dos-unbounded-operation/README.md b/detectors/dos-unbounded-operation/README.md new file mode 100644 index 00000000..214b59dc --- /dev/null +++ b/detectors/dos-unbounded-operation/README.md @@ -0,0 +1,43 @@ +# DoS unbounded operation + +### What it does + +This detector checks that when using for or while loops, their conditions limit the execution to a constant number of iterations. + +### Why is this bad? + +If the number of iterations is not limited to a specific range, it could potentially cause out of gas exceptions. + +### Known problems + +False positives are to be expected when using variables that can only be set using controlled flows that limit the values within acceptable ranges. + +### Example + +```rust +pub fn unrestricted_loop(for_loop_count: u64) -> u64 { + let mut count = 0; + for i in 0..for_loop_count { + count += i; + } + count +} +``` + +Use instead: + +```rust +const FIXED_COUNT: u64 = 1000; + +pub fn restricted_loop_with_const() -> u64 { + let mut sum = 0; + for i in 0..FIXED_COUNT { + sum += i; + } + sum +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation). From 9e1f447448e5539ac07ed724c93ebb73017c41ca Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 10:09:11 -0300 Subject: [PATCH 10/13] Add dos-unbounded-operation --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index e7494f33..3a8e39c8 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | | [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | | [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | +| [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 | ## CLI Options From be1dccb5959d56f341ac8f4cb5eed94c20ae481c Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 10:38:09 -0300 Subject: [PATCH 11/13] Add soroban-version --- test-cases/README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index d83a4a21..d9ef6fd2 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -184,3 +184,10 @@ storage space, or other failures in the contract's logic. We classified this type of vulnerability under the [Denial of Service](#vulnerability-categories) category and assigned it a Medium severity. + +### Soroban version + +Using a pinned version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. + +We classified this issue, a deviation from best practices which could have +security implications, under the [Best practices](#vulnerability-categories) category and assigned it an Enhancement severity. From cd77273fbc164fae7ae460c7fbb3119c33203db1 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 13:04:10 -0300 Subject: [PATCH 12/13] Add soroban-version --- detectors/soroban-version/README.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 detectors/soroban-version/README.md diff --git a/detectors/soroban-version/README.md b/detectors/soroban-version/README.md new file mode 100644 index 00000000..e2e340bc --- /dev/null +++ b/detectors/soroban-version/README.md @@ -0,0 +1,25 @@ +# Soroban version + +### What it does + +Warns you if you are using an old version of Soroban in the `Cargo.toml`. + +### Why is this bad? + +Using an old version of Soroban can be dangerous, as it may have bugs or security issues. + +### Example + +```toml +[dependencies] +soroban-sdk = { version = "=20.0.0" } + +[dev_dependencies] +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +``` + +Instead, use the latest available version in the `Cargo.toml`. + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version). From 99f05e9b2a583b7b35dfb52c4a6b6715910587ed Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 28 Dec 2023 13:08:24 -0300 Subject: [PATCH 13/13] Add soroban-version --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 3a8e39c8..c8545ed7 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | | [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | | [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 a pinned 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 | ## CLI Options