From f71ade0aacb4a833823cc1ad89e8ecea7e8eab61 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 3 Sep 2024 11:26:06 -0300 Subject: [PATCH] Remove zero-address detector --- README.md | 45 ++-- detectors/zero-address/Cargo.toml | 17 -- detectors/zero-address/src/lib.rs | 255 ------------------ ...tion.md => 20-incorrect-exponentiation.md} | 0 .../docs/detectors/20-zero-or-test-address.md | 43 --- ...d => 21-integer-overflow -or-underflow.md} | 0 test-cases/zero-address/Cargo.toml | 21 -- .../remediated-example/Cargo.toml | 16 -- .../remediated-example/src/lib.rs | 134 --------- .../vulnerable-example/Cargo.toml | 16 -- .../vulnerable-example/src/lib.rs | 88 ------ 11 files changed, 22 insertions(+), 613 deletions(-) delete mode 100644 detectors/zero-address/Cargo.toml delete mode 100644 detectors/zero-address/src/lib.rs rename docs/docs/detectors/{21-incorrect-exponentiation.md => 20-incorrect-exponentiation.md} (100%) delete mode 100644 docs/docs/detectors/20-zero-or-test-address.md rename docs/docs/detectors/{22-integer-overflow -or-underflow.md => 21-integer-overflow -or-underflow.md} (100%) delete mode 100644 test-cases/zero-address/Cargo.toml delete mode 100644 test-cases/zero-address/zero-address-1/remediated-example/Cargo.toml delete mode 100644 test-cases/zero-address/zero-address-1/remediated-example/src/lib.rs delete mode 100644 test-cases/zero-address/zero-address-1/vulnerable-example/Cargo.toml delete mode 100644 test-cases/zero-address/zero-address-1/vulnerable-example/src/lib.rs diff --git a/README.md b/README.md index 903707e5..17d0e56b 100644 --- a/README.md +++ b/README.md @@ -40,29 +40,28 @@ For more information on installation and usage, please refer to the [Getting Sta Currently Scout includes the following detectors. -| Detector ID | What it Detects | Test Cases | Severity | -| ------------------------------------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | -| [divide-before-multiply](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply) | Performing a division operation before a multiplication, leading to loss of precision. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-3) | Medium | -| [unsafe-unwrap](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap) | Inappropriate usage of the unwrap method, causing unexpected program crashes. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1)| Medium | -| [unsafe-expect](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-expect) | Improper usage of the expect method, leading to unexpected program crashes. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-expect/unsafe-expect-1)| Medium | -| [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check) | An arithmetic operation overflows or underflows the available memory allocated to the variable. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1)| Critical | -| [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values) | Avoid using block attributes for random number generation to prevent manipulation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)| Critical | -| [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1)| Critical | -| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) | Enhancement | -| [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | -| [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | -| [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | -| [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation) | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3) | Medium | -| [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) | Using an old version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) | Enhancement | -| [unused-return-enum](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum) | Return enum from a function is not completely used. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) | Minor | -| [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), [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 | +| Detector ID | What it Detects | Test Cases | Severity | +| ---------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | +| [divide-before-multiply](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply) | Performing a division operation before a multiplication, leading to loss of precision. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-3) | Medium | +| [unsafe-unwrap](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap) | Inappropriate usage of the unwrap method, causing unexpected program crashes. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1) | Medium | +| [unsafe-expect](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-expect) | Improper usage of the expect method, leading to unexpected program crashes. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-expect/unsafe-expect-1) | Medium | +| [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check) | An arithmetic operation overflows or underflows the available memory allocated to the variable. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1) | Critical | +| [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values) | Avoid using block attributes for random number generation to prevent manipulation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1) | Critical | +| [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) | Critical | +| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) | Enhancement | +| [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | +| [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error) | Code panics on error instead of using descriptive enum. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1) | Enhancement | +| [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | +| [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation) | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3) | Medium | +| [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) | Using an old version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) | Enhancement | +| [unused-return-enum](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum) | Return enum from a function is not completely used. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) | Minor | +| [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), [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 | +| [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 | ## Output formats 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))); -}