Skip to content

Commit

Permalink
Merge pull request #299 from CoinFabrik/migrate-unsafe-unwrap-documen…
Browse files Browse the repository at this point in the history
…tation

New unsafe-unwrap documentation
  • Loading branch information
matiascabello authored Aug 7, 2024
2 parents 53157fd + d4cc357 commit 738320b
Showing 1 changed file with 59 additions and 22 deletions.
81 changes: 59 additions & 22 deletions docs/docs/detectors/2-unsafe-unwrap.md
Original file line number Diff line number Diff line change
@@ -1,39 +1,76 @@
# Unsafe unwrap

### What it does
## Description

Checks for usage of `.unwrap()`
- Category: `Validations and error handling`
- Severity: `Minor`
- Detectors: [`unsafe-unwrap`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap)
- Test Cases: [`unsafe-unwrap-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1) [`unsafe-unwrap-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-2) [`unsafe-unwrap-3`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-3) [`unsafe-unwrap-4`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-4) [`unsafe-unwrap-5`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-5) [`unsafe-unwrap-6`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-6)

### Why is this bad?

`.unwrap()` might panic if the result value is an error or `None`.
In Rust, the `unwrap` method is commonly used for error handling. It retrieves the inner value of an `Option` or `Result`. If an error or `None` occurs, it calls `panic!` without a custom error message.

### Example
## Why is this bad?

`.unwrap()` might panic if the result value is an error or `None`. It is recommended to avoid the panic of a contract because it stops its execution, which might lead the contract to an inconsistent state if the panic occurs in the middle of state changes. Additionally, the panic could cause a transaction to fail.

## Issue example

Consider the following `Soroban` contract:

```rust
// example code where a warning is issued
fn main() {
let result = result_fn().unwrap("error");
}
fn result_fn() -> Result<u8, Error> {
Err(Error::new(ErrorKind::Other, "error"))
#[contractimpl]
impl UnsafeUnwrap {
pub fn unwrap(n: u64) -> u64 {
let result = Self::non_zero_or_error(n);
result.unwrap()
}

pub fn non_zero_or_error(n: u64) -> Result<u64, Error> {
if n == 0 {
return Err(Error::CustomError);
}
Ok(n)
}
}
```

Use instead:
In this contract, the `unwrap` function uses the `unwrap` method to save the result of the `non_zero_or_error` function. If the function returns `Err`, the contract will panic and halt execution, potentially leading to malicious exploitation to disrupt the contract's operation.

The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1/vulnerable-example).

## Remediated example

Instead of using `unwrap`, use a safer method for error handling. In this case, if the function returns `Err`, it will return a default value (like `0`).

```rust
// example code that does not raise a warning
fn main() {
let result = if let Ok(result) = result_fn() {
result
}
}
fn result_fn() -> Result<u8, Error> {
Err(Error::new(ErrorKind::Other, "error"))
#[contractimpl]
impl UnsafeUnwrap {
pub fn unwrap_or_default(n: u64) -> u64 {
let result = Self::non_zero_or_error(n);
result.unwrap_or(0)
}

pub fn non_zero_or_error(n: u64) -> Result<u64, Error> {
if n == 0 {
return Err(Error::CustomError);
}
Ok(n)
}
}
```

### Implementation
The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1/remediated-example).

## How is it detected?

Checks for usage of .unwrap()

## References

[Rust documentation: `unwrap`](https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap)




The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap).

0 comments on commit 738320b

Please sign in to comment.