From c25a5720ac7f931214c2f9ae908acd848a241000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Tue, 19 Mar 2024 10:33:49 -0300 Subject: [PATCH] 86 unused return enum (#90) * Add first test case * Add second test case * Add detector to list * Add detector * Edit second vulnerable test-case * Run cargo-fmt * Add detector unused-return-enum * Add unused return enum vulnerability * Add detector documentation unused return enum * Add second test case reference unused return enum * Fix some clippy errors on tests * Fix CI * Fix CI 2 --------- Co-authored-by: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> --- .github/workflows/test-detectors.yml | 1 + README.md | 1 + .../tests/integration_tests/detectors.rs | 12 +- .../detectors/configuration.rs | 4 +- detectors/unused-return-enum/Cargo.toml | 19 +++ detectors/unused-return-enum/README.md | 87 +++++++++++++ detectors/unused-return-enum/src/lib.rs | 117 ++++++++++++++++++ scout-audit-internal/src/detector.rs | 2 + .../src/detector/lint_message.rs | 1 + test-cases/README.md | 10 ++ .../remediated-example/Cargo.toml | 30 +++++ .../remediated-example/src/lib.rs | 52 ++++++++ .../vulnerable-example/Cargo.toml | 30 +++++ .../vulnerable-example/src/lib.rs | 54 ++++++++ .../remediated-example/Cargo.toml | 30 +++++ .../remediated-example/src/lib.rs | 51 ++++++++ .../vulnerable-example/Cargo.toml | 30 +++++ .../vulnerable-example/src/lib.rs | 54 ++++++++ 18 files changed, 574 insertions(+), 11 deletions(-) create mode 100644 detectors/unused-return-enum/Cargo.toml create mode 100644 detectors/unused-return-enum/README.md create mode 100644 detectors/unused-return-enum/src/lib.rs 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 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/.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/README.md b/README.md index 1cbbccc4..475cb80c 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), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) | Minor | ## CLI Options 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 { 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/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). diff --git a/detectors/unused-return-enum/src/lib.rs b/detectors/unused-return-enum/src/lib.rs new file mode 100644 index 00000000..458bad70 --- /dev/null +++ b/detectors/unused-return-enum/src/lib.rs @@ -0,0 +1,117 @@ +#![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.found_try + && (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", + ); + }); + } + } +} 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"; 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. 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..877811a8 --- /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}; + +#[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))); + } +} 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..91e84910 --- /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}; + +#[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 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..e4debe03 --- /dev/null +++ b/test-cases/unused-return-enum/unused-return-enum-2/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))); + } +}