diff --git a/README.md b/README.md index aa24d417..57b1d87e 100644 --- a/README.md +++ b/README.md @@ -61,10 +61,10 @@ Currently Scout includes the following detectors. | [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 | | [incorrect-exponentation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/incorrect-exponentiation) | Warns against incorrect usage of ´^´. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1) | Critical | | [token-interface-events](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/token-interface-events) | Warns if any of the token functions does not emit an event. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/token-interface-events/token-interface-events-1) | Medium | + ## Output formats You can choose the output format that best suit your needs. Scout offers html, markdown, json, pdf and sarif reports. To specify the desired output run the following command: diff --git a/detectors/zero-address/Cargo.toml b/detectors/zero-address/Cargo.toml deleted file mode 100644 index 21980847..00000000 --- a/detectors/zero-address/Cargo.toml +++ /dev/null @@ -1,17 +0,0 @@ -[package] -edition = "2021" -name = "zero-address" -version = "0.1.0" - -[lib] -crate-type = ["cdylib"] - -[dependencies] -clippy_utils = { workspace = true } -clippy_wrappers = { workspace = true } -dylint_linting = { workspace = true } -if_chain = { workspace = true } -utils = { workspace = true } - -[package.metadata.rust-analyzer] -rustc_private = true diff --git a/detectors/zero-address/src/lib.rs b/detectors/zero-address/src/lib.rs deleted file mode 100644 index 1bf915c8..00000000 --- a/detectors/zero-address/src/lib.rs +++ /dev/null @@ -1,255 +0,0 @@ -#![feature(rustc_private)] -#![feature(let_chains)] -extern crate rustc_ast; -extern crate rustc_hir; -extern crate rustc_middle; -extern crate rustc_span; -extern crate rustc_type_ir; - -use std::collections::HashSet; - -use clippy_wrappers::span_lint_and_help; -use if_chain::if_chain; -use rustc_ast::LitKind; -use rustc_hir::def_id::LocalDefId; -use rustc_hir::{ - def::Res, - intravisit::{walk_expr, FnKind, Visitor}, - BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Param, PatKind, QPath, -}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::middle::privacy::Level; -use rustc_span::Span; -use utils::{ - expr_to_address_of, expr_to_call, expr_to_lit, expr_to_path, get_node_type, path_to_resolved, - path_to_type_relative, resolution_to_local, type_to_path, -}; - -const LINT_MESSAGE: &str = "Not checking for a zero-address could lead to an insecure contract"; - -dylint_linting::declare_late_lint! { - pub ZERO_ADDRESS, - Warn, - LINT_MESSAGE, - { - name: "Zero Address", - long_message: "In the elliptic curve used by Soroban (Ed25519), the zero address has a known private key. Using this address as a null value (for example, for a contract's administrative account) is a common mistake, and can lead to losing control of the contract, instead of the contract being locked.", - severity: "Minor", - help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/zero-or-test-address", - vulnerability_class: "Validations and error handling", - } -} - -fn match_expr_as_function_call<'hir>( - expr: &'hir Expr<'hir>, - type_name: &str, - function_name: &str, -) -> Result<&'hir [Expr<'hir>], ()> { - let (expr_fn, exprs_args) = expr_to_call(&expr.kind)?; - let qpath = expr_to_path(&expr_fn.kind)?; - let (ty, path) = path_to_type_relative(&qpath)?; - let qpath2 = type_to_path(&ty.kind)?; - let (_, path2) = path_to_resolved(qpath2)?; - let type_id = path2 - .segments - .iter() - .map(|x| x.ident.to_string()) - .collect::>() - .join("::"); - //TODO: Need a better method to determine the type. - if type_id != type_name { - return Err(()); - } - - if path.ident.to_string() != function_name { - return Err(()); - } - - Ok(exprs_args) -} - -fn remove_address_of<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { - match expr_to_address_of(&expr.kind) { - Ok((_, _, sub)) => sub, - Err(_) => expr, - } -} - -fn expr_is_zero_addr<'hir>(expr: &'hir Expr<'hir>, cx: &rustc_lint::LateContext) -> bool { - let r = || -> Result { - let args = || -> Result<&'hir [Expr<'hir>], ()> { - let r = match_expr_as_function_call(expr, "soroban_sdk::Address", "from_string"); - if r.is_ok() { - return r; - } - match_expr_as_function_call(expr, "Address", "from_string") - }()?; - if args.len() != 1 { - return Ok(false); - } - let expr = remove_address_of(args.first().unwrap()); - let args = || -> Result<&'hir [Expr<'hir>], ()> { - let r = match_expr_as_function_call(expr, "soroban_sdk::String", "from_bytes"); - if r.is_ok() { - return r; - } - match_expr_as_function_call(expr, "String", "from_bytes") - }()?; - - if args.len() != 2 { - return Ok(false); - } - let env_arg = args.first().unwrap(); - let addr_arg = args.get(1).unwrap(); - - //Check that the first argument is either of type soroban_sdk::Env or of type &soroban_sdk::Env. - let env_arg = remove_address_of(env_arg); - let env_path = expr_to_path(&env_arg.kind)?; - let (_, env_resolved) = path_to_resolved(&env_path)?; - let env_id = resolution_to_local(&env_resolved.res)?; - let env_type = get_node_type(cx, env_id); - if env_type.to_string() != "soroban_sdk::Env" { - return Ok(false); - } - - let addr_arg = expr_to_lit(&addr_arg.kind)?; - - if let LitKind::ByteStr(data, _) = &addr_arg.node as &LitKind { - if data.len() != 56 { - return Ok(false); - } - //'G' - if *data.first().unwrap() != 71_u8 { - return Ok(false); - } - //52 times 'A' - for i in 1..53 { - if *data.get(i).unwrap() != 65_u8 { - return Ok(false); - } - } - - //'W' - if *data.get(53).unwrap() != 87_u8 { - return Ok(false); - } - - //'H' - if *data.get(54).unwrap() != 72_u8 { - return Ok(false); - } - - //'F' - Ok(*data.get(55).unwrap() == 70_u8) - } else { - Ok(false) - } - }(); - r.unwrap_or(false) -} - -fn get_param_hir_id(param: &Param) -> Option { - if let PatKind::Binding(_, b, _, _) = param.pat.kind { - Some(b) - } else { - None - } -} - -fn get_path_local_hir_id(expr: &Expr<'_>) -> Option { - if let ExprKind::Path(qpath) = &expr.kind - && let QPath::Resolved(_, path) = qpath - && let Res::Local(local) = path.res - { - Some(local) - } else { - None - } -} - -impl<'tcx> LateLintPass<'tcx> for ZeroAddress { - fn check_fn( - &mut self, - cx: &LateContext<'tcx>, - _: FnKind<'tcx>, - _: &'tcx FnDecl<'_>, - body: &'tcx Body<'_>, - _: Span, - id: LocalDefId, - ) { - if !cx - .effective_visibilities - .is_public_at_level(id, Level::Reexported) - { - return; - } - - struct ZeroCheckStorage<'tcx, 'tcx_ref> { - cx: &'tcx_ref LateContext<'tcx>, - acc_id_params: Vec<&'tcx Param<'tcx>>, - checked_params: HashSet<&'tcx HirId>, - } - - impl<'tcx> Visitor<'tcx> for ZeroCheckStorage<'tcx, '_> { - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - //Look if those params are compared with zero address - if let ExprKind::If(mut cond, _, _) = &expr.kind { - if let ExprKind::DropTemps(drop) = cond.kind { - cond = drop; - } - if_chain! { - if let ExprKind::Binary(op, lexpr, rexpr) = cond.kind; - if BinOpKind::Eq == op.node; - then { - for param in &self.acc_id_params { - let param_hir_id = get_param_hir_id(param); - if (param_hir_id == get_path_local_hir_id(lexpr) - && expr_is_zero_addr(rexpr, self.cx)) || - (param_hir_id == get_path_local_hir_id(rexpr) - && expr_is_zero_addr(lexpr, self.cx)) { - self.checked_params.insert(¶m.hir_id); - } - } - } - } - } - walk_expr(self, expr); - } - } - - let mut zerocheck_storage = ZeroCheckStorage { - cx, - acc_id_params: Vec::default(), - checked_params: HashSet::default(), - }; - - let mir_body = cx.tcx.optimized_mir(id); - for (arg, hir_param) in mir_body.args_iter().zip(body.params.iter()) { - let arg = &mir_body.local_decls[arg]; - if arg.ty.to_string() == "soroban_sdk::Address" { - zerocheck_storage.acc_id_params.push(hir_param); - } - } - - // If no arguments of accountId type is found, ignore this function - if zerocheck_storage.acc_id_params.is_empty() { - return; - } - - walk_expr(&mut zerocheck_storage, body.value); - - for param in zerocheck_storage.acc_id_params { - if zerocheck_storage.checked_params.contains(¶m.hir_id) { - continue; - } - span_lint_and_help( - cx, - ZERO_ADDRESS, - param.span, - LINT_MESSAGE, - None, - "This function should check if the AccountId passed is zero and revert if it is", - ); - } - } -} diff --git a/docs/docs/detectors/21-incorrect-exponentiation.md b/docs/docs/detectors/20-incorrect-exponentiation.md similarity index 100% rename from docs/docs/detectors/21-incorrect-exponentiation.md rename to docs/docs/detectors/20-incorrect-exponentiation.md diff --git a/docs/docs/detectors/20-zero-or-test-address.md b/docs/docs/detectors/20-zero-or-test-address.md deleted file mode 100644 index b3b715bf..00000000 --- a/docs/docs/detectors/20-zero-or-test-address.md +++ /dev/null @@ -1,43 +0,0 @@ -# Zero or test address - -### What it does -Checks whether the zero address is being inputed to a function without validation. - -### Why is this bad? -Because the private key for the zero address is known, anyone could take ownership of the contract. - -### Example - -```rust -pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - e.storage().persistent().set(&DataKey::Data, &data); - Ok(()) -} -``` - - -Use instead: -```rust -pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { - if admin - == Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", - )) - { - return Err(Error::InvalidNewAdmin); - } - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - e.storage().persistent().set(&DataKey::Data, &data); - Ok(()) -} -``` - -### Implementation - -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/zero-or-test-address). diff --git a/docs/docs/detectors/22-integer-overflow -or-underflow.md b/docs/docs/detectors/21-integer-overflow -or-underflow.md similarity index 100% rename from docs/docs/detectors/22-integer-overflow -or-underflow.md rename to docs/docs/detectors/21-integer-overflow -or-underflow.md diff --git a/test-cases/zero-address/Cargo.toml b/test-cases/zero-address/Cargo.toml deleted file mode 100644 index cedd9259..00000000 --- a/test-cases/zero-address/Cargo.toml +++ /dev/null @@ -1,21 +0,0 @@ -[workspace] -exclude = [".cargo", "target"] -members = ["zero-address-*/*"] -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/zero-address/zero-address-1/remediated-example/Cargo.toml b/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml deleted file mode 100644 index 3a1c35b8..00000000 --- a/test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -edition = "2021" -name = "zero-address-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/zero-address/zero-address-1/remediated-example/src/lib.rs b/test-cases/zero-address/zero-address-1/remediated-example/src/lib.rs deleted file mode 100644 index 7867e1e7..00000000 --- a/test-cases/zero-address/zero-address-1/remediated-example/src/lib.rs +++ /dev/null @@ -1,134 +0,0 @@ -#![no_std] - -#[cfg(any(test, feature = "testutils"))] -extern crate std; - -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; -#[contracttype] -#[derive(Clone)] -enum DataKey { - Admin, - Data, - ReadOnly, -} - -#[contract] -pub struct ZeroAddressContract; - -#[contracterror] -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -#[repr(u32)] -pub enum Error { - Ununitialized = 1, - NotAdmin = 2, - NoData = 3, - InvalidNewAdmin = 4, -} - -#[contractimpl] -impl ZeroAddressContract { - pub fn init(e: Env, admin: Address) -> Result<(), Error> { - if admin - == soroban_sdk::Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", - )) - { - return Err(Error::InvalidNewAdmin); - } - e.storage().persistent().set(&DataKey::Admin, &admin); - Ok(()) - } - - fn ensure_is_admin(e: &Env, admin: Address) -> Result { - let registered_admin = e - .storage() - .persistent() - .get::(&DataKey::Admin) - .ok_or(Error::Ununitialized)?; - if admin != registered_admin { - return Ok(false); - } - admin.require_auth(); - Ok(true) - } - - pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { - if admin - == Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", - )) - { - return Err(Error::InvalidNewAdmin); - } - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - e.storage().persistent().set(&DataKey::Data, &data); - Ok(()) - } - - pub fn get(e: Env) -> Result { - e.storage() - .persistent() - .get::(&DataKey::Data) - .ok_or(Error::NoData) - } - - pub fn change_admin(e: Env, admin: Address, new_admin: Address) -> Result<(), Error> { - if admin - == Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", - )) - { - return Err(Error::InvalidNewAdmin); - } - - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - - if new_admin - == Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", - )) - { - return Err(Error::InvalidNewAdmin); - } - - e.storage().persistent().set(&DataKey::Admin, &new_admin); - - Ok(()) - } -} - -#[test] -fn simple_test() { - use soroban_sdk::testutils::Address as _; - - let e = Env::default(); - e.mock_all_auths(); - let client = - ZeroAddressContractClient::new(&e, &e.register_contract(None, ZeroAddressContract {})); - let admin = Address::generate(&e); - client.init(&admin); - assert_eq!(client.try_get(), Err(Ok(Error::NoData))); - client.set(&admin, &5); - assert_eq!(client.get(), 5); - assert_eq!( - client.try_change_admin( - &admin, - &Address::from_string(&String::from_bytes( - &e, - b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF" - )) - ), - Err(Ok(Error::InvalidNewAdmin)) - ); - client.change_admin(&admin, &Address::generate(&e)); - assert_eq!(client.get(), 5); - assert_eq!(client.try_set(&admin, &6), Err(Ok(Error::NotAdmin))); -} 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 deleted file mode 100644 index 15be1e68..00000000 --- a/test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -edition = "2021" -name = "zero-address-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/zero-address/zero-address-1/vulnerable-example/src/lib.rs b/test-cases/zero-address/zero-address-1/vulnerable-example/src/lib.rs deleted file mode 100644 index f1e04235..00000000 --- a/test-cases/zero-address/zero-address-1/vulnerable-example/src/lib.rs +++ /dev/null @@ -1,88 +0,0 @@ -#![no_std] - -#[cfg(any(test, feature = "testutils"))] -extern crate std; - -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env}; - -#[contracttype] -#[derive(Clone)] -enum DataKey { - Admin, - Data, -} - -#[contract] -pub struct ZeroAddressContract; - -#[contracterror] -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -#[repr(u32)] -pub enum Error { - Ununitialized = 1, - NotAdmin = 2, - NoData = 3, -} - -#[contractimpl] -impl ZeroAddressContract { - pub fn init(e: Env, admin: Address) { - e.storage().persistent().set(&DataKey::Admin, &admin); - } - - fn ensure_is_admin(e: &Env, admin: Address) -> Result { - let registered_admin = e - .storage() - .persistent() - .get::(&DataKey::Admin) - .ok_or(Error::Ununitialized)?; - if admin != registered_admin { - return Ok(false); - } - admin.require_auth(); - Ok(true) - } - - pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - e.storage().persistent().set(&DataKey::Data, &data); - Ok(()) - } - - pub fn get(e: Env) -> Result { - e.storage() - .persistent() - .get::(&DataKey::Data) - .ok_or(Error::NoData) - } - - pub fn change_admin(e: Env, admin: Address, new_admin: Address) -> Result<(), Error> { - if !ZeroAddressContract::ensure_is_admin(&e, admin)? { - return Err(Error::NotAdmin); - } - - e.storage().persistent().set(&DataKey::Admin, &new_admin); - - Ok(()) - } -} - -#[test] -fn simple_test() { - use soroban_sdk::testutils::Address as _; - - let e = Env::default(); - e.mock_all_auths(); - let client = - ZeroAddressContractClient::new(&e, &e.register_contract(None, ZeroAddressContract {})); - let admin = Address::generate(&e); - client.init(&admin); - assert_eq!(client.try_get(), Err(Ok(Error::NoData))); - client.set(&admin, &5); - assert_eq!(client.get(), 5); - client.change_admin(&admin, &Address::generate(&e)); - assert_eq!(client.get(), 5); - assert_eq!(client.try_set(&admin, &6), Err(Ok(Error::NotAdmin))); -}