From e34dfa307f9c1ea9e4e0fa38fc9d0bd0cd90b97a Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 09:14:41 -0300 Subject: [PATCH 01/13] Add first test case --- .../remediated-example/Cargo.toml | 30 +++++++++++ .../remediated-example/src/lib.rs | 52 ++++++++++++++++++ .../vulnerable-example/Cargo.toml | 30 +++++++++++ .../vulnerable-example/src/lib.rs | 54 +++++++++++++++++++ 4 files changed, 166 insertions(+) create mode 100644 test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml create mode 100644 test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs create mode 100644 test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/src/lib.rs diff --git a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..75e353f4 --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "unused-return-enum-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=20.0.0" } + +[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] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..d439b144 --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs @@ -0,0 +1,52 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; + + match 100u128.checked_mul(absolute_difference / sum) { + Some(result) => Ok(result), + None => Err(Error::Overflow), + } + } +} + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{UnusedReturnEnum, UnusedReturnEnumClient}; + + #[test] + fn get_percentage_difference_works() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnusedReturnEnum); + let client = UnusedReturnEnumClient::new(&env, &contract_id); + + // When + let value1 = 100u128; + let value2 = 150u128; + let result = client.try_get_percentage_difference(&value1, &value2); + + // Then + assert_eq!(result, Ok(Ok(0))); + } +} diff --git a/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/Cargo.toml b/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..c90fa4dc --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "unused-return-enum-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=20.0.0" } + +[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] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..e4debe03 --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-1/vulnerable-example/src/lib.rs @@ -0,0 +1,54 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; + + match 100u128.checked_mul(absolute_difference / sum) { + Some(result) => result, + None => panic!("Overflow"), + }; + + Err(Error::Overflow) + } +} + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{Error, UnusedReturnEnum, UnusedReturnEnumClient}; + + #[test] + fn get_percentage_difference_panics() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnusedReturnEnum); + let client = UnusedReturnEnumClient::new(&env, &contract_id); + + // When + let value1 = 100u128; + let value2 = 150u128; + let result = client.try_get_percentage_difference(&value1, &value2); + + // Then + assert_eq!(result, Err(Ok(Error::Overflow))); + } +} From b08f02cba2431d86e79f6e73dd1de108c40315f7 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 09:37:33 -0300 Subject: [PATCH 02/13] Add second test case --- .../remediated-example/Cargo.toml | 30 ++++++++++ .../remediated-example/src/lib.rs | 51 ++++++++++++++++ .../vulnerable-example/Cargo.toml | 30 ++++++++++ .../vulnerable-example/src/lib.rs | 58 +++++++++++++++++++ 4 files changed, 169 insertions(+) create mode 100644 test-cases/unused-return-enum/unused-return-enum-2/remediated-example/Cargo.toml create mode 100644 test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs create mode 100644 test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/Cargo.toml create mode 100644 test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs diff --git a/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/Cargo.toml b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..1edbef17 --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "unused-return-enum-remediated-2" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=20.0.0" } + +[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] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..57d11f9a --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs @@ -0,0 +1,51 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = value1.abs_diff(value2); + let sum = value1 + value2; + let result = 100u128 + .checked_mul(absolute_difference / sum) + .ok_or(Error::Overflow)?; + Ok(result) + } +} + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{UnusedReturnEnum, UnusedReturnEnumClient}; + + #[test] + fn get_percentage_difference_works() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnusedReturnEnum); + let client = UnusedReturnEnumClient::new(&env, &contract_id); + + // When + let value1 = 100u128; + let value2 = 150u128; + let result = client.try_get_percentage_difference(&value1, &value2); + + // Then + assert_eq!(result, Ok(Ok(0))); + } +} diff --git a/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/Cargo.toml b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..aed3d0cc --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "unused-return-enum-vulnerable-2" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=20.0.0" } + +[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] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..42f91c2f --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs @@ -0,0 +1,58 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; + + let _ignore = 100u128 + .checked_mul(absolute_difference / sum) + .ok_or(Error::Overflow)?; + + match 100u128.checked_mul(absolute_difference / sum) { + Some(result) => result, + None => panic!("Overflow"), + }; + + Err(Error::Overflow) + } +} + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{Error, UnusedReturnEnum, UnusedReturnEnumClient}; + + #[test] + fn get_percentage_difference_panics() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnusedReturnEnum); + let client = UnusedReturnEnumClient::new(&env, &contract_id); + + // When + let value1 = 100u128; + let value2 = 150u128; + let result = client.try_get_percentage_difference(&value1, &value2); + + // Then + assert_eq!(result, Err(Ok(Error::Overflow))); + } +} From f43d01507d54ac00e0b670052748b2cbf6a4330b Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 09:39:37 -0300 Subject: [PATCH 03/13] Add detector to list --- scout-audit-internal/src/detector.rs | 2 ++ scout-audit-internal/src/detector/lint_message.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index b9b7a966..b6914bed 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -38,6 +38,7 @@ pub enum Detector { UnprotectedUpdateCurrentContractWasm, UnsafeExpect, UnsafeUnwrap, + UnusedReturnEnum, } impl Detector { @@ -58,6 +59,7 @@ impl Detector { Detector::AvoidUnsafeBlock => AVOID_UNSAFE_BLOCK_LINT_MESSAGE, Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE, + Detector::UnusedReturnEnum => UNUSED_RETURN_ENUM_LINT_MESSAGE, } } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index e4549536..4cf87b5c 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -16,3 +16,4 @@ pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE: &str = "This update_current_contract_wasm is called without access control"; pub const UNSAFE_EXPECT_LINT_MESSAGE: &str = "Unsafe usage of `expect`"; pub const UNSAFE_UNWRAP_LINT_MESSAGE: &str = "Unsafe usage of `unwrap`"; +pub const UNUSED_RETURN_ENUM_LINT_MESSAGE : &str = "If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug"; From da306444807584d0681c4034a12a6ec6cd307408 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 13:52:49 -0300 Subject: [PATCH 04/13] Add detector --- .github/workflows/test-detectors.yml | 1 + detectors/unused-return-enum/Cargo.toml | 19 ++++ detectors/unused-return-enum/src/lib.rs | 114 ++++++++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 detectors/unused-return-enum/Cargo.toml create mode 100644 detectors/unused-return-enum/src/lib.rs diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index 298fe969..3c17adc4 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -108,6 +108,7 @@ jobs: "unprotected-update-current-contract-wasm", "unsafe-expect", "unsafe-unwrap", + "unused-return-enum", ] runs-on: ${{ matrix.os }} steps: diff --git a/detectors/unused-return-enum/Cargo.toml b/detectors/unused-return-enum/Cargo.toml new file mode 100644 index 00000000..ec17fe3d --- /dev/null +++ b/detectors/unused-return-enum/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "unused-return-enum" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +scout-audit-internal = { workspace = true } + +[dev-dependencies] +dylint_testing = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/unused-return-enum/src/lib.rs b/detectors/unused-return-enum/src/lib.rs new file mode 100644 index 00000000..a8fab7c4 --- /dev/null +++ b/detectors/unused-return-enum/src/lib.rs @@ -0,0 +1,114 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_span; + +use if_chain::if_chain; +use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; +use rustc_hir::{Body, Expr, ExprKind, FnDecl, FnRetTy, MatchSource, QPath, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::{def_id::LocalDefId, sym, Span}; +use scout_audit_internal::Detector; + +dylint_linting::declare_late_lint! { + pub UNUSED_RETURN_ENUM, + Warn, + Detector::UnusedReturnEnum.get_lint_message() +} + +#[derive(Debug)] +struct CounterVisitor { + count_err: u32, + count_ok: u32, + found_try: bool, + found_return: bool, + span: Vec, +} + +impl<'tcx> Visitor<'tcx> for CounterVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr) { + match expr.kind { + ExprKind::Call(function, _) => { + if_chain! { + if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind; + if let Some(last_segment) = path.segments.last(); + then { + if last_segment.ident.name == sym::Ok { + self.count_ok += 1; + self.span.push(expr.span); + } else if last_segment.ident.name == sym::Err { + self.count_err += 1; + self.span.push(expr.span); + } + } + } + } + ExprKind::Ret(Some(return_value)) => { + if_chain! { + if let ExprKind::Call(function, _) = &return_value.kind; + if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind; + if let Some(last_segment) = path.segments.last(); + then { + if last_segment.ident.name != sym::Ok && last_segment.ident.name != sym::Err { + self.found_return = true; + } + } + } + } + ExprKind::Match(_, _, MatchSource::TryDesugar(_)) => { + self.found_try = true; + } + _ => {} + } + walk_expr(self, expr); + } +} +impl<'tcx> LateLintPass<'tcx> for UnusedReturnEnum { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + fnkind: FnKind<'tcx>, + decl: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: LocalDefId, + ) { + // If the function is not a method, or it comes from a macro expansion, we ignore it + if !matches!(fnkind, FnKind::Method(_, fnsig) if !fnsig.span.from_expansion()) { + return; + } + + // If the function returns a type different from Result, we ignore it + if_chain! { + if let FnRetTy::Return(return_type) = &decl.output; + if let TyKind::Path(qpath) = &return_type.kind; + if let QPath::Resolved(_ty, path) = qpath; + if let Some(path_segment) = path.segments.last(); + if path_segment.ident.name != sym::Result; + then { + return; + } + } + + let mut visitor = CounterVisitor { + count_ok: 0, + count_err: 0, + found_try: false, + found_return: false, + span: Vec::new(), + }; + + walk_expr(&mut visitor, body.value); + + if !visitor.found_return && (visitor.count_err == 0 || visitor.count_ok == 0) { + visitor.span.iter().for_each(|span| { + Detector::UnusedReturnEnum.span_lint_and_help( + cx, + UNUSED_RETURN_ENUM, + *span, + "If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug", + ); + }); + } + } +} From a55b319f75b944fda7f5776b91b9121f00747f03 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 15:04:26 -0300 Subject: [PATCH 05/13] Edit second vulnerable test-case --- .../unused-return-enum-2/vulnerable-example/src/lib.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs index 42f91c2f..e4debe03 100644 --- a/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs +++ b/test-cases/unused-return-enum/unused-return-enum-2/vulnerable-example/src/lib.rs @@ -20,10 +20,6 @@ impl UnusedReturnEnum { let absolute_difference = balance1.abs_diff(balance2); let sum = balance1 + balance2; - let _ignore = 100u128 - .checked_mul(absolute_difference / sum) - .ok_or(Error::Overflow)?; - match 100u128.checked_mul(absolute_difference / sum) { Some(result) => result, None => panic!("Overflow"), From aa49d6f1b3d1f3d95844ccadc7eea96692ba5b05 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 15:11:01 -0300 Subject: [PATCH 06/13] Run cargo-fmt --- .../unused-return-enum-1/remediated-example/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs index d439b144..d1c17189 100644 --- a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs +++ b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs @@ -19,7 +19,7 @@ impl UnusedReturnEnum { pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { let absolute_difference = balance1.abs_diff(balance2); let sum = balance1 + balance2; - + match 100u128.checked_mul(absolute_difference / sum) { Some(result) => Ok(result), None => Err(Error::Overflow), From 1df7ebeec47a0f57ec837f0e497b4ea2f18900c9 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:41:37 -0300 Subject: [PATCH 07/13] Add detector unused-return-enum --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 1cbbccc4..80b17661 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [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 | +| [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) | Minor | ## CLI Options From 7f0c8e31475af5c6aab5766dac257208311d1cd9 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:44:45 -0300 Subject: [PATCH 08/13] Add unused return enum vulnerability --- test-cases/README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index 096512b0..18afcac7 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -192,3 +192,13 @@ Using a pinned version of Soroban can be dangerous, as it may have bugs or secur 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. +### Unused return enum + +`Rust` messages can return a `Result` `enum` with a custom error type. This is +useful for the caller to know what went wrong when the message fails. The +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. From 618c6ff70dd60886a9f90836a6a3a505b89283c5 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:51:57 -0300 Subject: [PATCH 09/13] Add detector documentation unused return enum --- detectors/unused-return-enum/README.md | 87 ++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 detectors/unused-return-enum/README.md diff --git a/detectors/unused-return-enum/README.md b/detectors/unused-return-enum/README.md new file mode 100644 index 00000000..cd6d765f --- /dev/null +++ b/detectors/unused-return-enum/README.md @@ -0,0 +1,87 @@ +# Unused return enum + +### What it does + +It warns if a function that returns a Result type does not return the Result enum variant (Ok/Err). + +### Why is this bad? + +If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug. + +### Known problems + +If definitions of `Err()` and/or `Ok()` are in the code but do not flow to the return value due to the definition of a variable or because they are defined in a dead code block, the warning will not be shown. If the definitions are made in an auxiliary method, the warning will be shown, resulting in a false positive. + +### Example + +Instead of using: + +```rust +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; + + match 100u128.checked_mul(absolute_difference / sum) { + Some(result) => result, + None => panic!("Overflow"), + }; + + Err(Error::Overflow) + } +} +``` + +Use this: + +```rust +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; + +#[contract] +pub struct UnusedReturnEnum; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// An overflow was produced. + Overflow = 1, +} + +#[contractimpl] +impl UnusedReturnEnum { + /// Returns the percentage difference between two values. + pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; + + match 100u128.checked_mul(absolute_difference / sum) { + Some(result) => Ok(result), + None => Err(Error::Overflow), + } + } +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum). From 535f1d5c65dda0cd29e9a370e88d198325bc6b53 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:53:54 -0300 Subject: [PATCH 10/13] Add second test case reference unused return enum --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 80b17661..475cb80c 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [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 | -| [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) | Minor | +| [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 | ## CLI Options From 87bc837200037e31a5c2080efea76be81cd7e70c Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Fri, 15 Mar 2024 11:24:59 -0300 Subject: [PATCH 11/13] Fix some clippy errors on tests --- .../tests/integration_tests/detectors.rs | 12 ++++-------- .../integration_tests/detectors/configuration.rs | 4 +--- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/apps/cargo-scout-audit/tests/integration_tests/detectors.rs b/apps/cargo-scout-audit/tests/integration_tests/detectors.rs index 9b4842be..a3f8e616 100644 --- a/apps/cargo-scout-audit/tests/integration_tests/detectors.rs +++ b/apps/cargo-scout-audit/tests/integration_tests/detectors.rs @@ -44,7 +44,8 @@ fn test() { vec![false; integration_tests_to_run.as_ref().map_or(0, |v| v.len())]; // Get the configuration - let detectors_config = Configuration::build().expect(&"Failed to get the configuration".red()); + let detectors_config = Configuration::build() + .unwrap_or_else(|_| panic!("{}", "Failed to get the configuration".red().to_string())); // Run all integration tests for detector_config in detectors_config.detectors.iter() { @@ -64,15 +65,10 @@ fn test() { println!("\n{} {}", "Testing detector:".bright_cyan(), detector_name); for example in detector_config.testcases.iter() { if let Some(vulnerable_path) = &example.vulnerable_path { - execute_and_validate_testcase(&detector_name, lint_message, &vulnerable_path, true); + execute_and_validate_testcase(&detector_name, lint_message, vulnerable_path, true); } if let Some(remediated_path) = &example.remediated_path { - execute_and_validate_testcase( - &detector_name, - lint_message, - &remediated_path, - false, - ); + execute_and_validate_testcase(&detector_name, lint_message, remediated_path, false); } } } diff --git a/apps/cargo-scout-audit/tests/integration_tests/detectors/configuration.rs b/apps/cargo-scout-audit/tests/integration_tests/detectors/configuration.rs index e21da35c..dfab0317 100644 --- a/apps/cargo-scout-audit/tests/integration_tests/detectors/configuration.rs +++ b/apps/cargo-scout-audit/tests/integration_tests/detectors/configuration.rs @@ -34,7 +34,6 @@ impl Configuration { .ok_or(anyhow!("Failed to find testcases path"))? .join("test-cases"); let testcases_paths: Vec = std::fs::read_dir(&testcases_root_path)? - .into_iter() .filter_map(|r| r.ok().map(|f| f.path())) .filter(|r| r.is_dir()) .collect(); @@ -47,7 +46,6 @@ impl Configuration { let detector_name = detector.to_string(); let testcases_root_path = testcases_root_path.join(detector_name); let testcases_paths: Vec = std::fs::read_dir(testcases_root_path)? - .into_iter() .filter_map(|r| r.ok().map(|f| f.path())) .filter(|r| r.is_dir()) .collect(); @@ -101,7 +99,7 @@ impl Configuration { .into_iter() .sorted() .zip(Detector::iter().map(|d| d.to_string()).sorted()) - .filter(|(p, d)| p.file_name().unwrap().to_string_lossy() != d.to_string()) + .filter(|(p, d)| p.file_name().unwrap().to_string_lossy() != *d) .count(); if count > 0 { From d3c9eb4d8d7c7005640a8458e9649230c694acd0 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Fri, 15 Mar 2024 12:06:05 -0300 Subject: [PATCH 12/13] Fix CI --- .../unused-return-enum-1/remediated-example/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs index d1c17189..877811a8 100644 --- a/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs +++ b/test-cases/unused-return-enum/unused-return-enum-1/remediated-example/src/lib.rs @@ -1,6 +1,6 @@ #![no_std] -use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; +use soroban_sdk::{contract, contracterror, contractimpl}; #[contract] pub struct UnusedReturnEnum; From 86c18fed37a15dbe2fb56336bf0ee4f3e3498ff0 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Fri, 15 Mar 2024 12:19:13 -0300 Subject: [PATCH 13/13] Fix CI 2 --- detectors/unused-return-enum/src/lib.rs | 5 ++++- .../unused-return-enum-2/remediated-example/src/lib.rs | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/detectors/unused-return-enum/src/lib.rs b/detectors/unused-return-enum/src/lib.rs index a8fab7c4..458bad70 100644 --- a/detectors/unused-return-enum/src/lib.rs +++ b/detectors/unused-return-enum/src/lib.rs @@ -100,7 +100,10 @@ impl<'tcx> LateLintPass<'tcx> for UnusedReturnEnum { walk_expr(&mut visitor, body.value); - if !visitor.found_return && (visitor.count_err == 0 || visitor.count_ok == 0) { + if !visitor.found_return + && !visitor.found_try + && (visitor.count_err == 0 || visitor.count_ok == 0) + { visitor.span.iter().for_each(|span| { Detector::UnusedReturnEnum.span_lint_and_help( cx, diff --git a/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs index 57d11f9a..91e84910 100644 --- a/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs +++ b/test-cases/unused-return-enum/unused-return-enum-2/remediated-example/src/lib.rs @@ -1,6 +1,6 @@ #![no_std] -use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; +use soroban_sdk::{contract, contracterror, contractimpl}; #[contract] pub struct UnusedReturnEnum; @@ -17,8 +17,8 @@ pub enum Error { impl UnusedReturnEnum { /// Returns the percentage difference between two values. pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { - let absolute_difference = value1.abs_diff(value2); - let sum = value1 + value2; + let absolute_difference = balance1.abs_diff(balance2); + let sum = balance1 + balance2; let result = 100u128 .checked_mul(absolute_difference / sum) .ok_or(Error::Overflow)?;