diff --git a/README.md b/README.md index 903707e5..57b1d87e 100644 --- a/README.md +++ b/README.md @@ -61,8 +61,9 @@ 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 diff --git a/detectors/zero-address/Cargo.toml b/detectors/front-running/Cargo.toml similarity index 93% rename from detectors/zero-address/Cargo.toml rename to detectors/front-running/Cargo.toml index 21980847..48cb9894 100644 --- a/detectors/zero-address/Cargo.toml +++ b/detectors/front-running/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "zero-address" +name = "front-running" version = "0.1.0" [lib] diff --git a/detectors/front-running/src/conditional_checker.rs b/detectors/front-running/src/conditional_checker.rs new file mode 100644 index 00000000..b6a56ab7 --- /dev/null +++ b/detectors/front-running/src/conditional_checker.rs @@ -0,0 +1,64 @@ +use rustc_hir::{def::Res, BinOpKind, Expr, ExprKind, HirId, QPath, UnOp}; + +const PANIC_INDUCING_FUNCTIONS: [&str; 2] = ["panic", "bail"]; + +#[derive(Clone, Copy, Hash, Eq, PartialEq, Default, Debug)] +pub struct ConditionalChecker { + pub greater_expr: Option, + pub lesser_expr: Option, +} + +impl ConditionalChecker { + fn handle_binary_op(&mut self, op: BinOpKind, left: &Expr, right: &Expr) -> bool { + match op { + BinOpKind::Ge | BinOpKind::Gt => { + self.greater_expr = get_res_hir_id(left); + self.lesser_expr = get_res_hir_id(right); + true + } + BinOpKind::Le | BinOpKind::Lt => { + self.lesser_expr = get_res_hir_id(left); + self.greater_expr = get_res_hir_id(right); + true + } + _ => false, + } + } + + pub fn handle_condition(&mut self, condition: &Expr) -> bool { + match &condition.kind { + ExprKind::Binary(op, left, right) => self.handle_binary_op(op.node, left, right), + ExprKind::Unary(UnOp::Not, unary_expr) => { + if let ExprKind::Binary(op, left, right) = &unary_expr.kind { + self.handle_binary_op(op.node, left, right) + } else { + false + } + } + _ => false, + } + } +} + +pub fn get_res_hir_id(expr: &Expr) -> Option { + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { + if let Res::Local(hir_id) = path.res { + Some(hir_id) + } else { + None + } + } else { + None + } +} + +pub fn is_panic_inducing_call(func: &Expr<'_>) -> bool { + if let ExprKind::Path(QPath::Resolved(_, path)) = &func.kind { + return PANIC_INDUCING_FUNCTIONS.iter().any(|&func| { + path.segments + .iter() + .any(|segment| segment.ident.name.as_str().contains(func)) + }); + } + false +} diff --git a/detectors/front-running/src/lib.rs b/detectors/front-running/src/lib.rs new file mode 100644 index 00000000..20a6d267 --- /dev/null +++ b/detectors/front-running/src/lib.rs @@ -0,0 +1,195 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_span; + +mod conditional_checker; + +use clippy_utils::higher::If; +use clippy_wrappers::span_lint; +use conditional_checker::{get_res_hir_id, is_panic_inducing_call, ConditionalChecker}; +use if_chain::if_chain; +use rustc_hir::{ + def::Res::{self}, + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, HirId, LetStmt, Path, QPath, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::{ + def_id::{DefId, LocalDefId}, + Span, Symbol, +}; +use std::{ + collections::{HashMap, HashSet}, + vec, +}; +use utils::{get_node_type_opt, FunctionCallVisitor}; + +const LINT_MESSAGE: &str = + "The transferred amount should be checked against a minimum to prevent front-running"; + +dylint_linting::impl_late_lint! { + pub FRONT_RUNNING, + Warn, + LINT_MESSAGE, + FrontRunning::default(), + { + name: "Front Running Detection", + long_message: "This lint checks for potential front-running vulnerabilities in token transfers", + severity: "Warning", + help: "Consider implementing a minimum amount check before the transfer", + vulnerability_class: "MEV", + } +} + +#[derive(Default)] +struct FrontRunning { + function_call_graph: HashMap>, + checked_functions: HashSet, +} + +impl<'tcx> LateLintPass<'tcx> for FrontRunning { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + local_def_id: LocalDefId, + ) { + let def_id = local_def_id.to_def_id(); + self.checked_functions.insert(cx.tcx.def_path_str(def_id)); + + if span.from_expansion() { + return; + } + + let mut function_call_visitor = + FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); + function_call_visitor.visit_body(body); + + let function_params: HashSet<_> = + body.params.iter().map(|param| param.pat.hir_id).collect(); + + let mut front_running_visitor = FrontRunningVisitor { + checked_hir_ids: HashSet::new(), + conditional_checker: Vec::new(), + cx, + filtered_local_variables: HashSet::new(), + function_params, + transfer_amount_id: Vec::new(), + }; + front_running_visitor.visit_body(body); + + for (transfer_amount_id, span) in &front_running_visitor.transfer_amount_id { + if !front_running_visitor + .checked_hir_ids + .contains(transfer_amount_id) + && front_running_visitor + .filtered_local_variables + .contains(transfer_amount_id) + { + span_lint(cx, FRONT_RUNNING, *span, LINT_MESSAGE); + } + } + } +} + +struct FrontRunningVisitor<'a, 'tcx> { + checked_hir_ids: HashSet, + conditional_checker: Vec, + cx: &'a LateContext<'tcx>, + filtered_local_variables: HashSet, + function_params: HashSet, + transfer_amount_id: Vec<(HirId, Span)>, +} + +impl FrontRunningVisitor<'_, '_> { + fn add_to_checked_hir_ids(&mut self, last_conditional_checker: ConditionalChecker) { + if let (Some(lesser_hir_id), Some(greater_hir_id)) = ( + last_conditional_checker.lesser_expr, + last_conditional_checker.greater_expr, + ) { + if self.function_params.contains(&lesser_hir_id) { + self.checked_hir_ids.insert(greater_hir_id); + } + } + } + + fn local_uses_parameter(&self, expr: &Expr) -> bool { + match &expr.kind { + ExprKind::MethodCall(_, _, args, _) | ExprKind::Call(_, args) => { + args.iter().any(|arg| { + get_res_hir_id(arg) + .map_or(false, |hir_id| self.function_params.contains(&hir_id)) + }) + } + ExprKind::Binary(_, left_expr, right_expr) => { + self.local_uses_parameter(left_expr) || self.local_uses_parameter(right_expr) + } + _ => false, + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { + fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) { + if let Some(init) = &local.init { + if self.local_uses_parameter(init) { + self.filtered_local_variables.insert(local.pat.hir_id); + } + } + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + // Check if the expression is a transfer method call, then store the HirId of the amount parameter + if_chain! { + if let ExprKind::MethodCall(path_segment, receiver, args, ..) = expr.kind; + if path_segment.ident.name == Symbol::intern("transfer"); + if let Some(receiver_type) = get_node_type_opt(self.cx, &receiver.hir_id); + if receiver_type.to_string() == "soroban_sdk::token::TokenClient<'_>"; + if let ExprKind::AddrOf(_, _, amount_expr, ..) = args[2].kind; + if let ExprKind::Path(QPath::Resolved(_, Path { segments, .. }), ..) = amount_expr.kind; + if let Some(segment) = segments.first(); + if let Res::Local(hir_id) = segment.res; + then { + self.transfer_amount_id.push((hir_id, expr.span)); + } + } + + // If we are inside an 'if' statement, check if the current expression is a return or a panic inducing call + if let Some(last_conditional_checker) = self.conditional_checker.last().copied() { + match &expr.kind { + ExprKind::Ret(..) => { + self.add_to_checked_hir_ids(last_conditional_checker); + } + ExprKind::Call(func, _) if is_panic_inducing_call(func) => { + self.add_to_checked_hir_ids(last_conditional_checker); + } + _ => {} + } + } + + // Check if the expression has an 'if' and if it does, check if it meets our condition + if let Some(If { + cond, + then: if_expr, + r#else: _, + }) = If::hir(expr) + { + let mut conditional_checker = ConditionalChecker { + greater_expr: None, + lesser_expr: None, + }; + if conditional_checker.handle_condition(cond) { + self.conditional_checker.push(conditional_checker); + walk_expr(self, if_expr); + self.conditional_checker.pop(); + return; + } + } + + walk_expr(self, expr); + } +} diff --git a/detectors/token-interface-inference/Cargo.toml b/detectors/token-interface-inference/Cargo.toml new file mode 100644 index 00000000..4ad4c023 --- /dev/null +++ b/detectors/token-interface-inference/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "token-interface-inference" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +utils = { workspace = true } +edit-distance = "=2.1.2" + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/token-interface-inference/src/lib.rs b/detectors/token-interface-inference/src/lib.rs new file mode 100644 index 00000000..b339a222 --- /dev/null +++ b/detectors/token-interface-inference/src/lib.rs @@ -0,0 +1,159 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint_and_help; +use edit_distance::edit_distance; +use rustc_hir::{intravisit::Visitor, Node}; +use rustc_lint::{LateContext, LateLintPass}; + +use rustc_errors::MultiSpan; +use rustc_span::Span; + +use std::collections::HashSet; +use std::vec; +use std::{ + collections::HashMap, + ops::{Div, Mul}, +}; +use utils::FunctionCallVisitor; + +use rustc_span::def_id::DefId; + +const LINT_MESSAGE: &str = + "This contract seems like a Token, consider implementing the Token Interface trait"; +const CANONICAL_FUNCTIONS_AMOUNT: u16 = 10; +const INCLUDED_FUNCTIONS_THRESHOLD: u16 = 60; + +dylint_linting::impl_late_lint! { + pub TOKEN_INTERFACE_INFERENCE, + Warn, + "", + TokenInterfaceInference::default(), + { + name: "Token Interface Implementation Analyzer", + long_message: "Implementing the Token Interface trait helps to ensure proper compliance of the SEP-41 standard.", + severity: "Enhancement", + help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/token-interface-inference", + vulnerability_class: "Best Practices", + } +} + +#[derive(Default)] +struct TokenInterfaceInference { + function_call_graph: HashMap>, + checked_functions: HashSet, + canonical_funcs_def_id: HashSet, + impl_token_interface_trait: bool, + detected_canonical_functions_count: u16, + funcs_spans: Vec, +} + +impl<'tcx> LateLintPass<'tcx> for TokenInterfaceInference { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) { + if let rustc_hir::ItemKind::Impl(impl_block) = item.kind { + if let Some(trait_ref) = impl_block.of_trait { + let trait_def_id = trait_ref.path.res.def_id(); + let trait_name = cx.tcx.def_path_str(trait_def_id); + + if trait_name == "soroban_sdk::token::TokenInterface" { + self.impl_token_interface_trait = true; + } + } + } + } + + fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { + // Verify if the contract implements the token interface. + if self.impl_token_interface_trait { + return; + } + + if self + .detected_canonical_functions_count + .mul(100) + .div(CANONICAL_FUNCTIONS_AMOUNT) + >= INCLUDED_FUNCTIONS_THRESHOLD + { + span_lint_and_help( + cx, + TOKEN_INTERFACE_INFERENCE, + MultiSpan::from_spans(self.funcs_spans.clone()), + LINT_MESSAGE, + None, + "", + ); + } + } + + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: rustc_hir::intravisit::FnKind<'tcx>, + _fn_decl: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx rustc_hir::Body<'tcx>, + span: Span, + local_def_id: rustc_span::def_id::LocalDefId, + ) { + let def_id = local_def_id.to_def_id(); + self.checked_functions.insert(cx.tcx.def_path_str(def_id)); + + if span.from_expansion() { + return; + } + + let fn_name = cx.tcx.def_path_str(def_id); + + let fn_name_span = if let Some(node) = cx.tcx.hir().get_if_local(def_id) { + match node { + Node::Item(item) => Some(item.ident.span), + Node::ImplItem(impl_item) => Some(impl_item.ident.span), + _ => None, + } + } else { + None + }; + + let mut function_call_visitor = + FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); + function_call_visitor.visit_body(body); + + // If the function is part of the token interface, I store its defid. + if verify_token_interface_function_similarity(fn_name.clone()) { + self.detected_canonical_functions_count += 1; + self.canonical_funcs_def_id.insert(def_id); + if let Some(span) = fn_name_span { + self.funcs_spans.push(span); + } + } + } +} + +fn verify_token_interface_function_similarity(fn_name: String) -> bool { + let canonical_functions_formatted = [ + "allowance", + "approve", + "balance", + "transfer", + "transferfrom", + "burn", + "burnfrom", + "decimals", + "name", + "symbol", + "mint", + ]; + let function_name = String::from(fn_name.split("::").last().unwrap()); + let formatted_name: String = function_name + .to_lowercase() + .replace("_", "") + .split_whitespace() + .collect(); + + canonical_functions_formatted + .iter() + .any(|cf| edit_distance(formatted_name.as_str(), cf) <= 1) +} 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/docs/docs/detectors/24-token-interface-events.md b/docs/docs/detectors/24-token-interface-events.md new file mode 100644 index 00000000..df6ef16c --- /dev/null +++ b/docs/docs/detectors/24-token-interface-events.md @@ -0,0 +1,79 @@ +# Token interface events + +## Description + +- Category: `Best practices` +- Severity: `Medium` +- Detectors: [`token-interface-events`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/token-interface-events) +- Test Cases: [`token-interface-events-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/token-interface-events/token-interface-events-1) + + +In Rust, the token contracts have a special interface with certain requirements. One of these requirements is related to events; this requirement states that token functions must emit the events in the specified format. If this does not happen, the contract will have potential errors. + +## Why is this bad? + +If the token functions do not emit events, the following errors may occur: + +* Token standard compliance + +* Transparency: Events provide a transparent way to log and broadcast important actions like token transfers, approvals, and minting/burning. This transparency is crucial for users, developers, and external systems to monitor and react to contract activities. + +* Interoperability: Many decentralized applications (dApps) rely on events to interact with tokens. Without events, these applications might not be able to function correctly, as they would have no way of knowing when a transfer or other important action has occurred. Also, off-chain systems, like wallets, exchanges, and block explorers, use events to track token activity. If events are not implemented, these systems may encounter errors in providing accurate and real-time information about the token. + +* Debugging and Auditing: Events are very helpful for debugging and auditing smart contracts. They are useful because they provide detailed information about what happened in the contract during execution. + +## Issue example + +Consider the following `Soroban` contract: + +```rust + + fn transfer(env: Env, from: Address, to: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to), &(to_balance + amount)); + } + +``` + +In this example, the `transfer()` function does not emit an event. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/token-interface-events/token-interface-events-1/vulnerable-example). + + +## Remediated example + +```rust + fn transfer(env: Env, from: Address, to: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } +``` +In this example, the `transfer()` function emits an event. + +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/token-interface-events/token-interface-events-1/remediated-example). + +## How is it detected? + +If the token interface trait is being used, check if all of the token's functions emit events. + + + + diff --git a/test-cases/zero-address/Cargo.toml b/test-cases/front-running/Cargo.toml similarity index 81% rename from test-cases/zero-address/Cargo.toml rename to test-cases/front-running/Cargo.toml index cedd9259..5a1bfab3 100644 --- a/test-cases/zero-address/Cargo.toml +++ b/test-cases/front-running/Cargo.toml @@ -1,10 +1,10 @@ [workspace] exclude = [".cargo", "target"] -members = ["zero-address-*/*"] +members = ["front-running-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=21.4.0" } +soroban-sdk = { version = "=20.0.0" } [profile.release] codegen-units = 1 @@ -19,3 +19,4 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" + diff --git a/test-cases/front-running/front-running-1/remediated-example/Cargo.toml b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..a9947e6b --- /dev/null +++ b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "front-running-remediated-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0" } +soroban-token-sdk = { version = "20.0.0" } + +[dev-dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/front-running/front-running-1/remediated-example/src/lib.rs b/test-cases/front-running/front-running-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..6795865a --- /dev/null +++ b/test-cases/front-running/front-running-1/remediated-example/src/lib.rs @@ -0,0 +1,173 @@ +#![no_std] + +use soroban_sdk::{ + contract, contractimpl, contracttype, + token::{StellarAssetClient, TokenClient}, + Address, Env, +}; + +#[contracttype] +pub enum DataKey { + Token, +} + +#[contract] +pub struct FrontRunning; + +#[contractimpl] +impl FrontRunning { + pub fn init(e: Env, contract: Address) { + e.storage().persistent().set(&DataKey::Token, &contract); + } + + pub fn get_token(e: Env) -> Address { + get_token(&e) + } + + pub fn mint(e: Env, to: Address, amount: i128) { + StellarAssetClient::new(&e, &get_token(&e)).mint(&to, &amount); + } + + pub fn approve(e: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u32) { + TokenClient::new(&e, &get_token(&e)).approve(&from, &spender, &amount, &expiration_ledger); + } + + pub fn transfer(e: Env, from: Address, to: Address, amount: i128, min_amount: i128) { + let transfer_amount = get_conversion_price(amount); + assert!(transfer_amount >= min_amount, "Insufficient amount"); + TokenClient::new(&e, &get_token(&e)).transfer(&from, &to, &transfer_amount); + } + + pub fn allowance(e: Env, from: Address, spender: Address) -> i128 { + TokenClient::new(&e, &get_token(&e)).allowance(&from, &spender) + } +} + +fn get_token(e: &Env) -> Address { + e.storage().persistent().get(&DataKey::Token).unwrap() +} + +fn get_conversion_price(amount: i128) -> i128 { + // This function symbolizes a change in the state of the blockchain + 100 * amount +} + +#[cfg(test)] +mod tests { + extern crate std; + + use soroban_sdk::{ + testutils::{AuthorizedFunction, AuthorizedInvocation}, + token::TokenClient, + Address, Env, IntoVal, Symbol, + }; + + use crate::{FrontRunning, FrontRunningClient}; + use soroban_sdk::testutils::Address as _; + + #[test] + fn test_approve() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let from = Address::generate(&env); + let spender = Address::generate(&env); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .approve(&from, &spender, &200, &200); + + let auths = env.auths(); + assert_eq!( + auths, + std::vec![( + from.clone(), + AuthorizedInvocation { + function: AuthorizedFunction::Contract(( + token_contract_id.clone(), + Symbol::new(&env, "approve"), + (&from, &spender, 200_i128, 200_u32).into_val(&env) + )), + sub_invocations: std::vec![] + } + )] + ); + + // Check allowance + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.allowance(&from, &spender), 200); + } + + #[test] + fn test_transfer() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &1, &0); + + // Check final balances + let from_balance = token_client.balance(&from); + let to_balance = token_client.balance(&to); + + assert_eq!(from_balance, 10000 - 100); + assert_eq!(to_balance, 100); + } + + #[test] + #[should_panic(expected = "Insufficient amount")] + fn test_front_running() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &100, &10001); + // The contract should now panic since the transfer amount is less than the minimum amount + } +} diff --git a/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..77555903 --- /dev/null +++ b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "front-running-vulnerable-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0" } +soroban-token-sdk = { version = "20.0.0" } + +[dev-dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs b/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..b7fbf131 --- /dev/null +++ b/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs @@ -0,0 +1,142 @@ +#![no_std] + +use soroban_sdk::{ + contract, contractimpl, contracttype, + token::{StellarAssetClient, TokenClient}, + Address, Env, +}; + +#[contracttype] +pub enum DataKey { + Token, +} + +#[contract] +pub struct FrontRunning; + +#[contractimpl] +impl FrontRunning { + pub fn init(e: Env, contract: Address) { + e.storage().persistent().set(&DataKey::Token, &contract); + } + + pub fn get_token(e: Env) -> Address { + get_token(&e) + } + + pub fn mint(e: Env, to: Address, amount: i128) { + StellarAssetClient::new(&e, &get_token(&e)).mint(&to, &amount); + } + + pub fn approve(e: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u32) { + TokenClient::new(&e, &get_token(&e)).approve(&from, &spender, &amount, &expiration_ledger); + } + + pub fn transfer(e: Env, from: Address, to: Address, amount: i128) { + let transfer_amount = get_conversion_price(amount); + TokenClient::new(&e, &get_token(&e)).transfer(&from, &to, &transfer_amount); + } + + pub fn allowance(e: Env, from: Address, spender: Address) -> i128 { + TokenClient::new(&e, &get_token(&e)).allowance(&from, &spender) + } +} + +fn get_token(e: &Env) -> Address { + e.storage().persistent().get(&DataKey::Token).unwrap() +} + +fn get_conversion_price(amount: i128) -> i128 { + // This function symbolizes a change in the state of the blockchain + 100 * amount +} + +#[cfg(test)] +mod tests { + extern crate std; + + use soroban_sdk::{ + testutils::{AuthorizedFunction, AuthorizedInvocation}, + token::TokenClient, + Address, Env, IntoVal, Symbol, + }; + + use crate::{FrontRunning, FrontRunningClient}; + use soroban_sdk::testutils::Address as _; + + #[test] + fn test_approve() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let from = Address::generate(&env); + let spender = Address::generate(&env); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .approve(&from, &spender, &200, &200); + + let auths = env.auths(); + assert_eq!( + auths, + std::vec![( + from.clone(), + AuthorizedInvocation { + function: AuthorizedFunction::Contract(( + token_contract_id.clone(), + Symbol::new(&env, "approve"), + (&from, &spender, 200_i128, 200_u32).into_val(&env) + )), + sub_invocations: std::vec![] + } + )] + ); + + // Check allowance + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.allowance(&from, &spender), 200); + } + + #[test] + fn test_transfer() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &1); + + // Check final balances (optional) + let from_balance = token_client.balance(&from); + let to_balance = token_client.balance(&to); + + assert_eq!(from_balance, 10000 - 100); + assert_eq!(to_balance, 100); + } +} diff --git a/test-cases/token-interface-inference/Cargo.toml b/test-cases/token-interface-inference/Cargo.toml new file mode 100644 index 00000000..2a6b4662 --- /dev/null +++ b/test-cases/token-interface-inference/Cargo.toml @@ -0,0 +1,23 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["token-interface-inference-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=21.4.0" } +soroban-token-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/token-interface-inference/token-interface-inference-1/remediated-example/Cargo.toml b/test-cases/token-interface-inference/token-interface-inference-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..00a49f87 --- /dev/null +++ b/test-cases/token-interface-inference/token-interface-inference-1/remediated-example/Cargo.toml @@ -0,0 +1,35 @@ +[package] +name = "token-interface-inference-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "=21.4.0" +soroban-token-sdk = { version = "=21.4.0" } + + +[dev_dependencies] +soroban-sdk = { version = "=21.4.0", features = ["testutils"] } +soroban-token-sdk = { version = "=21.4.0" } + + +[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/token-interface-inference/token-interface-inference-1/remediated-example/src/lib.rs b/test-cases/token-interface-inference/token-interface-inference-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..545ad103 --- /dev/null +++ b/test-cases/token-interface-inference/token-interface-inference-1/remediated-example/src/lib.rs @@ -0,0 +1,211 @@ +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, token, Address, Env, String, +}; + +use soroban_sdk::token::TokenInterface; +use soroban_token_sdk::TokenUtils; + +#[derive(Clone)] +#[contracttype] +pub struct TokenMetadata { + pub decimals: u32, + pub name: String, + pub symbol: String, + pub admin: Address, +} + +#[derive(Clone, Default)] +#[contracttype] +pub struct AllowanceFromSpender { + pub amount: i128, + pub expiration_ledger: u32, +} + +#[derive(Clone)] +#[contracttype] +pub enum DataKey { + Balance(Address), + TokenMetadata, + AllowanceFromSpender(Address, Address), +} + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum VTError { + AlreadyInitialized = 1, + NotInitialized = 2, +} + +#[contract] +pub struct TokenInterfaceEvents; + +#[contractimpl] +impl TokenInterfaceEvents { + pub fn initialize( + env: Env, + admin: Address, + decimals: u32, + name: String, + symbol: String, + ) -> Result<(), VTError> { + let current_token_metadata: Option = + env.storage().instance().get(&DataKey::TokenMetadata); + if current_token_metadata.is_some() { + return Err(VTError::AlreadyInitialized); + } else { + env.storage().instance().set( + &DataKey::TokenMetadata, + &TokenMetadata { + decimals, + name, + symbol, + admin, + }, + ); + } + + Ok(()) + } + + pub fn get_metadata(env: Env) -> TokenMetadata { + env.storage() + .instance() + .get(&DataKey::TokenMetadata) + .unwrap() + } + + pub fn mint(env: Env, to: Address, amount: i128) { + Self::get_metadata(env.clone()).admin.require_auth(); + let previous_balance: i128 = env + .clone() + .storage() + .instance() + .get(&DataKey::Balance(to.clone())) + .unwrap_or(0); + env.storage() + .instance() + .set(&DataKey::Balance(to), &(previous_balance + amount)); + } + + fn get_allowance(env: Env, from: Address, spender: Address) -> AllowanceFromSpender { + env.storage() + .instance() + .get(&DataKey::AllowanceFromSpender(from, spender)) + .unwrap_or_default() + } +} + +#[contractimpl] +impl token::TokenInterface for TokenInterfaceEvents { + fn allowance(env: Env, from: Address, spender: Address) -> i128 { + let allowance = Self::get_allowance(env.clone(), from, spender); + if allowance.expiration_ledger < env.ledger().sequence() { + 0 + } else { + allowance.amount + } + } + + fn approve(env: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u32) { + from.require_auth(); + assert!(env.ledger().sequence() < expiration_ledger || amount == 0); + env.storage().instance().set( + &DataKey::AllowanceFromSpender(from.clone(), spender.clone()), + &AllowanceFromSpender { + amount, + expiration_ledger, + }, + ); + + TokenUtils::new(&env) + .events() + .approve(from, spender, amount, expiration_ledger); + } + + fn balance(env: Env, id: Address) -> i128 { + env.storage() + .instance() + .get(&DataKey::Balance(id)) + .unwrap_or(0) + } + + fn transfer(env: Env, from: Address, to: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + fn transfer_from(env: Env, spender: Address, from: Address, to: Address, amount: i128) { + let spender_allowance = Self::allowance(env.clone(), from.clone(), spender.clone()); + assert!(spender_allowance >= amount); + + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + let mut allowance = Self::get_allowance(env.clone(), from.clone(), spender.clone()); + allowance.amount -= amount; + + env.storage().instance().set( + &DataKey::AllowanceFromSpender(from.clone(), spender), + &allowance, + ); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + fn burn(env: Env, from: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + TokenUtils::new(&env).events().burn(from, amount); + } + + fn burn_from(env: Env, spender: Address, from: Address, amount: i128) { + let spender_allowance = Self::allowance(env.clone(), from.clone(), spender.clone()); + assert!(spender_allowance >= amount); + let from_balance = Self::balance(env.clone(), from.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + + let mut allowance = Self::get_allowance(env.clone(), from.clone(), spender.clone()); + allowance.amount -= amount; + env.storage().instance().set( + &DataKey::AllowanceFromSpender(from.clone(), spender), + &allowance, + ); + TokenUtils::new(&env).events().burn(from, amount); + } + fn decimals(env: Env) -> u32 { + Self::get_metadata(env).decimals + } + fn name(env: Env) -> String { + Self::get_metadata(env).name + } + fn symbol(env: Env) -> String { + Self::get_metadata(env).symbol + } +} diff --git a/test-cases/token-interface-inference/token-interface-inference-1/vulnerable-example/Cargo.toml b/test-cases/token-interface-inference/token-interface-inference-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..dfa7802b --- /dev/null +++ b/test-cases/token-interface-inference/token-interface-inference-1/vulnerable-example/Cargo.toml @@ -0,0 +1,36 @@ +[package] +name = "token-interface-inference-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "=21.4.0" +soroban-token-sdk = { version = "=21.4.0" } + + +[dev_dependencies] +soroban-sdk = { version = "=21.4.0", features = ["testutils"] } +soroban-token-sdk = { version = "=21.4.0" } + + +[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/token-interface-inference/token-interface-inference-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-inference/token-interface-inference-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..6e4b0ae0 --- /dev/null +++ b/test-cases/token-interface-inference/token-interface-inference-1/vulnerable-example/src/lib.rs @@ -0,0 +1,176 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; + +#[derive(Clone, Debug)] +#[contracttype] +pub struct TokenMetadata { + pub decimals: u32, + pub name: String, + pub symbol: String, + pub admin: Address, +} + +#[derive(Clone, Default)] +#[contracttype] +pub struct AllowanceFromSpender { + pub amount: i128, + pub expiration_ledger: u32, +} + +#[derive(Clone)] +#[contracttype] +pub enum DataKey { + Balance(Address), + TokenMetadata, + AllowanceFromSpender(Address, Address), +} + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum TIIError { + AlreadyInitialized = 1, + NotInitialized = 2, +} + +#[contract] +pub struct TokenInterfaceInference; + +#[contractimpl] +impl TokenInterfaceInference { + pub fn initialize( + env: Env, + admin: Address, + decimals: u32, + name: String, + symbol: String, + ) -> Result<(), TIIError> { + let current_token_metadata: Option = + env.storage().instance().get(&DataKey::TokenMetadata); + if current_token_metadata.is_some() { + return Err(TIIError::AlreadyInitialized); + } else { + env.storage().instance().set( + &DataKey::TokenMetadata, + &TokenMetadata { + decimals, + name, + symbol, + admin, + }, + ); + } + + Ok(()) + } + + pub fn get_metadata(env: Env) -> TokenMetadata { + env.storage() + .instance() + .get(&DataKey::TokenMetadata) + .unwrap() + } + + pub fn mint(env: Env, to: Address, amount: i128) { + Self::get_metadata(env.clone()).admin.require_auth(); + let previous_balance: i128 = env + .clone() + .storage() + .instance() + .get(&DataKey::Balance(to.clone())) + .unwrap_or(0); + env.storage() + .instance() + .set(&DataKey::Balance(to), &(previous_balance + amount)); + } + + pub fn transfer(env: Env, from: Address, to: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to), &(to_balance + amount)); + } + + pub fn balance(env: Env, id: Address) -> i128 { + env.storage() + .instance() + .get(&DataKey::Balance(id)) + .unwrap_or(0) + } + + pub fn burn(env: Env, from: Address, amount: i128) { + from.require_auth(); + let from_balance = Self::balance(env.clone(), from.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from), &(from_balance - amount)); + } + + pub fn approve( + env: Env, + from: Address, + spender: Address, + amount: i128, + expiration_ledger: u32, + ) { + from.require_auth(); + assert!(env.ledger().sequence() < expiration_ledger || amount == 0); + env.storage().instance().set( + &DataKey::AllowanceFromSpender(from, spender), + &AllowanceFromSpender { + amount, + expiration_ledger, + }, + ); + } + + pub fn transfer_from(env: Env, spender: Address, from: Address, to: Address, amount: i128) { + let spender_allowance = Self::allowance(env.clone(), from.clone(), spender.clone()); + assert!(spender_allowance >= amount); + + let from_balance = Self::balance(env.clone(), from.clone()); + let to_balance = Self::balance(env.clone(), to.clone()); + assert!(from_balance >= amount); + env.storage() + .instance() + .set(&DataKey::Balance(from.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + let mut allowance = Self::get_allowance(env.clone(), from.clone(), spender.clone()); + allowance.amount -= amount; + + env.storage() + .instance() + .set(&DataKey::AllowanceFromSpender(from, spender), &allowance); + } + + pub fn allowance(env: Env, from: Address, spender: Address) -> i128 { + let allowance = Self::get_allowance(env.clone(), from, spender); + if allowance.expiration_ledger < env.ledger().sequence() { + 0 + } else { + allowance.amount + } + } + + pub fn decimal(env: Env) -> u32 { + Self::get_metadata(env).decimals + } + + fn get_allowance(env: Env, from: Address, spender: Address) -> AllowanceFromSpender { + env.storage() + .instance() + .get(&DataKey::AllowanceFromSpender(from, spender)) + .unwrap_or_default() + } +} 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))); -}