diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index 98dd3f85..ad876ad6 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -101,6 +101,7 @@ jobs: "divide-before-multiply", "insufficiently-random-values", "overflow-check", + "set-contract-storage", "unprotected-update-current-contract-wasm", "unsafe-expect", "unsafe-unwrap", diff --git a/detectors/set-contract-storage/Cargo.toml b/detectors/set-contract-storage/Cargo.toml new file mode 100644 index 00000000..923e9e64 --- /dev/null +++ b/detectors/set-contract-storage/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "set-contract-storage" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +scout-audit-internal = { workspace = true } + +[dev-dependencies] +dylint_testing = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/set-contract-storage/src/lib.rs b/detectors/set-contract-storage/src/lib.rs new file mode 100644 index 00000000..14606d8b --- /dev/null +++ b/detectors/set-contract-storage/src/lib.rs @@ -0,0 +1,89 @@ +#![feature(rustc_private)] +#![warn(unused_extern_crates)] + +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_hir::def_id::LocalDefId; +use rustc_hir::intravisit::Visitor; +use rustc_hir::intravisit::{walk_expr, FnKind}; +use rustc_hir::{Body, FnDecl}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::Span; +use scout_audit_internal::Detector; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// Checks for calls to env.storage() without a prior call to env.require_auth() + /// + /// ### Why is this bad? + /// Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations. + /// + /// ### Known problems + /// Only check the function call, so false positives could result. + /// + /// ### Example + /// ```rust + /// fn set_contract_storage(env: Env) { + /// let _storage = env.storage().instance(); + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn set_contract_storage(env: Env, user: Address) { + /// user.require_auth(); + /// let _storage = env.storage().instance(); + /// } + /// ``` + pub SET_STORAGE_WARN, + Warn, + Detector::SetContractStorage.get_lint_message() +} + +impl<'tcx> LateLintPass<'tcx> for SetStorageWarn { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'_>, + body: &'tcx Body<'_>, + _: Span, + _: LocalDefId, + ) { + struct SetContractStorageVisitor { + found_auth: bool, + storage_without_auth: Vec, + } + + impl<'tcx> Visitor<'tcx> for SetContractStorageVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(path, _, _, _) = &expr.kind { + let method_name = path.ident.name.as_str(); + if method_name == "require_auth" { + self.found_auth = true; + } else if method_name == "storage" && !self.found_auth { + self.storage_without_auth.push(expr.span); + } + } + walk_expr(self, expr); + } + } + + let mut visitor = SetContractStorageVisitor { + found_auth: false, + storage_without_auth: Vec::new(), + }; + + walk_expr(&mut visitor, body.value); + + for span in visitor.storage_without_auth { + Detector::SetContractStorage.span_lint_and_help( + cx, + SET_STORAGE_WARN, + span, + "Ensure that the caller is authorized to use storage", + ); + } + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index 5e523e1c..81908728 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -30,6 +30,7 @@ pub enum Detector { DivideBeforeMultiply, InsufficientlyRandomValues, OverflowCheck, + SetContractStorage, UnprotectedUpdateCurrentContractWasm, UnsafeExpect, UnsafeUnwrap, @@ -43,6 +44,7 @@ impl Detector { Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, + Detector::SetContractStorage => SET_CONTRACT_STORAGE_LINT_MESSAGE, Detector::UnprotectedUpdateCurrentContractWasm => { UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index bac80143..4663ebaf 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -4,6 +4,7 @@ pub const DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str = "Division before multiplication might result in a loss of precision"; pub const INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE: &str = "Use env.prng() to generate random numbers, and remember that all random numbers are under the control of validators"; pub const OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile"; +pub const SET_CONTRACT_STORAGE_LINT_MESSAGE:&str = "Abitrary users should not have control over keys because it implies writing any value of left mapping, lazy variable, or the main struct of the contract located in position 0 of the storage"; pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE: &str = "This update_current_contract_wasm is called without access control"; pub const UNSAFE_EXPECT_LINT_MESSAGE: &str = "Unsafe usage of `expect`"; diff --git a/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..ef5fac58 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..fee28b2c --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/src/lib.rs @@ -0,0 +1,48 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[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(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::{testutils, Address, Env}; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + env.mock_all_auths(); + let user =
::random(&env); + + // When + let first_increment = client.increment(&user); + let second_increment = client.increment(&user); + let third_increment = client.increment(&user); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +} diff --git a/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..81ed2a8e --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..84c06259 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/src/lib.rs @@ -0,0 +1,46 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[contractimpl] +impl SetContractStorage { + /// Increment an internal counter; return the new value. + pub fn increment(env: Env) -> u32 { + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + + // When + let first_increment = client.increment(); + let second_increment = client.increment(); + let third_increment = client.increment(); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +} diff --git a/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..ef5fac58 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..60be6993 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-2/remediated-example/src/lib.rs @@ -0,0 +1,48 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[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().temporary(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(&COUNTER, 100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::{testutils, Address, Env}; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + env.mock_all_auths(); + let user =
::random(&env); + + // When + let first_increment = client.increment(&user); + let second_increment = client.increment(&user); + let third_increment = client.increment(&user); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +} diff --git a/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..81ed2a8e --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..d1fdf98a --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-2/vulnerable-example/src/lib.rs @@ -0,0 +1,46 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[contractimpl] +impl SetContractStorage { + /// Increment an internal counter; return the new value. + pub fn increment(env: Env) -> u32 { + let storage = env.storage().temporary(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(&COUNTER, 100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + + // When + let first_increment = client.increment(); + let second_increment = client.increment(); + let third_increment = client.increment(); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +} diff --git a/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/Cargo.toml new file mode 100644 index 00000000..ef5fac58 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/src/lib.rs new file mode 100644 index 00000000..40dc7ded --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-3/remediated-example/src/lib.rs @@ -0,0 +1,48 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[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().persistent(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(&COUNTER, 100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::{testutils, Address, Env}; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + env.mock_all_auths(); + let user =
::random(&env); + + // When + let first_increment = client.increment(&user); + let second_increment = client.increment(&user); + let third_increment = client.increment(&user); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +} diff --git a/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/Cargo.toml b/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..81ed2a8e --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "set-contract-storage-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/src/lib.rs b/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..dc517241 --- /dev/null +++ b/test-cases/set-contract-storage/set-contract-storage-3/vulnerable-example/src/lib.rs @@ -0,0 +1,46 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct SetContractStorage; + +#[contractimpl] +impl SetContractStorage { + /// Increment an internal counter; return the new value. + pub fn increment(env: Env) -> u32 { + let storage = env.storage().persistent(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + count += 1; + storage.set(&COUNTER, &count); + storage.bump(&COUNTER, 100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{SetContractStorage, SetContractStorageClient}; + + #[test] + fn increment() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, SetContractStorage); + let client = SetContractStorageClient::new(&env, &contract_id); + + // When + let first_increment = client.increment(); + let second_increment = client.increment(); + let third_increment = client.increment(); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 2); + assert_eq!(third_increment, 3); + } +}