diff --git a/README.md b/README.md index a45c329e..dd872fe7 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ Currently Scout includes the following detectors. | [iterators-over-indexing](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing) |Iterating with hardcoded indexes is slower than using an iterator. Also, if the index is out of bounds, it will panic. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing-1) | Enhancement | | [assert-violation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation) | Avoid the usage of the macro assert!, it can panic.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1) | Enhancement | | [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation) | Modifying mappings with an arbitrary key given by users can be a significant vulnerability. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-2) | Critical | -| [dos-unexpected-revert-with-vector](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) | DoS due to improper storage. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1) | Medium | +| [dos-unexpected-revert-with-vector](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) | DoS due to improper storage. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2) | Medium | | [unrestricted-transfer-from](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unrestricted-transfer-from) | Avoid passing an user-defined parameter as a `from` field in transfer-from. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1) | Critical | | [unsafe-map-get](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-map-get) | Inappropriate usage of the `get` method for `Map` in soroban | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-map-get/unsafe-map-get-1) | Medium | | [zero-or-test-address](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/zero-or-test-address) | Avoid zero or test address assignment to prevent contract control loss. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/zero-or-test-address/zero-or-test-address-1) | Medium | diff --git a/detectors/Cargo.toml b/detectors/Cargo.toml index de320451..589ab058 100644 --- a/detectors/Cargo.toml +++ b/detectors/Cargo.toml @@ -5,7 +5,7 @@ resolver = "2" [workspace.dependencies] clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "51a1cf0" } -clippy_wrappers = { package = "scout-audit-clippy-wrappers", git = "https://github.com/CoinFabrik/scout-audit/", rev = "c41db6ac6752c97b1516d6950ba4d0a9b995d594" } +clippy_wrappers = { package = "scout-audit-clippy-wrappers", git = "https://github.com/CoinFabrik/scout-audit/", rev = "9597702f61abab9d85363bcfa2a3f35887f24182" } common = { path = "common" } dylint_linting = { package = "scout-audit-dylint-linting", git = "https://github.com/CoinFabrik/scout-audit/", rev = "6d6819a" } if_chain = "=1.0.2" diff --git a/detectors/dynamic-instance-storage/Cargo.toml b/detectors/dynamic-instance-storage/Cargo.toml new file mode 100644 index 00000000..3060bf9f --- /dev/null +++ b/detectors/dynamic-instance-storage/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/dynamic-instance-storage/src/lib.rs b/detectors/dynamic-instance-storage/src/lib.rs new file mode 100644 index 00000000..9319f082 --- /dev/null +++ b/detectors/dynamic-instance-storage/src/lib.rs @@ -0,0 +1,97 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint; +use if_chain::if_chain; +use rustc_hir::{ + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{Ty, TyKind}; +use rustc_span::{def_id::LocalDefId, Span, Symbol}; +use utils::{get_node_type_opt, is_soroban_storage, SorobanStorageType}; + +const LINT_MESSAGE: &str = "This function may lead to excessive instance storage growth, which could increase execution costs or potentially cause DoS"; + +dylint_linting::impl_late_lint! { + pub DYNAMIC_INSTANCE_STORAGE, + Warn, + LINT_MESSAGE, + DynamicInstanceStorage, + { + name: "Dynamic Instance Storage Analyzer", + long_message: "Detects potential misuse of instance storage that could lead to unnecessary growth or storage-related vulnerabilities.", + severity: "Warning", + help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-instance-storage", + vulnerability_class: "Resource Management", + } +} + +#[derive(Default)] +struct DynamicInstanceStorage; + +impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + _: LocalDefId, + ) { + if span.from_expansion() { + return; + } + + let mut storage_warn_visitor = DynamicInstanceStorageVisitor { cx }; + storage_warn_visitor.visit_body(body); + } +} + +struct DynamicInstanceStorageVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for DynamicInstanceStorageVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if_chain! { + // Detect calls to `set` method + if let ExprKind::MethodCall(path, receiver, args, _) = &expr.kind; + if path.ident.name == Symbol::intern("set"); + // Get the type of the receiver and check if it is an instance storage + if let Some(receiver_ty) = get_node_type_opt(self.cx, &receiver.hir_id); + if is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Instance); + // Check if the value being set is a dynamic type + if args.len() >= 2; + if let Some(value_type) = get_node_type_opt(self.cx, &args[1].hir_id); + if is_dynamic_type(self.cx, &value_type); + then { + span_lint(self.cx, DYNAMIC_INSTANCE_STORAGE, expr.span, LINT_MESSAGE) + } + } + + walk_expr(self, expr) + } +} + +fn is_dynamic_type(cx: &LateContext, ty: &Ty) -> bool { + match ty.kind() { + TyKind::Str => true, + TyKind::Slice(_) => true, + TyKind::Dynamic(..) => true, + TyKind::Array(element_ty, _) => is_dynamic_type(cx, element_ty), + TyKind::Adt(adt_def, _) => { + let type_name = cx.tcx.item_name(adt_def.did()); + matches!(type_name.as_str(), "Vec" | "String" | "Map" | "LinkedList") + } + TyKind::RawPtr(ty, _) => is_dynamic_type(cx, ty), + TyKind::Ref(_, ty, _) => is_dynamic_type(cx, ty), + TyKind::Tuple(substs) => substs.iter().any(|ty| is_dynamic_type(cx, &ty)), + _ => false, + } +} diff --git a/docs/docs/detectors/1-divide-before-multiply.md b/docs/docs/detectors/1-divide-before-multiply.md index 54d4f3c7..5d5f5367 100644 --- a/docs/docs/detectors/1-divide-before-multiply.md +++ b/docs/docs/detectors/1-divide-before-multiply.md @@ -1,43 +1,55 @@ # Divide before multiply -### What it does +## Description -Checks the existence of a division before a multiplication. +- Category: `Arithmetic` +- Severity: `Medium` +- Detectors: [`divide-before-multiply`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply) +- Test Cases: [`divide-before-multiply-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1) [`divide-before-multiply-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-2) [`divide-before-multiply-3`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-3) + +In Rust, the order of operations can influence the precision of the result, especially in integer arithmetic. -### Why is this bad? +## Why is this bad? -Performing a division operation before multiplication can lead to a loss of precision. It might even result in an unintended zero value. +Performing a division operation before a multiplication can lead to a loss of precision as division between integers might return zero. -### Example +## Issue example + +Consider the following `Soroban` contract: ```rust -// Example 1 - Vulnerable -let x = 10; -let y = 6; -let z = x / y * 3; // z evaluates to 3 - -// Example 2 - Vulnerable -let a = 1; -let b = 2; -let c = a / b * 3; // c evaluates to 0 + + pub fn split_profit(percentage: u64, total_profit: u64) -> u64 { + (percentage / 100) * total_profit +} + ``` -Use instead: +In this contract, the `split_profit` function divides the `percentage` by `100` before multiplying it with `total_profit`. This could lead to a loss of precision if `percentage` is less than `100` as the division would return `0`. This could lead to incorrect calculations and potential financial loss in a real-world smart contract. + + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1/vulnerable-example). + + +## Remediated example + +Reverse the order of operations to ensure multiplication occurs before division. ```rust -// Example 1 - Remediated -let x = 10; -let y = 6; -let z = x * 3 / y; // z evaluates to 5 - -// Example 2 - Remediated -let a = 1; -let b = 2; -let c = a * 3 / b; // c evaluates to 1 + + pub fn split_profit(&self, percentage: u64, total_profit: u64) -> u64 { + (percentage * total_profit) / 100 + } + ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1/remediated-example). + +## How is it detected? -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply). +Checks the existence of a division before a multiplication. +## References +[Rust documentation: `Integer Division`](https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators) + diff --git a/docs/docs/detectors/10-avoid-unsafe-block.md b/docs/docs/detectors/10-avoid-unsafe-block.md index 3423035b..b2a8186f 100644 --- a/docs/docs/detectors/10-avoid-unsafe-block.md +++ b/docs/docs/detectors/10-avoid-unsafe-block.md @@ -1,47 +1,74 @@ # Avoid unsafe block -### What it does +## Description -Checks for usage of `unsafe` blocks. +- Category: `Validations and error handling` +- Severity: `Critical` +- Detector: [`avoid-unsafe-block`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) +- Test Cases: [`avoid-unsafe-block-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) + +The use of unsafe blocks in Rust is generally discouraged due to the potential risks it poses to the safety and reliability of the code. Rust's primary appeal lies in its ability to provide memory safety guarantees, which are largely enforced through its ownership and type systems. When you enter an unsafe block, you're effectively bypassing these safety checks. These blocks require the programmer to manually ensure that memory is correctly managed and accessed, which is prone to human error and can be challenging even for experienced developers. Therefore, unsafe blocks should only be used when absolutely necessary and when the safety of the operations within can be assured. -### Why is this bad? +## Why is this bad? `unsafe` blocks should not be used unless absolutely necessary. The use of unsafe blocks in Rust is discouraged because they bypass Rust's memory safety checks, potentially leading to issues like undefined behavior and security vulnerabilities. -### Example +## Issue example - ```rust -pub fn unsafe_function(n: u64) -> u64 { - unsafe { - let mut i = n as f64; - let mut y = i.to_bits(); - y = 0x5fe6ec85e7de30da - (y >> 1); - i = f64::from_bits(y); - i *= 1.5 - 0.5 * n as f64 * i * i; - i *= 1.5 - 0.5 * n as f64 * i * i; +Consider the following `Soroban` contract: - let result_ptr: *mut f64 = &mut i; - let result = *result_ptr; +```rust +#[contractimpl] +impl AvoidUnsafeBlock { + pub fn unsafe_function(n: u64) -> u64 { + unsafe { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; - result.to_bits() - } + let result_ptr: *mut f64 = &mut i; + + (*result_ptr).to_bits() + } + } } ``` -Use instead: +In this example we can see that it creates a raw pointer named `result_ptr`. Then `(*result_ptr).to_bits()` dereferences the raw pointer. This directly accesses the memory location and calls the `to_bits` method on the value stored at that location. + +Raw pointers bypass Rust's type safety system and memory management features. If something goes wrong with the calculations or the value of n, dereferencing the pointer could lead to a memory access violations or undefined behavior. - ```rust -pub fn unsafe_function(n: u64) -> u64 { +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example). + + +## Remediated example + +By removing the raw pointer, the following version eliminates the issue associated with dereferencing memory in an unsafe way. Rust's type safety checks ensure memory is accessed correctly, preventing the potential issues mentioned earlier. + +```rust + #[contractimpl] +impl AvoidUnsafeBlock { + pub fn unsafe_function(n: u64) -> u64 { let mut i = n as f64; let mut y = i.to_bits(); y = 0x5fe6ec85e7de30da - (y >> 1); i = f64::from_bits(y); i *= 1.5 - 0.5 * n as f64 * i * i; i *= 1.5 - 0.5 * n as f64 * i * i; - result.to_bits() -} + i.to_bits() + } +} ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example). + +## How is it detected? + +Checks for usage of `unsafe` blocks. + + -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block). \ No newline at end of file + diff --git a/docs/docs/detectors/12-soroban-version.md b/docs/docs/detectors/12-soroban-version.md index efd7da2a..ac83cae3 100644 --- a/docs/docs/detectors/12-soroban-version.md +++ b/docs/docs/detectors/12-soroban-version.md @@ -1,25 +1,56 @@ # Soroban version -### What it does +## Description -Warns you if you are using an old version of Soroban in the `Cargo.toml`. +- Category: `Best practices` +- Severity: `Enhacement` +- Detector: [`soroban-version`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) +- Test Cases: [`soroban-version-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) + +Using an outdated version of Soroban can lead to issues in our contract. It's a good practice to use the latest version. -### Why is this bad? +## Why is this bad? Using an old version of Soroban can be dangerous, as it may have bugs or security issues. -### Example +## Issue example + + +Consider the following `Cargo.toml`: ```toml -[dependencies] -soroban-sdk = { version = "=20.0.0" } + [dependencies] + soroban-sdk = { version = "=19.0.0" } -[dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } + [dev_dependencies] + soroban-sdk = { version = "=19.0.0", features = ["testutils"] } ``` -Instead, use the latest available version in the `Cargo.toml`. +Problems can arise if the version is not updated to the latest available. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1/vulnerable-example). + + +## Remediated example + +```toml + [dependencies] + // Use the latest version available. + soroban-sdk = { workspace = true } + + [dev_dependencies] + soroban-sdk = { workspace = true, features = ["testutils"] } +``` + +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1/remediated-example) + +## How is it detected? + +Warns you if you are using an old version of Soroban in the `Cargo.toml`. + +## References + +- [Floating Pragma](https://swcregistry.io/docs/SWC-103/) +- [outdated Compiler Version](https://swcregistry.io/docs/SWC-102/) -### Implementation -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version). 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). diff --git a/docs/docs/detectors/14-iterators-over-indexing.md b/docs/docs/detectors/14-iterators-over-indexing.md index 6481a44d..2da13bf7 100644 --- a/docs/docs/detectors/14-iterators-over-indexing.md +++ b/docs/docs/detectors/14-iterators-over-indexing.md @@ -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 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 { + pub fn sum(e: Env) -> Result { let mut ret = 0_i32; let vec = e .storage() @@ -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 { + pub fn sum(e: Env) -> Result { let mut ret = 0_i32; let vec = e .storage() @@ -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. diff --git a/docs/docs/detectors/15-assert-violation.md b/docs/docs/detectors/15-assert-violation.md index 09ca76c7..87e39a97 100644 --- a/docs/docs/detectors/15-assert-violation.md +++ b/docs/docs/detectors/15-assert-violation.md @@ -1,33 +1,54 @@ -# Assert violation +# Assert violation -### What it does​ +## Description -Checks for `assert!` macro usage. +- Category: `Validations and error handling` +- Severity: `Enhancement` +- Detector: [`assert-violation`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation) +- Test Cases: [`assert-violation-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1) -### Why is this bad?​ +The `assert!` macro is used in Rust to ensure that a certain condition holds true at a certain point in your code. -The `assert!` macro can cause the contract to panic. +## Why is it bad? -### Example​ +The `assert!` macro can cause the contract to panic. It is recommended to avoid this, 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. -```rust -pub fn assert_if_greater_than_10(_env: Env, value: u128) -> bool { - assert!(value <= 10, "value should be less than 10"); - true - } +## Issue example + +Consider the following `Soroban` contract: + +```rust + pub fn assert_if_greater_than_10(_env: Env, value: u128) -> bool { + assert!(value <= 10, "value should be less than 10"); + true + } ``` -Use instead: + +The problem arises from the use of the `assert!` macro, if the condition is not met, the contract panics. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1/vulnerable-example). + +## Remediated example + +Avoid the use of `assert!` macro. Instead, use a proper error and return it. ```rust -pub fn assert_if_greater_than_10(_env: Env, value: u128) -> Result { - if value <= 10 { - Ok(true) - } else { - Err(AVError::GreaterThan10) - } - } + pub fn assert_if_greater_than_10(_env: Env, value: u128) -> Result { + if value <= 10 { + Ok(true) + } else { + Err(AVError::GreaterThan10) + } + } ``` -### Implementation -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1/remediated-example). + +## How is it detected? + +Checks for `assert!` macro usage. + +## References + +- [Assert violation](https://docs.alephzero.org/aleph-zero/security-course-by-kudelski-security/ink-developers-security-guideline#assert-violation) diff --git a/docs/docs/detectors/16-unprotected-mapping-operation.md b/docs/docs/detectors/16-unprotected-mapping-operation.md index e06f9710..cbd62470 100644 --- a/docs/docs/detectors/16-unprotected-mapping-operation.md +++ b/docs/docs/detectors/16-unprotected-mapping-operation.md @@ -1,12 +1,15 @@ -# Unprotected Mapping Operation +# Unprotected mapping operation -### What it does +## Description -It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field of the type `AccountId`. +- Category: `Authorization` +- Severity: `Critical` +- Detector: [`unprotected-mapping-operation`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation) +- Test Cases: [`unprotected-mapping-operation-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1) [`unprotected-mapping-operation-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-2) -### Why is this bad? +In Rust, Modifying mappings with an arbitrary key given by the user could lead to several issues. Ideally, only users who have been previously verified should be able to do it. -Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons: +## Why is this bad? - Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitation are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author. @@ -14,12 +17,12 @@ Modifying mappings with an arbitrary key given by users can be a significant vul - Denial-of-Service (DoS) Attacks: If users can set arbitrary keys, they may be able to create mappings with a large number of entries, potentially causing the contract to exceed its gas limit. This could lead to denial-of-service attacks, making the contract unusable for other users. -### Known problems +## Issue example -### Example +Consider the following `Soroban` contract: ```rust - pub fn set_balance(env: Env, address: Address, balance: i128) -> State { + pub fn set_balance(env: Env, address: Address, balance: i128) -> State { // Get the current state. let mut state = Self::get_state(env.clone()); @@ -33,8 +36,14 @@ Modifying mappings with an arbitrary key given by users can be a significant vul state } ``` +The `set_balance()` function allows anyone to call it and modify the account balances in the state. It lacks authorization checks and allows modifying the mutable state directly. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example). + -Use instead: +## Remediated example + +The fix adds an `address.require_auth()` step, likely checking user permissions to update balances. This ensures only authorized users can modify account data. ```rust pub fn set_balance(env: Env, address: Address, balance: i128) -> State { @@ -55,6 +64,8 @@ Use instead: } ``` -### Implementation +The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example). + +## How is it detected? -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation). +It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field of the type `AccountId`. diff --git a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md index 9656b552..bbe1e7e5 100644 --- a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md +++ b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md @@ -1,28 +1,93 @@ # DoS unexpected revert with vector -### What it does +## Description -Checks for array pushes without access control. +- Category: `Authorization` +- Severity: `Critical` +- Detector: [`dos-unexpected-revert-with-vector`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) +- Test Cases: [`dos-unexpected-revert-with-vector-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1) [`dos-unexpected-revert-with-vector-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2) -### Why is this bad? +This issue of DoS through unexpected revert arises when a smart contract does not handle storage size errors correctly, and a user can add an excessive number of entries, leading to an unexpected revert of transactions by other users and a Denial of Service. -Arrays have a maximum size according to the storage cell. If the array is full, the push will revert. This can be used to prevent the execution of a function. +## Why it is bad? -### Known problems +In Soroban smart contracts, a Denial of Service (DoS) issue through unexpected reverts can occur if the contract does not properly manage storage limits or handle errors when the storage capacity is exceeded. If a user adds an excessive number of entries, it could trigger a revert, causing subsequent transactions by other users to fail unexpectedly, leading to a potential Denial of Service. -If the owner validation is performed in an auxiliary function, the warning will be shown, resulting in a false positive. -### Example +## Issue example + +The smart contract we developed for his example allows users to vote for one of different candidates. The smart contract contains a struct named `UnexpectedRevert` that stores the total number of votes, a list of candidates, their votes, and whether an account has voted. It also stores information about the most voted candidate and when the vote will end. + +Consider the following `Soroban` contract: ```rust +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { + // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + candidates: Vec
, + votes: Map, + already_voted: Map, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); //CHECK + let state = State { + total_votes: 0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + candidates: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&STATE, &state); + Ok(state) + } + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { - caller.require_auth(); let mut state = Self::get_state(env.clone()); if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } if state.already_voted.contains_key(caller.clone()) { - return Err(URError::AccountAlreadyVoted); + Err(URError::AccountAlreadyVoted) } else { state.candidates.push_back(candidate.clone()); state.votes.set(candidate, 0); @@ -30,9 +95,121 @@ If the owner validation is performed in an auxiliary function, the warning will } } + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let state = Self::get_state(env.clone()); + state + .votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidates.len() as u64 + } + + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + if index < state.candidates.len() { + Ok(state.candidates.get(index).unwrap()) + } else { + Err(URError::CandidateDoesntExist) + } + } + + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.already_voted.set(caller, true); + let votes = state + .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? + .checked_add(1) + .ok_or(URError::Overflow)?; + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&STATE).unwrap() + } +} ``` +The smart contract has several functions that allow adding a candidate, getting +votes for a specific candidate, getting the account ID of the most voted +candidate, getting the total votes, getting the total number of candidates, +getting a candidate by index, checking if an account has voted, and voting for +a candidate. + +In this case, we see that a vector is being used to store the array of candidates for an election. +Notice how the candidates array push operation has no access control or proper storage management in the function `add_candidate()`, which can cause a revert if the array is full. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example). + +## Remediated example -Use instead: +This issue can be addressed in different ways. + +On the one hand, if the amount of candidates is going to be limited, and only authorized users are going to add new candidates, then enforcing this authorization would be a sufficient fix to prevent attackers from filling the array and producing a denial of service attack. + +```rust +pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + // Require authorization from an admin set at contract initalization. + state.admin.require_auth(); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); + Ok(()) + } +} +``` + +Alternatively, if any user should be authorized to add new candidates, a different data structure should be used, one without the limitations of a vector. For example, a dictionary can be implemented in Soroban by defining a struct for the `Candidate`, accessible through a `DataKey` enum like the one we have here. This data structure does not have the storage limitations of vectors, and using it to handle new candidates would prevent the issue. ```rust pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { @@ -44,6 +221,7 @@ Use instead: if Self::account_has_voted(env.clone(), caller.clone()) { return Err(URError::AccountAlreadyVoted); } else { + // Replace the Vector with a mapping like structure made with a DataKey enum. env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); state.total_candidates += 1; env.storage().instance().set(&DataKey::State, &state); @@ -52,6 +230,14 @@ Use instead: } ``` -### Implementation +This remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example). + +## How is it detected? + +Checks if the contract uses the vec type without using a require auth previously. + +## References -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector). +- [SWC-113](https://swcregistry.io/docs/SWC-113) +- https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/#dos-with-unexpected-revert +- [Ethernaut: King](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/King.sol) diff --git a/docs/docs/detectors/18-unrestricted-transfer-from.md b/docs/docs/detectors/18-unrestricted-transfer-from.md index 7137b205..2c513394 100644 --- a/docs/docs/detectors/18-unrestricted-transfer-from.md +++ b/docs/docs/detectors/18-unrestricted-transfer-from.md @@ -1,60 +1,77 @@ -# Unrestricted Transfer From +# Unrestricted transfer from -### What it does +## Description -It warns you if a `transfer_from` function is called with a user-defined parameter in the `from` field. +- Category: `Validations and error handling` +- Severity: `High` +- Detectors: [`unrestricted-transfer-from`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unrestricted-transfer-from) +- Test Cases: [`unrestricted-transfer-from-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1) -### Why is this bad? +Allowing unrestricted `transfer_from` operations poses a significant issue. When `from` arguments for that function is provided directly by the user, this might enable the withdrawal of funds from any actor with token approval on the contract. -An user Alice can approve a contract to spend their tokens. An user Bob can call that contract, use that allowance to send themselves Alice's tokens. +## Why is this bad? +The absence of proper authorization checks for sensitive operations, like `transfer_from`, can lead to the loss of funds or other undesired consequences. For example, if a user, Alice, approves a contract to spend her tokens, and the contract lacks proper authorization checks, another user, Bob, could invoke the contract and potentially transfer Alice's tokens to himself without her explicit consent. -### Example +## Issue example +Consider the following `Soroban` function: ```rust -pub fn deposit(env: Env, from: Address) -> Result<(), UTFError> { - let mut state: State = Self::get_state(env.clone())?; - state.buyer.require_auth(); - if state.status != Status::Created { - return Err(UTFError::StatusMustBeCreated); + pub fn deposit(env: Env, from: Address) -> Result<(), UTFError> { + let mut state: State = Self::get_state(env.clone())?; + state.buyer.require_auth(); + if state.status != Status::Created { + return Err(UTFError::StatusMustBeCreated); + } + let token_client = token::Client::new(&env, &state.token); + token_client.transfer_from( + &env.current_contract_address(), + &from, + &env.current_contract_address(), + &state.amount, + ); + state.status = Status::Locked; + env.storage().instance().set(&STATE, &state); + Ok(()) } - let token_client = token::Client::new(&env, &state.token); - token_client.transfer_from( - &env.current_contract_address(), - &from, - &env.current_contract_address(), - &state.amount, - ); - state.status = Status::Locked; - env.storage().instance().set(&STATE, &state); - Ok(()) -} ``` +The issue in this `deposit` function arises from the use of `from`, an user-defined parameter as an argument in the `from` field of the `transfer_from` function. Alice can approve a contract to spend their tokens, then Bob can call that contract, use that allowance to send as themselves Alice's tokens. + +The code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1/vulnerable-example). + +## Remediated example -Use instead: +Avoid using user-defined arguments as `from` parameter in `transfer_from`. Instead, use `state.buyer` as shown in the following example. ```rust - pub fn deposit(env: Env) -> Result<(), UTFError> { - let mut state: State = Self::get_state(env.clone())?; - state.buyer.require_auth(); - if state.status != Status::Created { - return Err(UTFError::StatusMustBeCreated); - } - let token_client = token::Client::new(&env, &state.token); - token_client.transfer_from( - &env.current_contract_address(), - &state.buyer, - &env.current_contract_address(), - &state.amount, - ); - state.status = Status::Locked; - env.storage().instance().set(&STATE, &state); - Ok(()) - } + pub fn deposit(env: Env) -> Result<(), UTFError> { + let mut state: State = Self::get_state(env.clone())?; + state.buyer.require_auth(); + if state.status != Status::Created { + return Err(UTFError::StatusMustBeCreated); + } + let token_client = token::Client::new(&env, &state.token); + token_client.transfer_from( + &env.current_contract_address(), + &state.buyer, + &env.current_contract_address(), + &state.amount, + ); + state.status = Status::Locked; + env.storage().instance().set(&STATE, &state); + Ok(()) + } ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1/remediated-example). + +## How is it detected? + +It warns you if a `transfer_from` function is called with a user-defined parameter in the `from` field. + +## References + +- [Slither: Arbitrary from in transferFrom](https://github.com/crytic/slither/wiki/Detector-Documentation#arbitrary-from-in-transferfrom) -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unrestricted-transfer-from). diff --git a/docs/docs/detectors/19-unsafe-map-get.md b/docs/docs/detectors/19-unsafe-map-get.md index 53175ae2..98e69e68 100644 --- a/docs/docs/detectors/19-unsafe-map-get.md +++ b/docs/docs/detectors/19-unsafe-map-get.md @@ -1,14 +1,21 @@ # Unsafe map get -### What it does +## Description -This detector identifies instances where unsafe methods like `get`, `get_unchecked`, and `try_get_unchecked` are used on `Map` objects in Soroban smart contracts. +- Category: `Validations and error handling` +- Severity: `Medium` +- Detectors: [`unsafe-map-get`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-map-get) +- Test Cases: [`unsafe-map-get-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-map-get/unsafe-map-get-1) -### Why is this bad? +The use of certain methods (`get`, `get_unchecked`, `try_get_unchecked`) on a `Map` object in the Soroban environment without appropriate error handling can lead to potential runtime panics. This issue stems from accessing the map's values with keys that may not exist, without using safer alternatives that check the existence of the key. -These methods are risky because they can lead to panics if the key does not exist in the map. Using these methods without proper checks increases the risk of runtime errors that can disrupt the execution of the smart contract and potentially lead to unexpected behavior or denial of service. +## Why is it bad? -### Example +These methods can lead to panics if the key does not exist in the map. Using these methods without proper checks increases the risk of runtime errors that can disrupt the execution of the smart contract and potentially lead to unexpected behavior or denial of service. + +## Issue example + +Consider the following `Soroban` contract: ```rust pub fn get_from_map(env: Env) -> Option { @@ -18,8 +25,9 @@ pub fn get_from_map(env: Env) -> Option { map.get(1) } ``` +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example). -Use instead: +## Remediated example ```rust pub fn get_map_with_different_values(env: Env, key: i32) -> Result, Error> { @@ -34,7 +42,8 @@ pub fn get_map_with_different_values(env: Env, key: i32) -> Result, } ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example). -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-map-get). +## How is it detected? +Checks for array pushes without access control. diff --git a/docs/docs/detectors/2-unsafe-unwrap.md b/docs/docs/detectors/2-unsafe-unwrap.md index a49f21b5..56dbb8f0 100644 --- a/docs/docs/detectors/2-unsafe-unwrap.md +++ b/docs/docs/detectors/2-unsafe-unwrap.md @@ -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 { - 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 { + 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 { - 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 { + 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). + diff --git a/docs/docs/detectors/21-incorrect-exponentiation.md b/docs/docs/detectors/21-incorrect-exponentiation.md index b2584959..b9726d37 100644 --- a/docs/docs/detectors/21-incorrect-exponentiation.md +++ b/docs/docs/detectors/21-incorrect-exponentiation.md @@ -1,36 +1,60 @@ -# Incorrect Exponentiation +# Incorrect exponentiation -### What it does +## Description -Warns about `^` being a `bit XOR` operation instead of an exponentiation. +- Vulnerability Category: `Arithmetic` +- Vulnerability Severity: `Critical` +- Detectors: [`incorrect-exponentiation`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/incorrect-exponentiation) +- Test Cases: [`incorrect-exponentiation-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1) + +The operator `^` is not an exponential operator, it is a bitwise XOR. Make sure to use `pow()` instead for exponentiation. In case of performing a XOR operation, use `.bitxor()` for clarity. -### Why is this bad? +## Why is it bad? -It can introduce unexpected behaviour in the smart contract. +It can produce unexpected behaviour in the smart contract. -#### More info +## Issue example -- https://doc.rust-lang.org/std/ops/trait.BitXor.html#tymethod.bitxor +In the following example, the `^` operand is being used for exponentiation. But in Rust, `^` is the operand for an XOR operation. If misused, this could lead to unexpected behaviour in our contract. -### Example +Consider the following `Soroban` contract: ```rust - pub fn init(e: Env){ - e.storage() - .instance() - .set::(&DataKey::Data, &((255_u128 ^ 2) - 1)); + pub fn exp_data_3(e: Env) -> u128 { + let mut data = e.storage() + .instance() + .get::(&DataKey::Data) + .expect("Data not found"); + + data ^= 3; + data } ``` -Use instead: + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1/vulnerable-example). + +## Remediated example + +A possible solution is to use the method `pow()`. But, if a XOR operation is wanted, `.bitxor()` method is recommended. ```rust - pub fn init(e: Env) { - e.storage() - .instance() - .set::(&DataKey::Data, &(255_u128.pow(2) - 1)); + pub fn exp_data_3(e: Env) -> u128 { + let data = e.storage() + .instance() + .get::(&DataKey::Data) + .expect("Data not found"); + + data.pow(3) } ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1/remediated-example). + +## How is it detected? + +Warns about `^` being a `bit XOR` operation instead of an exponentiation. + + +## References -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/incorrect-exponentiation). +- https://doc.rust-lang.org/std/ops/trait.BitXor.html diff --git a/docs/docs/detectors/22-integer-overflow -or-underflow.md b/docs/docs/detectors/22-integer-overflow -or-underflow.md new file mode 100644 index 00000000..38ee757c --- /dev/null +++ b/docs/docs/detectors/22-integer-overflow -or-underflow.md @@ -0,0 +1,64 @@ +# Integer overflow or underflow + +## Description + +- Category: `Arithmetic` +- Severity: `Critical` +- Detectors: [`integer-overflow-or-underflow`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/integer-overflow-or-underflow) +- Test Cases: [`integer-overflow-or-underflow-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1) +[`integer-overflow-or-underflow-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2) +[`integer-overflow-or-underflow-3`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3) +[`integer-overflow-or-underflow-4`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4) +[`integer-overflow-or-underflow-5`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5) + +In Rust, arithmetic operations can result in a value that falls outside the allowed numerical range for a given type. When the result exceeds the maximum value of the range, it's called an overflow, and when it falls below the minimum value of the range, it's called an underflow. + +## Why is this bad? + +If there are arithmetic operations with overflow or underflow problems, and if errors are not handled correctly, incorrect results will be generated, bringing potential problems for the contract. Additionally, these types of errors can allow attackers to drain a contract’s funds or manipulate its logic. + +## Issue example + +Consider the following `Soroban` contract: + +```rust + + pub fn add(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current + value; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + +``` + +In this example, an operation is performed on two u32 values without any safeguards against overflow if it occurs. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example). + + +## Remediated example + +```rust +pub fn add(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_add(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } +``` +In this example, the `checked_add` method is used to perform the addition. It returns the sum if no overflow occurs; otherwise, it returns `None`, with an OverflowError variant indicating that an overflow error has occurred. + + + +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example). + +## How is it detected? + +Checks if there’s any numerical overflow or underflow. + + + + diff --git a/docs/docs/detectors/3-unsafe-expect.md b/docs/docs/detectors/3-unsafe-expect.md index b1abfc42..9a30f69d 100644 --- a/docs/docs/detectors/3-unsafe-expect.md +++ b/docs/docs/detectors/3-unsafe-expect.md @@ -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 { - 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 { - 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). + diff --git a/docs/docs/detectors/5-insufficiently-random-values.md b/docs/docs/detectors/5-insufficiently-random-values.md index 62bac4c7..87812d6e 100644 --- a/docs/docs/detectors/5-insufficiently-random-values.md +++ b/docs/docs/detectors/5-insufficiently-random-values.md @@ -1,17 +1,25 @@ -# Insuficciently random values +# Insufficiently random values -### What it does -Checks the usage of `ledger().timestamp()` or `ledger().sequence()` for generation of random numbers. +## Description + +- Category: `Block attributes` +- Severity: `Critical` +- Detector: [`insufficiently-random-values`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values) +- Test Cases: [`insufficiently-random-values-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1) + +Using block attributes like `timestamp` or `sequence` for random number generation in Soroban smart contracts is not recommended due to the predictability of these values. Block attributes are publicly visible and deterministic, making it easy for malicious actors to anticipate their values and manipulate outcomes to their advantage. It's important to use a source that is both unpredictable and external to the blockchain environment, reducing the potential for malicious exploitation. + +## Why is this bad? + +Using `ledger().timestamp()` is not recommended because it could be potentially manipulated by validator, which might lead to potential problems. On the other hand, `ledger().sequence()` is publicly available. An attacker could predict the random number to be generated to manipulate the code and perform an attack on the contract. -### Why is this bad? -Using `ledger().timestamp()` is not recommended because it could be potentially manipulated by validator. On the other hand, `ledger().sequence()` is publicly available, an attacker could predict the random number to be generated. -### Example +## Issue example + +Consider the following `Soroban` contract: ```rust -#[contractimpl] -impl Contract { - pub fn generate_random_value_timestamp(env: Env, max_val: u64) -> Result { +pub fn generate_random_value_timestamp(env: Env, max_val: u64) -> Result { if max_val == 0 { Err(Error::MaxValZero) } else { @@ -19,6 +27,7 @@ impl Contract { Ok(val) } } + pub fn generate_random_value_sequence(env: Env, max_val: u32) -> Result { if max_val == 0 { Err(Error::MaxValZero) @@ -27,25 +36,46 @@ impl Contract { Ok(val) } } -} + ``` -Use instead: +The issue lies in these functions use of blockchain-provided data like block timestamp and sequence number for pseudo-random number generation. This reliance on predictable blockchain data makes the generated values susceptible to manipulation by attackers. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example). + + +## Remediated example + +Avoid using block attributes like `timestamp` or `sequence` for randomness generation, and consider using PRNG instead. ```rust -#[contractimpl] -impl Contract { - pub fn generate_random_value(env: Env, max_val: u64) -> Result { + + pub fn generate_random_value(env: Env, max_val: u64) -> Result { if max_val == 0 { Err(Error::MaxValZero) } else { - let val = env.prng().u64_in_range(0..max_val); + let val = env.prng().gen_range(0..max_val); Ok(val) } } -} + ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example). + +## How is it detected? + +Checks the usage of `ledger().timestamp()` or `ledger().sequence()` for generation of random numbers. + + +## References + +- https://dasp.co/#item-6 +- https://blog.sigmaprime.io/solidity-security.html#SP-6 +- [SWC-120](https://swcregistry.io/docs/SWC-120) +- [SWC-116](https://swcregistry.io/docs/SWC-116) +- [Ethernaut: Coinflip](https://ethernaut.openzeppelin.com/level/0x4dF32584890A0026e56f7535d0f2C6486753624f) +- [Slither: Weak PRNG](https://github.com/crytic/slither/wiki/Detector-Documentation#weak-PRNG) +- [Slither: Dangerous usage of block.timestamp](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp) -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values). \ No newline at end of file + diff --git a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md index 94824131..6b4e5f4d 100644 --- a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md +++ b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md @@ -1,17 +1,26 @@ -# Unprotected update of current contract wasm +# Unprotected update current contract wasm -### What it does +## Description -It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. +- Category: `Authorization` +- Severity: `Critical` +- Detector: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) +- Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) + +It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. -### Why is this bad? + +## Why is this bad? If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it. -### Example +## Issue example + +Consider the following `Soroban` contract: ```rust -#[contractimpl] + + #[contractimpl] impl UpgradeableContract { pub fn init(e: Env, admin: Address) { e.storage().instance().set(&DataKey::Admin, &admin); @@ -22,17 +31,24 @@ impl UpgradeableContract { } pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); - e.deployer().update_current_contract_wasm(new_wasm_hash); } } -``` -Use instead: +``` + +This contract allows upgrades through the `update_current_contract_wasm` function. If just anyone can call this function, they could modify the contract behaviour. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example). + + +## Remediated example + +To prevent this, the function should be restricted to administrators or authorized users only. ```rust -#[contractimpl] + + #[contractimpl] impl UpgradeableContract { pub fn init(e: Env, admin: Address) { e.storage().instance().set(&DataKey::Admin, &admin); @@ -51,6 +67,8 @@ impl UpgradeableContract { } ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example). -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) +## How is it detected? + +It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. diff --git a/docs/docs/detectors/7-avoid-core-mem-forget.md b/docs/docs/detectors/7-avoid-core-mem-forget.md index f2cafbf0..fe03abec 100644 --- a/docs/docs/detectors/7-avoid-core-mem-forget.md +++ b/docs/docs/detectors/7-avoid-core-mem-forget.md @@ -1,31 +1,45 @@ -# Avoid core mem forget usage +# Avoid core::mem::forget usage -### What it does +## Description -Checks for `core::mem::forget` usage. +- Category: `Best practices` +- Severity: `Enhancement` +- Detector: [`avoid-core-mem-forget`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) +- Test Cases: [`avoid-core-mem-forget-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) -### Why is this bad? -This is a bad practice because it can lead to memory leaks, resource leaks and logic errors. +The `core::mem::forget` function is used to forget about a value without running its destructor. -### Example +## Why is this bad? -```rust -pub fn forget_something(n: WithoutCopy) -> u64 { - core::mem::forget(n); - 0 -} -``` +Using this function is a bad practice because this can lead to memory leaks, resource leaks and logic errors. + +## Issue example -Use instead: +Consider the following `Soroban` contract: ```rust -pub fn forget_something(n: WithoutCopy) -> u64 { - let _ = n; - 0 -} + + pub fn forget_something(n: WithoutCopy) -> u64 { + core::mem::forget(n); + 0 + } ``` -### Implementation +The problem arises from the use of the `core::mem::forget` function. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1/vulnerable-example). + + +## Remediated example + +Use the pattern `let _ = n;` or the `.drop()` method instead of `core::mem::forget(n);`. + +## How is it detected? + +Checks for `core::mem::forget` usage. + +## References -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget). \ No newline at end of file +- [Memory Management](https://docs.alephzero.org/aleph-zero/security-course-by-kudelski-security/ink-developers-security-guideline#memory-management) + diff --git a/docs/docs/detectors/8-set-contract-storage.md b/docs/docs/detectors/8-set-contract-storage.md index 85350694..f7caa866 100644 --- a/docs/docs/detectors/8-set-contract-storage.md +++ b/docs/docs/detectors/8-set-contract-storage.md @@ -1,34 +1,74 @@ # Set contract storage -### What it does +## Description -Checks for calls to `env.storage()` without a prior call to `env.require_auth()`. +- Category: `Authorization` +- Severity: `Critical` +- Detector: [`set-contract-storage`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) +- Test Cases: [`set-contract-storage-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1) [`set-contract-storage-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2) [`set-contract-storage-3`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) [`set-contract-storage-4`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-4) -### Why is this bad? +Smart contracts can store important information in memory which changes through the contract's lifecycle. Changes happen via user interaction with the smart contract. An unauthorized set contract storage issue happens when a smart contract call allows a user to set or modify contract memory when he was not supposed to be authorized. -Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations. +## Why is this bad? -### Known problems +Unauthorized access to storage by a user can lead to serious issues, as they might manipulate memory data to attack the contract. -Only check the function call, so false positives could result. +## Issue example -### Example +Consider the following `Soroban` contract: ```rust -fn set_contract_storage(env: Env) { - let _storage = env.storage().instance(); -} + + #[contractimpl] +impl SetContractStorage { + /// Increment an internal counter; return the new value. + pub fn increment(env: Env, user: Address) -> u32 { + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&user).unwrap_or_default(); + count += 1; + storage.set(&user, &count); + storage.extend_ttl(100, 100); + count + } +} + ``` -Use instead: +In this example we see that any user may access the SetContractStorage() function, and therefore modify the value of the internal counter. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example). + + +## Remediated example + +Arbitrary users should not have control over keys because it implies writing any value of a mapping, lazy variable, or the main struct of the contract located in position 0 of the storage. To prevent this issue, set access control and proper authorization validation for the SetContractStorage() function. + +For example, the code below, ensures only the authorized users can call SetContractStorage(). ```rust -fn set_contract_storage(env: Env, user: Address) { - user.require_auth(); - let _storage = env.storage().instance(); + + #[contractimpl] +impl SetContractStorage { + /// Increment an internal counter; return the new value. + pub fn increment(env: Env, user: Address) -> u32 { + user.require_auth(); + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&user).unwrap_or_default(); + count += 1; + storage.set(&user, &count); + storage.extend_ttl(100, 100); + count + } } + ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1/remediated-example). + +## How is it detected? + +Checks for calls to env.storage() without a prior call to env.require_auth(). + + -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage). + diff --git a/docs/docs/detectors/9-avoid-panic-error.md b/docs/docs/detectors/9-avoid-panic-error.md index 5e0fc181..125cb86f 100644 --- a/docs/docs/detectors/9-avoid-panic-error.md +++ b/docs/docs/detectors/9-avoid-panic-error.md @@ -1,38 +1,54 @@ -# Panic error +# Avoid panic error -### What it does +## Description -The `panic!` macro is used to stop execution when a condition is not met. -This is useful for testing and prototyping, but should be avoided in production code. +- Category: `Validations and error handling` +- Severity: `Enhancement` +- Detector: [`avoid-panic-error`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) +- Test Cases: [`avoid-panic-error-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) -### Why is this bad? +The panic! macro is used to stop execution when a condition is not met. This is useful for testing and prototyping, but should be avoided in production code. -The usage of `panic!` is not recommended because it will stop the execution of the caller contract. +Using `Result` as return type for functions that can fail is the idiomatic way to handle errors in Rust. The `Result` type is an enum that can be either `Ok` or `Err`. The `Err` variant can contain an error message. The `?` operator can be used to propagate the error message to the caller. -### Known problems +This way, the caller can decide how to handle the error, although the state of the contract is always reverted on the callee. -While this linter detects explicit calls to `panic!`, there are some ways to raise a panic such as `unwrap()` or `expect()`. +## Why is this bad? -### Example +The usage of `panic!` is not recommended because it will stop the execution of the caller contract. This could lead the contract to an inconsistent state if the execution stops in the middle of state changes. Additionally, if execution stops, it could cause a transaction to fail. + +## Issue example + +In the following example, the `panic!` command is being used to handle errors, disallowing the caller to handle the error in a different way, and completely stopping execution of the caller contract. + +Consider the following `Soroban` contract: ```rust pub fn add(env: Env, value: u32) -> u32 { - let storage = env.storage().instance(); - let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); - match count.checked_add(value) { - Some(value) => count = value, - None => panic!("Overflow error"), - } - storage.set(&COUNTER, &count); - storage.extend_ttl(100, 100); - count -} + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + match count.checked_add(value) { + Some(value) => count = value, + None => panic!("Overflow error"), + } + storage.set(&COUNTER, &count); + storage.extend_ttl(100, 100); + count + } ``` +The add function takes a value as an argument and adds it to the value stored in the contract's storage. The function first checks if the addition will cause an overflow. If the addition will cause an overflow, the function will panic. If the addition will not cause an overflow, the function will add the value to the contract's storage. -Use instead: +The usage of panic! in this example, is not recommended because it will stop the execution of the caller contract. If the method was called by the user, then he will receive ContractTrapped as the only error message. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example). + + +## Remediated example + +A possible remediation goes as follows: ```rust -pub fn add(env: Env, value: u32) -> Result { + pub fn add(env: Env, value: u32) -> Result { let storage = env.storage().instance(); let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); match count.checked_add(value) { @@ -43,8 +59,24 @@ pub fn add(env: Env, value: u32) -> Result { storage.extend_ttl(100, 100); Ok(count) } + ``` +And adding the following Error enum: + +```rust +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, +} +``` +By first defining the Error enum and then returning a Result<(), Error>, more information is added to the caller and, e.g. the caller contract could decide to revert the transaction or to continue execution. + +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example). + +## How is it detected? + +Checks the use of the macro `panic!`. -### Implementation -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error). \ No newline at end of file + diff --git a/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md b/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md new file mode 100644 index 00000000..5586d367 --- /dev/null +++ b/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md @@ -0,0 +1,246 @@ +# DoS unexpected revert with vector + +## Description + +- Vulnerability Category: `DoS` +- Severity: `Medium` +- Detectors: [`dos-unexpected-revert-with-vector`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) +- Test Cases: [`dos-unexpected-revert-with-vector-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1), [`dos-unexpected-revert-with-vector-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2) + +This vulnerability of DoS through unexpected revert arises when a smart +contract does not handle storage size errors correctly, and a user can add an +excessive number of entries, leading to an unexpected revert of transactions +by other users and a Denial of Service. This vulnerability can be exploited by +an attacker to perform a DoS attack on the network and can result in lost +funds, poor user experience, and even harm the network's overall security. + +## Exploit Scenario + +The vulnerable smart contract we developed for his example allows users to +vote for one of different candidates. +The smart contract contains a struct named `UnexpectedRevert` that stores the +total number of votes, a list of candidates, their votes, and whether an +account has voted. It also stores information about the most voted candidate +and when the vote will end. + +```rust +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { + // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + candidates: Vec
, + votes: Map, + already_voted: Map, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); //CHECK + let state = State { + total_votes: 0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + candidates: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&STATE, &state); + Ok(state) + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); + Ok(()) + } + } + + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let state = Self::get_state(env.clone()); + state + .votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidates.len() as u64 + } + + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + if index < state.candidates.len() { + Ok(state.candidates.get(index).unwrap()) + } else { + Err(URError::CandidateDoesntExist) + } + } + + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.already_voted.set(caller, true); + let votes = state + .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? + .checked_add(1) + .ok_or(URError::Overflow)?; + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&STATE).unwrap() + } +} +``` + +The smart contract has several functions that allow adding a candidate, getting +votes for a specific candidate, getting the account ID of the most voted +candidate, getting the total votes, getting the total number of candidates, +getting a candidate by index, checking if an account has voted, and voting for +a candidate. + +In this case, we see that a vector is being used to store the array of candidates for an election. +Notice how the candidates array push operation has no access control or proper storage management in the function `add_candidate()`, which can cause a revert if the array is full. + + +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs). + +## Remediation + +This issue can be addressed in different ways. + +On the one hand, if the amount of candidates is going to be limited, and only authorized users are going to add new candidates, then enforcing this authorization would be a sufficient fix to prevent attackers from filling the array and producing a denial of service attack. + +```rust +pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + // Require authorization from an admin set at contract initalization. + state.admin.require_auth(); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); + Ok(()) + } +} +``` +This remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs). + + +Alternatively, if any user should be authorized to add new candidates, a different data structure should be used, one without the limitations of a vector. For example, a dictionary can be implemented in Soroban by defining a struct for the `Candidate`, accessible through a `DataKey` enum like the one we have here. This data structure does not have the storage limitations of vectors, and using it to handle new candidates would prevent the issue. + +```rust + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if Self::account_has_voted(env.clone(), caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + // Replace the Vector with a mapping like structure made with a DataKey enum. + env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); + state.total_candidates += 1; + env.storage().instance().set(&DataKey::State, &state); + Ok(()) + } +} +``` + +This remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs). + +## References + +- [SWC-113](https://swcregistry.io/docs/SWC-113) +- https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/#dos-with-unexpected-revert +- [Ethernaut: King](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/King.sol) \ No newline at end of file diff --git a/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md b/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md deleted file mode 100644 index 803af1db..00000000 --- a/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md +++ /dev/null @@ -1,59 +0,0 @@ -# Unprotected update current contract wasm - -## Description - -- Vulnerability Category: `Authorization` -- Vulnerability Severity: `Critical` -- Detectors: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) -- Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) - -It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it. - -## Exploit Scenario - -Consider the following `Soroban` contract: - -```rust -#[contractimpl] -impl UpgradeableContract { - pub fn init(e: Env, admin: Address) { - e.storage().instance().set(&DataKey::Admin, &admin); - } - - pub fn version() -> u32 { - 1 - } - - pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - e.deployer().update_current_contract_wasm(new_wasm_hash); - } -} -``` -This contract allows upgrades through the `update_current_contract_wasm` function. If just anyone can call this function, they could modify the contract behaviour. - -The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example). - -## Remediation - -To prevent this, the function should be restricted to administrators or authorized users only. - -```rust -#[contractimpl] -impl UpgradeableContract { - pub fn init(e: Env, admin: Address) { - e.storage().instance().set(&DataKey::Admin, &admin); - } - - pub fn version() -> u32 { - 1 - } - - pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); - admin.require_auth(); - - e.deployer().update_current_contract_wasm(new_wasm_hash); - } -} -``` -The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example). \ No newline at end of file diff --git a/scripts/run-tests.py b/scripts/run-tests.py index eba5931c..1b2e3f3e 100644 --- a/scripts/run-tests.py +++ b/scripts/run-tests.py @@ -34,10 +34,10 @@ def run_tests(detector): def run_unit_tests(root): start_time = time.time() - returncode, stdout, _ = run_subprocess(["cargo", "test", "--all-features"], root) + returncode, _, stderr = run_subprocess(["cargo", "test", "--all-features"], root) print_results( returncode, - stdout, + stderr, "unit-test", root, time.time() - start_time, @@ -48,6 +48,8 @@ def run_unit_tests(root): def run_integration_tests(detector, root): start_time = time.time() + detectors_path = os.path.join(os.getcwd(), "detectors") + returncode, stdout, _ = run_subprocess( [ "cargo", @@ -56,7 +58,7 @@ def run_integration_tests(detector, root): detector, "--metadata", "--local-detectors", - os.path.join(os.getcwd(), "detectors"), + detectors_path, ], root, ) @@ -70,7 +72,7 @@ def run_integration_tests(detector, root): detector_metadata = parse_json_from_string(stdout) if not isinstance(detector_metadata, dict): - print("Failed to extract JSON:", detector_metadata) + print("Failed to extract JSON:\n", detector_metadata) return True detector_key = detector.replace("-", "_") diff --git a/scripts/utils.py b/scripts/utils.py index ffa7008d..b2996b2f 100644 --- a/scripts/utils.py +++ b/scripts/utils.py @@ -37,7 +37,7 @@ def parse_json_from_string(console_output): except json.JSONDecodeError: return "Extracted string is not valid JSON" else: - return "No JSON found in the console output" + return console_output def run_subprocess(command: list, cwd: str): diff --git a/templates/test-case/Cargo.toml b/templates/test-case/Cargo.toml index 4f5e2bca..9a687e66 100644 --- a/templates/test-case/Cargo.toml +++ b/templates/test-case/Cargo.toml @@ -1,13 +1,13 @@ [package] +edition = "2021" name = "incrementor" version = "0.1.0" -edition = "2021" [lib] crate-type = ["cdylib"] [dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [dev-dependencies] soroban-sdk = { version = "=20.0.0", features = ["testutils"] } @@ -16,15 +16,16 @@ soroban-sdk = { version = "=20.0.0", features = ["testutils"] } testutils = ["soroban-sdk/testutils"] [profile.release] -opt-level = "z" -overflow-checks = true +codegen-units = 1 debug = 0 -strip = "symbols" debug-assertions = false -panic = "abort" -codegen-units = 1 lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" [profile.release-with-logs] -inherits = "release" debug-assertions = true + +inherits = "release" diff --git a/test-cases/assert-violation/Cargo.toml b/test-cases/assert-violation/Cargo.toml index 319daf6b..4aa7883f 100644 --- a/test-cases/assert-violation/Cargo.toml +++ b/test-cases/assert-violation/Cargo.toml @@ -4,7 +4,7 @@ members = ["assert-violation-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/avoid-core-mem-forget/Cargo.toml b/test-cases/avoid-core-mem-forget/Cargo.toml index 0674889d..9858f1af 100644 --- a/test-cases/avoid-core-mem-forget/Cargo.toml +++ b/test-cases/avoid-core-mem-forget/Cargo.toml @@ -4,7 +4,7 @@ members = ["avoid-core-mem-forget-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/avoid-panic-error/Cargo.toml b/test-cases/avoid-panic-error/Cargo.toml index 059f7eca..993a8540 100644 --- a/test-cases/avoid-panic-error/Cargo.toml +++ b/test-cases/avoid-panic-error/Cargo.toml @@ -4,7 +4,7 @@ members = ["avoid-panic-error-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/avoid-unsafe-block/Cargo.toml b/test-cases/avoid-unsafe-block/Cargo.toml index 9abd5348..25ba830e 100644 --- a/test-cases/avoid-unsafe-block/Cargo.toml +++ b/test-cases/avoid-unsafe-block/Cargo.toml @@ -4,7 +4,7 @@ members = ["avoid-unsafe-block-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/divide-before-multiply/Cargo.toml b/test-cases/divide-before-multiply/Cargo.toml index 5b7f4963..c01e744c 100644 --- a/test-cases/divide-before-multiply/Cargo.toml +++ b/test-cases/divide-before-multiply/Cargo.toml @@ -4,7 +4,7 @@ members = ["divide-before-multiply-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/dos-unbounded-operation/Cargo.toml b/test-cases/dos-unbounded-operation/Cargo.toml index 68a4495c..3bbb7890 100644 --- a/test-cases/dos-unbounded-operation/Cargo.toml +++ b/test-cases/dos-unbounded-operation/Cargo.toml @@ -4,7 +4,7 @@ members = ["dos-unbounded-operation-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/dos-unexpected-revert-with-vector/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml index 82ef8b11..eb91610f 100644 --- a/test-cases/dos-unexpected-revert-with-vector/Cargo.toml +++ b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml @@ -4,7 +4,7 @@ members = ["dos-unexpected-revert-with-vector-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 @@ -19,4 +19,3 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" - diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs index 3dc6323c..d8c39715 100644 --- a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs @@ -1,6 +1,9 @@ #![no_std] -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] @@ -19,105 +22,65 @@ pub enum URError { #[contracttype] pub struct State { total_votes: u64, - total_candidates: u32, + candidates: Vec
, + votes: Map, + already_voted: Map, most_voted_candidate: Address, candidate_votes: u64, vote_timestamp_end: u64, + admin: Address, } -#[derive(Debug, Clone, PartialEq)] -#[contracttype] -pub struct Candidate { - votes: u64, -} - -#[derive(Default)] -#[contracttype] -pub struct AlreadyVoted { - voted: bool, -} - -#[contracttype] -pub enum DataKey { - State, - AlreadyVoted(Address), - Candidate(Address), -} +const STATE: Symbol = symbol_short!("STATE"); #[contract] pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { - pub fn set_candidate(env: Env, candidate: Address, votes: u64) { - let cand = Candidate { votes }; - - env.storage() - .instance() - .set(&DataKey::Candidate(candidate), &cand); - } - - pub fn retrieve_candidate(env: Env, candidate: Address) -> Result { - env.storage() - .instance() - .get(&DataKey::Candidate(candidate)) - .unwrap_or(Err(URError::CandidateDoesntExist)) - } - - pub fn init(env: Env, end_timestamp: u64) -> Result { + pub fn init(env: Env, end_timestamp: u64, admin: Address) -> Result { if end_timestamp <= env.ledger().timestamp() { return Err(URError::TimestampBeforeCurrentBlock); } let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); - let zero_addr = Address::from_string(&zero_string); // Whenever this is zero address it will mean no candidate has yet been more voted + let zero_addr = Address::from_string(&zero_string); //CHECK let state = State { total_votes: 0, - total_candidates: 0, most_voted_candidate: zero_addr, candidate_votes: 0, + candidates: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), vote_timestamp_end: end_timestamp, + admin, }; - env.storage().instance().set(&DataKey::State, &state); + env.storage().instance().set(&STATE, &state); Ok(state) } pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { - caller.require_auth(); let mut state = Self::get_state(env.clone()); + state.admin.require_auth(); if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } - if Self::account_has_voted(env.clone(), caller.clone()) { + if state.already_voted.contains_key(caller.clone()) { Err(URError::AccountAlreadyVoted) } else { - env.storage().instance().set( - &DataKey::Candidate(candidate.clone()), - &Candidate { votes: 0 }, - ); - state.total_candidates += 1; - env.storage().instance().set(&DataKey::State, &state); + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); Ok(()) } } - pub fn account_has_voted(env: Env, caller: Address) -> bool { - let already_voted: AlreadyVoted = env - .storage() - .instance() - .get(&DataKey::AlreadyVoted(caller)) - .unwrap_or_default(); - already_voted.voted - } - pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { - let result: Option = - env.storage().instance().get(&DataKey::Candidate(candidate)); - match result { - Some(cand) => Ok(cand.votes), - None => Err(URError::CandidateDoesntExist), - } + let state = Self::get_state(env.clone()); + state + .votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) } pub fn most_voted_candidate_votes(env: Env) -> u64 { @@ -135,19 +98,25 @@ impl UnexpectedRevert { state.total_votes } - pub fn get_total_candidates(env: Env) -> u32 { + pub fn get_total_candidates(env: Env) -> u64 { let state = Self::get_state(env); - state.total_candidates + state.candidates.len() as u64 } - pub fn get_candidate(env: Env, addr: Address) -> Result { - let result: Option = env.storage().instance().get(&DataKey::Candidate(addr)); - match result { - Some(cand) => Ok(cand), - None => Err(URError::CandidateDoesntExist), + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + if index < state.candidates.len() { + Ok(state.candidates.get(index).unwrap()) + } else { + Err(URError::CandidateDoesntExist) } } + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { caller.require_auth(); let mut state = Self::get_state(env.clone()); @@ -155,20 +124,18 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } - if Self::account_has_voted(env.clone(), caller.clone()) { + if state.already_voted.contains_key(caller.clone()) { Err(URError::AccountAlreadyVoted) } else { - env.storage().instance().set( - &DataKey::AlreadyVoted(caller.clone()), - &AlreadyVoted { voted: true }, - ); - let votes = Self::get_candidate(env.clone(), candidate.clone())? + state.already_voted.set(caller, true); + let votes = state .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? .checked_add(1) .ok_or(URError::Overflow)?; - env.storage() - .instance() - .set(&DataKey::Candidate(candidate.clone()), &Candidate { votes }); + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; if state.candidate_votes < votes { state.candidate_votes = votes; state.most_voted_candidate = candidate; @@ -183,6 +150,6 @@ impl UnexpectedRevert { } pub fn get_state(env: Env) -> State { - env.storage().instance().get(&DataKey::State).unwrap() + env.storage().instance().get(&STATE).unwrap() } } diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..2c803be8 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-remediated-2" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..65ee078a --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs @@ -0,0 +1,187 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { + // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + total_candidates: u32, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct Candidate { + votes: u64, +} + +#[derive(Default)] +#[contracttype] +pub struct AlreadyVoted { + voted: bool, +} + +#[contracttype] +pub enum DataKey { + State, + AlreadyVoted(Address), + Candidate(Address), +} + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + pub fn set_candidate(env: Env, candidate: Address, votes: u64) { + let cand = Candidate { votes }; + + env.storage() + .instance() + .set(&DataKey::Candidate(candidate), &cand); + } + + pub fn retrieve_candidate(env: Env, candidate: Address) -> Result { + env.storage() + .instance() + .get(&DataKey::Candidate(candidate)) + .unwrap_or(Err(URError::CandidateDoesntExist)) + } + + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); // Whenever this is zero address it will mean no candidate has yet been more voted + let state = State { + total_votes: 0, + total_candidates: 0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&DataKey::State, &state); + Ok(state) + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if Self::account_has_voted(env.clone(), caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + env.storage().instance().set( + &DataKey::Candidate(candidate.clone()), + &Candidate { votes: 0 }, + ); + state.total_candidates += 1; + env.storage().instance().set(&DataKey::State, &state); + Ok(()) + } + } + + pub fn account_has_voted(env: Env, caller: Address) -> bool { + let already_voted: AlreadyVoted = env + .storage() + .instance() + .get(&DataKey::AlreadyVoted(caller)) + .unwrap_or_default(); + already_voted.voted + } + + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let result: Option = + env.storage().instance().get(&DataKey::Candidate(candidate)); + match result { + Some(cand) => Ok(cand.votes), + None => Err(URError::CandidateDoesntExist), + } + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u32 { + let state = Self::get_state(env); + state.total_candidates + } + + pub fn get_candidate(env: Env, addr: Address) -> Result { + let result: Option = env.storage().instance().get(&DataKey::Candidate(addr)); + match result { + Some(cand) => Ok(cand), + None => Err(URError::CandidateDoesntExist), + } + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if Self::account_has_voted(env.clone(), caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + env.storage().instance().set( + &DataKey::AlreadyVoted(caller.clone()), + &AlreadyVoted { voted: true }, + ); + let votes = Self::get_candidate(env.clone(), candidate.clone())? + .votes + .checked_add(1) + .ok_or(URError::Overflow)?; + env.storage() + .instance() + .set(&DataKey::Candidate(candidate.clone()), &Candidate { votes }); + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&DataKey::State).unwrap() + } +} diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..c250d033 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-vulnerable-2" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..728137e9 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs @@ -0,0 +1,152 @@ +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { + // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + candidates: Vec
, + votes: Map, + already_voted: Map, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); //CHECK + let state = State { + total_votes: 0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + candidates: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&STATE, &state); + Ok(state) + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); + Ok(()) + } + } + + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let state = Self::get_state(env.clone()); + state + .votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidates.len() as u64 + } + + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + if index < state.candidates.len() { + Ok(state.candidates.get(index).unwrap()) + } else { + Err(URError::CandidateDoesntExist) + } + } + + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.already_voted.set(caller, true); + let votes = state + .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? + .checked_add(1) + .ok_or(URError::Overflow)?; + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&STATE).unwrap() + } +} diff --git a/test-cases/dynamic-instance-storage/Cargo.toml b/test-cases/dynamic-instance-storage/Cargo.toml new file mode 100644 index 00000000..2af55099 --- /dev/null +++ b/test-cases/dynamic-instance-storage/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["dynamic-instance-storage-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=21.4.0" } + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..6da39cf1 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-remediated-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..f6be86d0 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs @@ -0,0 +1,43 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Symbol, Vec}; + +#[contract] +pub struct VectorStorage; + +#[contractimpl] +impl VectorStorage { + pub fn store_vector(e: Env, data: Vec) { + e.storage() + .persistent() + .set(&Symbol::new(&e, "vector_data"), &data); + } + + pub fn get_vector(e: Env) -> Vec { + e.storage() + .persistent() + .get(&Symbol::new(&e, "vector_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{vec, Env}; + + #[test] + fn test_vector_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, VectorStorage); + let client = VectorStorageClient::new(&env, &contract_id); + + // When + let test_vec = vec![&env, 1, 2, 3, 4, 5]; + client.store_vector(&test_vec); + + // Then + let retrieved_vec = client.get_vector(); + assert_eq!(test_vec, retrieved_vec); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..7122885e --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-vulnerable-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..10f79de7 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs @@ -0,0 +1,43 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Symbol, Vec}; + +#[contract] +pub struct VectorStorage; + +#[contractimpl] +impl VectorStorage { + pub fn store_vector(e: Env, data: Vec) { + e.storage() + .instance() + .set(&Symbol::new(&e, "vector_data"), &data); + } + + pub fn get_vector(e: Env) -> Vec { + e.storage() + .instance() + .get(&Symbol::new(&e, "vector_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{vec, Env}; + + #[test] + fn test_vector_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, VectorStorage); + let client = VectorStorageClient::new(&env, &contract_id); + + // When + let test_vec = vec![&env, 1, 2, 3, 4, 5]; + client.store_vector(&test_vec); + + // Then + let retrieved_vec = client.get_vector(); + assert_eq!(test_vec, retrieved_vec); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..7d90d213 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-remediated-2" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..354b0abc --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs @@ -0,0 +1,43 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, String, Symbol}; + +#[contract] +pub struct StringStorage; + +#[contractimpl] +impl StringStorage { + pub fn store_string(e: Env, data: String) { + e.storage() + .persistent() + .set(&Symbol::new(&e, "string_data"), &data); + } + + pub fn get_string(e: Env) -> String { + e.storage() + .persistent() + .get(&Symbol::new(&e, "string_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_string_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, StringStorage); + let client = StringStorageClient::new(&env, &contract_id); + + // When + let test_string = String::from_str(&env, "Hello, Soroban!"); + client.store_string(&test_string); + + // Then + let retrieved_string = client.get_string(); + assert_eq!(test_string, retrieved_string); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..9241c0f4 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-vulnerable-2" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..79a36aa0 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs @@ -0,0 +1,43 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, String, Symbol}; + +#[contract] +pub struct StringStorage; + +#[contractimpl] +impl StringStorage { + pub fn store_string(e: Env, data: String) { + e.storage() + .instance() + .set(&Symbol::new(&e, "string_data"), &data); + } + + pub fn get_string(e: Env) -> String { + e.storage() + .instance() + .get(&Symbol::new(&e, "string_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_string_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, StringStorage); + let client = StringStorageClient::new(&env, &contract_id); + + // When + let test_string = String::from_str(&env, "Hello, Soroban!"); + client.store_string(&test_string); + + // Then + let retrieved_string = client.get_string(); + assert_eq!(test_string, retrieved_string); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml new file mode 100644 index 00000000..fa1d3059 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-remediated-3" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs new file mode 100644 index 00000000..f81a77f2 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs @@ -0,0 +1,52 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + e.storage() + .persistent() + .set(&Symbol::new(&e, "map_data"), &data); + } + + pub fn get_map(e: Env) -> Map { + e.storage() + .persistent() + .get(&Symbol::new(&e, "map_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let retrieved_map = client.get_map(); + assert_eq!( + test_map.get(symbol_short!("key1")), + retrieved_map.get(symbol_short!("key1")) + ); + assert_eq!( + test_map.get(symbol_short!("key2")), + retrieved_map.get(symbol_short!("key2")) + ); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..5f0d3c3d --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-instance-storage-vulnerable-3" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..20a33671 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs @@ -0,0 +1,52 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + e.storage() + .instance() + .set(&Symbol::new(&e, "map_data"), &data); + } + + pub fn get_map(e: Env) -> Map { + e.storage() + .instance() + .get(&Symbol::new(&e, "map_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let retrieved_map = client.get_map(); + assert_eq!( + test_map.get(symbol_short!("key1")), + retrieved_map.get(symbol_short!("key1")) + ); + assert_eq!( + test_map.get(symbol_short!("key2")), + retrieved_map.get(symbol_short!("key2")) + ); + } +} diff --git a/test-cases/incorrect-exponentiation/Cargo.toml b/test-cases/incorrect-exponentiation/Cargo.toml index fc211bbd..4e28b497 100644 --- a/test-cases/incorrect-exponentiation/Cargo.toml +++ b/test-cases/incorrect-exponentiation/Cargo.toml @@ -4,7 +4,7 @@ members = ["incorrect-exponentiation-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "20.3.2" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/insufficiently-random-values/Cargo.toml b/test-cases/insufficiently-random-values/Cargo.toml index a5301348..9433190f 100644 --- a/test-cases/insufficiently-random-values/Cargo.toml +++ b/test-cases/insufficiently-random-values/Cargo.toml @@ -4,7 +4,7 @@ members = ["insufficiently-random-values-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/integer-overflow-or-underflow/Cargo.toml b/test-cases/integer-overflow-or-underflow/Cargo.toml index ca2a186c..aa613703 100644 --- a/test-cases/integer-overflow-or-underflow/Cargo.toml +++ b/test-cases/integer-overflow-or-underflow/Cargo.toml @@ -4,7 +4,7 @@ members = ["integer-overflow-or-underflow-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=21.3.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/iterators-over-indexing/Cargo.toml b/test-cases/iterators-over-indexing/Cargo.toml index a2ed9201..954db6ee 100644 --- a/test-cases/iterators-over-indexing/Cargo.toml +++ b/test-cases/iterators-over-indexing/Cargo.toml @@ -4,7 +4,7 @@ members = ["iterators-over-indexing-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/overflow-check/overflow-check-1/remediated-example/Cargo.toml b/test-cases/overflow-check/overflow-check-1/remediated-example/Cargo.toml index a7e7c02d..5cd50d42 100644 --- a/test-cases/overflow-check/overflow-check-1/remediated-example/Cargo.toml +++ b/test-cases/overflow-check/overflow-check-1/remediated-example/Cargo.toml @@ -4,14 +4,11 @@ edition = "2021" name = "overflow-check-remediated-1" version = "0.1.0" -[lib] -crate-type = ["cdylib"] - [dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { version = "=21.4.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/overflow-check/overflow-check-1/vulnerable-example/Cargo.toml b/test-cases/overflow-check/overflow-check-1/vulnerable-example/Cargo.toml index b38e7396..ce7c42f4 100644 --- a/test-cases/overflow-check/overflow-check-1/vulnerable-example/Cargo.toml +++ b/test-cases/overflow-check/overflow-check-1/vulnerable-example/Cargo.toml @@ -4,14 +4,11 @@ edition = "2021" name = "overflow-check-vulnerable-1" version = "0.1.0" -[lib] -crate-type = ["cdylib"] - [dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { version = "=21.4.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/set-contract-storage/Cargo.toml b/test-cases/set-contract-storage/Cargo.toml index 4ed17155..9b92dc23 100644 --- a/test-cases/set-contract-storage/Cargo.toml +++ b/test-cases/set-contract-storage/Cargo.toml @@ -4,7 +4,7 @@ members = ["set-contract-storage-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/soroban-version/soroban-version-1/remediated-example/Cargo.toml b/test-cases/soroban-version/soroban-version-1/remediated-example/Cargo.toml index 57d38b52..1b1850f3 100644 --- a/test-cases/soroban-version/soroban-version-1/remediated-example/Cargo.toml +++ b/test-cases/soroban-version/soroban-version-1/remediated-example/Cargo.toml @@ -7,10 +7,24 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = { workspace = true } +soroban-sdk = { version = "=21.4.0" } [dev-dependencies] -soroban-sdk = { workspace = true, features = ["testutils"] } +soroban-sdk = { version = "=21.4.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = false +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/soroban-version/soroban-version-1/vulnerable-example/Cargo.toml b/test-cases/soroban-version/soroban-version-1/vulnerable-example/Cargo.toml index 4e7b3fb9..c91f53e8 100644 --- a/test-cases/soroban-version/soroban-version-1/vulnerable-example/Cargo.toml +++ b/test-cases/soroban-version/soroban-version-1/vulnerable-example/Cargo.toml @@ -7,10 +7,24 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = { workspace = true } +soroban-sdk = { version = "=20.0.0" } [dev-dependencies] -soroban-sdk = { workspace = true, features = ["testutils"] } +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = false +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/unprotected-mapping-operation/Cargo.toml b/test-cases/unprotected-mapping-operation/Cargo.toml index 46ee0cfb..42594604 100644 --- a/test-cases/unprotected-mapping-operation/Cargo.toml +++ b/test-cases/unprotected-mapping-operation/Cargo.toml @@ -4,7 +4,7 @@ members = ["unprotected-mapping-operation-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/unprotected-update-current-contract-wasm/Cargo.toml b/test-cases/unprotected-update-current-contract-wasm/Cargo.toml index 399bb063..e6c34a02 100644 --- a/test-cases/unprotected-update-current-contract-wasm/Cargo.toml +++ b/test-cases/unprotected-update-current-contract-wasm/Cargo.toml @@ -4,7 +4,7 @@ members = ["unprotected-update-current-contract-wasm-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/unrestricted-transfer-from/Cargo.toml b/test-cases/unrestricted-transfer-from/Cargo.toml index 4e7e2772..54f81f26 100644 --- a/test-cases/unrestricted-transfer-from/Cargo.toml +++ b/test-cases/unrestricted-transfer-from/Cargo.toml @@ -4,7 +4,7 @@ members = ["unrestricted-transfer-from-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 @@ -19,4 +19,3 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" - diff --git a/test-cases/unsafe-expect/Cargo.toml b/test-cases/unsafe-expect/Cargo.toml index a163f8ba..d220ab60 100644 --- a/test-cases/unsafe-expect/Cargo.toml +++ b/test-cases/unsafe-expect/Cargo.toml @@ -4,7 +4,7 @@ members = ["unsafe-expect-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/unsafe-map-get/Cargo.toml b/test-cases/unsafe-map-get/Cargo.toml index efc07045..3a88faaa 100644 --- a/test-cases/unsafe-map-get/Cargo.toml +++ b/test-cases/unsafe-map-get/Cargo.toml @@ -4,7 +4,7 @@ members = ["unsafe-map-get-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/unsafe-unwrap/Cargo.toml b/test-cases/unsafe-unwrap/Cargo.toml index d7b45aae..6c67b435 100644 --- a/test-cases/unsafe-unwrap/Cargo.toml +++ b/test-cases/unsafe-unwrap/Cargo.toml @@ -4,7 +4,7 @@ members = ["unsafe-unwrap-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/unused-return-enum/Cargo.toml b/test-cases/unused-return-enum/Cargo.toml index f5202497..08340fe4 100644 --- a/test-cases/unused-return-enum/Cargo.toml +++ b/test-cases/unused-return-enum/Cargo.toml @@ -4,7 +4,7 @@ members = ["unused-return-enum-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/vec-could-be-mapping/Cargo.toml b/test-cases/vec-could-be-mapping/Cargo.toml new file mode 100644 index 00000000..c105dde9 --- /dev/null +++ b/test-cases/vec-could-be-mapping/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["vec-could-be-mapping-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=21.4.0" } + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml index 9b4e6811..1af4e7bb 100644 --- a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml @@ -7,24 +7,10 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0-rc2" +soroban-sdk = { workspace = true } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { workspace = true, features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] - -[profile.release] -codegen-units = 1 -debug = 0 -debug-assertions = false -lto = true -opt-level = "z" -overflow-checks = true -panic = "abort" -strip = "symbols" - -[profile.release-with-logs] -debug-assertions = true -inherits = "release" diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain deleted file mode 100644 index 8811e92c..00000000 --- a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain +++ /dev/null @@ -1,3 +0,0 @@ -[toolchain] -channel = "nightly-2023-09-29" -components = ["llvm-tools-preview", "rustc-dev"] diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml index 860c0f59..4ba1dbdc 100644 --- a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml @@ -7,24 +7,10 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0-rc2" +soroban-sdk = { workspace = true } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { workspace = true, features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] - -[profile.release] -codegen-units = 1 -debug = 0 -debug-assertions = false -lto = true -opt-level = "z" -overflow-checks = true -panic = "abort" -strip = "symbols" - -[profile.release-with-logs] -debug-assertions = true -inherits = "release" diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain deleted file mode 100644 index 8811e92c..00000000 --- a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain +++ /dev/null @@ -1,3 +0,0 @@ -[toolchain] -channel = "nightly-2023-09-29" -components = ["llvm-tools-preview", "rustc-dev"] diff --git a/test-cases/soroban-version/Cargo.toml b/test-cases/zero-address/Cargo.toml similarity index 81% rename from test-cases/soroban-version/Cargo.toml rename to test-cases/zero-address/Cargo.toml index de5686c5..cedd9259 100644 --- a/test-cases/soroban-version/Cargo.toml +++ b/test-cases/zero-address/Cargo.toml @@ -1,10 +1,10 @@ [workspace] exclude = [".cargo", "target"] -members = ["soroban-version-*/*"] +members = ["zero-address-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=20.0.0" } +soroban-sdk = { version = "=21.4.0" } [profile.release] codegen-units = 1 diff --git a/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml b/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml index fc542d0e..3a1c35b8 100644 --- a/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml +++ b/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml @@ -7,24 +7,10 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0-rc2" +soroban-sdk = { workspace = true } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { workspace = true, features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] - -[profile.release] -codegen-units = 1 -debug = 0 -debug-assertions = false -lto = true -opt-level = "z" -overflow-checks = true -panic = "abort" -strip = "symbols" - -[profile.release-with-logs] -debug-assertions = true -inherits = "release" diff --git a/test-cases/zero-address/zero-address-1/remediated-example/rust-toolchain b/test-cases/zero-address/zero-address-1/remediated-example/rust-toolchain deleted file mode 100644 index 8811e92c..00000000 --- a/test-cases/zero-address/zero-address-1/remediated-example/rust-toolchain +++ /dev/null @@ -1,3 +0,0 @@ -[toolchain] -channel = "nightly-2023-09-29" -components = ["llvm-tools-preview", "rustc-dev"] diff --git a/test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml b/test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml index 195c8b6e..15be1e68 100644 --- a/test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml +++ b/test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml @@ -7,24 +7,10 @@ version = "0.1.0" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0-rc2" +soroban-sdk = { workspace = true } [dev-dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } +soroban-sdk = { workspace = true, features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] - -[profile.release] -codegen-units = 1 -debug = 0 -debug-assertions = false -lto = true -opt-level = "z" -overflow-checks = true -panic = "abort" -strip = "symbols" - -[profile.release-with-logs] -debug-assertions = true -inherits = "release" diff --git a/test-cases/zero-address/zero-address-1/vulnerable-example/rust-toolchain b/test-cases/zero-address/zero-address-1/vulnerable-example/rust-toolchain deleted file mode 100644 index 8811e92c..00000000 --- a/test-cases/zero-address/zero-address-1/vulnerable-example/rust-toolchain +++ /dev/null @@ -1,3 +0,0 @@ -[toolchain] -channel = "nightly-2023-09-29" -components = ["llvm-tools-preview", "rustc-dev"] diff --git a/utils/src/lib.rs b/utils/src/lib.rs index 22c331d8..d119cbad 100644 --- a/utils/src/lib.rs +++ b/utils/src/lib.rs @@ -11,3 +11,6 @@ pub use lint_utils::*; mod constant_analyzer; pub use constant_analyzer::*; + +mod type_utils; +pub use type_utils::*; diff --git a/utils/src/soroban_utils/mod.rs b/utils/src/soroban_utils/mod.rs index 5816d63e..6b97c8e6 100644 --- a/utils/src/soroban_utils/mod.rs +++ b/utils/src/soroban_utils/mod.rs @@ -12,6 +12,9 @@ use rustc_span::def_id::DefId; const SOROBAN_ENV: &str = "soroban_sdk::Env"; const SOROBAN_ADDRESS: &str = "soroban_sdk::Address"; const SOROBAN_MAP: &str = "soroban_sdk::Map"; +const SOROBAN_INSTANCE_STORAGE: &str = "soroban_sdk::storage::Instance"; +const SOROBAN_TEMPORARY_STORAGE: &str = "soroban_sdk::storage::Temporary"; +const SOROBAN_PERSISTENT_STORAGE: &str = "soroban_sdk::storage::Persistent"; /// Determines whether a function defined by its `DefId` is part of a Soroban contract implementation. /// @@ -78,3 +81,30 @@ pub fn is_soroban_address(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { pub fn is_soroban_map(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { is_soroban_type(cx, expr_type, SOROBAN_MAP) } + +pub enum SorobanStorageType { + Any, + Instance, + Temporary, + Persistent, +} + +/// Checks if the provided type is a Soroban storage type (Instance, Temporary, or Persistent). +pub fn is_soroban_storage( + cx: &LateContext<'_>, + expr_type: Ty<'_>, + storage_type: SorobanStorageType, +) -> bool { + match storage_type { + SorobanStorageType::Any => { + is_soroban_type(cx, expr_type, SOROBAN_INSTANCE_STORAGE) + || is_soroban_type(cx, expr_type, SOROBAN_TEMPORARY_STORAGE) + || is_soroban_type(cx, expr_type, SOROBAN_PERSISTENT_STORAGE) + } + SorobanStorageType::Instance => is_soroban_type(cx, expr_type, SOROBAN_INSTANCE_STORAGE), + SorobanStorageType::Temporary => is_soroban_type(cx, expr_type, SOROBAN_TEMPORARY_STORAGE), + SorobanStorageType::Persistent => { + is_soroban_type(cx, expr_type, SOROBAN_PERSISTENT_STORAGE) + } + } +} diff --git a/utils/src/type_utils.rs b/utils/src/type_utils.rs new file mode 100644 index 00000000..942003b6 --- /dev/null +++ b/utils/src/type_utils.rs @@ -0,0 +1,12 @@ +extern crate rustc_hir; +extern crate rustc_lint; +extern crate rustc_middle; + +use rustc_hir::HirId; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; + +/// Get the type of a node, if it exists. +pub fn get_node_type_opt<'tcx>(cx: &LateContext<'tcx>, hir_id: &HirId) -> Option> { + cx.typeck_results().node_type_opt(*hir_id) +}