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

New iterators-over-indexing documentation #316

Merged
merged 2 commits into from
Aug 12, 2024
Merged
Changes from 1 commit
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
36 changes: 25 additions & 11 deletions docs/docs/detectors/14-iterators-over-indexing.md
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
# Iterators-over-indexing
# Iterators over indexing

### What it does
## Description

It warns if the for loop uses indexing instead of iterator. If the indexing goes to length it will not raise a warning.
- Category: `Best practices`
- Severity: `Enhancement`
- Detector: [`iterators-over-indexing`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing)
- Test Cases: [`iterators-over-indexing-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing/iterators-over-indexing-1)

### Why is this bad?
In Rust, sequences can be traversed using iterators or direct indexing. However, the least efficient way is through direct indexing.

## Why is this bad?

Accessing a vector by index is slower than using an iterator. Also, if the index is out of bounds, it will panic.
When you iterate over a data structure with fixed limits, you might exceed the limit, causing a panic in the contract and other potential errors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Una sugerencia para que quede un poquito más claro: "When you iterate over a data structure with fixed limits in a Soroban smart contract, exceeding those limits can cause the contract to panic, potentially leading to errors or unexpected behavior."


## Issue example

### Example​
Consider the following `Soroban` contract:

```rust
pub fn sum(e: Env) -> Result<i32, Error> {
pub fn sum(e: Env) -> Result<i32, Error> {
let mut ret = 0_i32;
let vec = e
.storage()
Expand All @@ -28,9 +33,14 @@ Accessing a vector by index is slower than using an iterator. Also, if the index
Ok(ret)
}
```
Use instead:
The problem arises in the for loop. If `vec` has less than 4 elements, the contract will panic.

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

## Remediated example

```rust
pub fn sum(e: Env) -> Result<i32, Error> {
pub fn sum(e: Env) -> Result<i32, Error> {
let mut ret = 0_i32;
let vec = e
.storage()
Expand All @@ -43,7 +53,11 @@ Use instead:
Ok(ret)
}
```
### Implementation

The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing).
Instead of using a fixed loop, iterate through the vector itself using `for i in vec`. This ensures the loop iterates only for valid elements present in the vector.

The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing/iterators-over-indexing-1/remediated-example).

## How is it detected?

It warns if the for loop uses indexing instead of iterator. If the indexing goes to length it will not raise a warning.