Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

33 update vulnerability and detector documentation #34

Merged
merged 5 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions detectors/divide-before-multiply/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Divide before multiply

### What it does

Checks the existence of a division before a multiplication.

### Why is this bad?

Division between two integers might return zero.

### Example

```rust
let x = 1;
let y = 2;
let z = x / y * 3;
```

Use instead:

```rust
let x = 1;
let y = 2;
let z = x * 3 / y;
```

### Implementation

The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply).
90 changes: 90 additions & 0 deletions detectors/overflow-check/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Overflow-check

### What it does

Checks that `overflow-checks` is enabled in the `[profile.release]` section of the `Cargo.toml`.

### Why is this bad?

Integer overflow will trigger a panic in debug builds or will wrap in
release mode. Division by zero will cause a panic in either mode. In some applications one
wants explicitly checked, wrapping or saturating arithmetic.


### Example

```toml

[package]
name = "overflow-check-vulnerable-1"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["cdylib"]

[dependencies]
soroban-sdk = "20.0.0-rc2"

[dev_dependencies]
soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] }

[features]
testutils = ["soroban-sdk/testutils"]

[profile.release]
opt-level = "z"
overflow-checks = false
debug = 0
strip = "symbols"
debug-assertions = false
panic = "abort"
codegen-units = 1
lto = true

[profile.release-with-logs]
inherits = "release"
debug-assertions = true
```

Use instead:

```toml

[package]
name = "overflow-check-remediated-1"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["cdylib"]

[dependencies]
soroban-sdk = "20.0.0-rc2"

[dev_dependencies]
soroban-sdk = { version = "20.0.0-rc2", 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]
overflow-checks = true
inherits = "release"
debug-assertions = true

```

### Implementation

The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/integer-overflow-or-underflow).
39 changes: 39 additions & 0 deletions detectors/unsafe-expect/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Unsafe expect

### What it does

Checks for usage of `.expect()`

### Why is this bad?

`.expect()` might panic if the result value is an error or `None`.

### Example

```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"))
}
```

Use instead:

```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"))
}
```

### Implementation

The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-expect).
39 changes: 39 additions & 0 deletions detectors/unsafe-unwrap/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Unsafe unwrap

### What it does

Checks for usage of `.unwrap()`

### Why is this bad?

`.unwrap()` might panic if the result value is an error or `None`.

### Example

```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"))
}
```

Use instead:

```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"))
}
```

### Implementation

The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap).
35 changes: 33 additions & 2 deletions test-cases/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ We used the above Vulnerability Categories, along with common examples of vulner

## Vulnerability Classes

As a result of our research, we have so far identified thirteen types of vulnerabilities.
As a result of our research, we have so far identified 4 types of vulnerabilities.

What follows is a description of each vulnerability in the context of Stellar Soroban smart contracts. In each case, we have produced at least one [test-case](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases) smart contract that exposes one of these vulnerabilities.

Check our
[test-cases](https://github.com/CoinFabrik/scout/tree/main/test-cases)
[test-cases](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases)
for code examples of these vulnerabilities and their respective remediations.


Expand All @@ -73,3 +73,34 @@ This vulnerability class pertains to the inappropriate usage of the `unwrap` met
This vulnerability again falls under the [Validations and error handling](#vulnerability-categories) category and has a Medium severity.

In our example, we consider an contract that utilizes the `unwrap` method to retrieve the balance of an account from a mapping. If there is no entry for the specified account, the contract will panic and abruptly halt execution, opening avenues for malicious exploitation.


### Unsafe expect

In Rust, the `expect` method is commonly used for error handling. It retrieves the value from a `Result` or `Option` and panics with a specified error message if an error occurs. However, using `expect` can lead to unexpected program crashes.

This vulnerability falls under the [Validations and error handling](#vulnerability-categories) category
and has a Medium severity.

In our example, we see an exploit scenario involving a contract using the `expect` method in a function that retrieves the balance of an account. If there is no entry for the account, the contract panics and halts execution, enabling malicious exploitation.

### Integer overflow or underflow

This type of vulnerability occurs when an arithmetic operation attempts to
create a numeric value that is outside the valid range in substrate, e.g,
a `u8` unsigned integer can be at most _M:=2^8-1=255_, hence the sum `M+1`
produces an overflow.

An overflow/underflow is typically caught and generates an error. When it
is not caught, the operation will result in an inexact result which could
lead to serious problems.

We classified this type of vulnerability under
the [Arithmetic](#vulnerability-categories) category and assigned it a
Critical severity.

In the context of Soroban, we found that this vulnerability could only be
realized if `overflow-checks` is set to `False` in the `[profile.release]` section of the `Cargo.toml`.
Notwithstanding, there are contexts where developers do turn off checks for
valid reasons and hence the reason for including this vulnerability in the
list.