Skip to content

Commit

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

New unsafe-expect documentation
  • Loading branch information
matiascabello authored Aug 11, 2024
2 parents 78af369 + 600358e commit d713e7a
Showing 1 changed file with 39 additions and 22 deletions.
61 changes: 39 additions & 22 deletions docs/docs/detectors/3-unsafe-expect.md
Original file line number Diff line number Diff line change
@@ -1,39 +1,56 @@
# Unsafe expect

### What it does
## Description

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

### Why is this bad?
In Rust, the `expect` method is often used for error handling. It returns the contained `Ok` value for a `Result` or `Some` value for an `Option`. If an error occurs, it calls `panic!` with a provided error message.

`.expect()` might panic if the result value is an error or `None`.
## Why is this bad?

### Example
`.expect()` 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().expect("error");
}
fn result_fn() -> Result<u8, Error> {
Err(Error::new(ErrorKind::Other, "error"))

pub fn balance_of(env: Env, owner: Address) -> i128 {
let state = Self::get_state(env);
state.balances.get(owner).expect("could not get balance")
}

```

Use instead:
In this contract, the `balance_of` function uses the expect method to retrieve the balance of an account. If there is no entry for this account in the balances mapping, the contract will panic and halt execution, which could be exploited maliciously to disrupt the contract's operation.

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


## Remediated example

Instead of using `expect`, use a safer method for error handling. In this case, if there is no entry for an account in the `balances` mapping, 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"))

pub fn balance_of(env: Env, owner: Address) -> i128 {
let state = Self::get_state(env);
state.balances.get(owner).unwrap_or(0)
}

```

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


## How is it detected?

Checks for usage of .expect().


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

0 comments on commit d713e7a

Please sign in to comment.