diff --git a/docs/docs/detectors/13-unused-return-enum.md b/docs/docs/detectors/13-unused-return-enum.md index edc50b99..9ea2232a 100644 --- a/docs/docs/detectors/13-unused-return-enum.md +++ b/docs/docs/detectors/13-unused-return-enum.md @@ -1,30 +1,34 @@ # Unused return enum -### What it does +## Description -It warns if a function that returns a Result type does not return the Result enum variant (Ok/Err). +- Vulnerability Category: `Validations and error handling` +- Vulnerability Severity: `Minor` +- Detectors: [`unused-return-enum`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum) +- Test Cases: [`unused-return-enum-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1) [`unused-return-enum-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) + +Soroban 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. -### Why is this bad? +The definition in Rust of the `Result` enum is: -If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug. +```rust +enum Result { + Ok(T), + Err(E), +} +``` -### Known problems +## Why is this bad? -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. +If either variant (`Ok` or `Err`) is not used in the code, it could indicate that the `Result` type is unnecessary and that the code could be simplified. Alternatively, it might suggest a bug where a possible outcome is not being handled properly. -### Example -Instead of using: -```rust -#![no_std] +## Issue example -use soroban_sdk::{contract, contracterror, contractimpl}; +Consider the following `Soroban` contract: -#[contract] -pub struct UnusedReturnEnum; - -#[contracterror] +```rust #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { @@ -32,10 +36,9 @@ pub enum Error { Overflow = 1, } -#[contractimpl] -impl UnusedReturnEnum { - /// Returns the percentage difference between two values. - pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + + +pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { let absolute_difference = balance1.abs_diff(balance2); let sum = balance1 + balance2; @@ -45,21 +48,25 @@ impl UnusedReturnEnum { }; Err(Error::Overflow) - } -} + } ``` -Use this: +This is a `Soroban` message that returns the percentage difference between two values. -```rust -#![no_std] +The function then returns an error enum variant `TradingPairErrors::Overflow`. +However, the function never returns a `Result` enum variant `Ok`, thus always +failing. -use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1/remediated-example ). -#[contract] -pub struct UnusedReturnEnum; +## Remediated example -#[contracterror] +This function could be easily fixed by returning a `Result` enum variant `Ok` +when the percentage difference is calculated successfully. By providing a check in +the linter that ensures that all the variants of the `Result` enum are used, this +bug could have been avoided. This is shown in the example below: + +```rust #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { @@ -67,10 +74,8 @@ pub enum Error { Overflow = 1, } -#[contractimpl] -impl UnusedReturnEnum { - /// Returns the percentage difference between two values. - pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { + +pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result { let absolute_difference = balance1.abs_diff(balance2); let sum = balance1 + balance2; @@ -79,9 +84,10 @@ impl UnusedReturnEnum { None => Err(Error::Overflow), } } -} ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum//unused-return-enum-1/remediated-example). -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum). \ No newline at end of file +## How is it detected? + +It warns if a function that returns a Result type does not return the Result enum variant (Ok/Err).