From 54ca7f3351f3ec1266a19ccde0d3865d0efb5a42 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Mon, 12 Aug 2024 14:28:13 -0300 Subject: [PATCH 1/8] token interface events emission verifications, detector and test cases --- detectors/token-interface-events/Cargo.toml | 16 + detectors/token-interface-events/src/lib.rs | 273 ++++++++++++++++++ test-cases/token-interface-events/Cargo.toml | 28 ++ .../remediated-example/Cargo.toml | 39 +++ .../remediated-example/src/lib.rs | 211 ++++++++++++++ .../vulnerable-example/Cargo.toml | 39 +++ .../vulnerable-example/src/lib.rs | 201 +++++++++++++ .../remediated-example/Cargo.toml | 39 +++ .../remediated-example/src/lib.rs | 233 +++++++++++++++ .../vulnerable-example/Cargo.toml | 39 +++ .../vulnerable-example/src/lib.rs | 230 +++++++++++++++ utils/src/lib.rs | 3 + utils/src/token_interface_utils/mod.rs | 112 +++++++ 13 files changed, 1463 insertions(+) create mode 100644 detectors/token-interface-events/Cargo.toml create mode 100644 detectors/token-interface-events/src/lib.rs create mode 100644 test-cases/token-interface-events/Cargo.toml create mode 100644 test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml create mode 100644 test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs create mode 100644 test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs create mode 100644 test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml create mode 100644 test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs create mode 100644 test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml create mode 100644 test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs create mode 100644 utils/src/token_interface_utils/mod.rs diff --git a/detectors/token-interface-events/Cargo.toml b/detectors/token-interface-events/Cargo.toml new file mode 100644 index 00000000..644b6b4d --- /dev/null +++ b/detectors/token-interface-events/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "token-interface-events" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/token-interface-events/src/lib.rs b/detectors/token-interface-events/src/lib.rs new file mode 100644 index 00000000..ddef6874 --- /dev/null +++ b/detectors/token-interface-events/src/lib.rs @@ -0,0 +1,273 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint_and_help; + +use rustc_hir::{ + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, +}; +use rustc_lint::{LateContext, LateLintPass}; + +use rustc_span::Span; + +use std::collections::HashMap; +use std::collections::HashSet; +use std::vec; +use utils::{is_soroban_function, verify_token_interface_function, FunctionCallVisitor}; + +use rustc_span::def_id::DefId; + +const LINT_MESSAGE: &str = + "Storage change that involves token manipulation is performed without emiting an event"; +const CANONICAL_FUNCTIONS_AMOUNT: usize = 10; + +dylint_linting::impl_late_lint! { + pub VERIFY_TRANSFER, + Warn, + "", + TokenInterfaceEvents::default(), + { + name: "Storage Changed without Emiting an Event in Token Interface implementations", + long_message: " It can originate a problem when a canonical function does not emit an event expected by the contract's clients.", + severity: "", + help: "", + vulnerability_class: "", + } +} + +#[derive(Default)] +struct TokenInterfaceEvents { + function_call_graph: HashMap>, + checked_functions: HashSet, + eventless_storage_changers: HashSet, + defids_with_events: HashSet, + canonical_funcs_def_id: HashSet, + impl_token_interface_trait: bool, +} + +/// Used to add to a HashSet all the DefIds of the functions that are called starting from a specific parent in the function call graph. +/// # Params: +/// - fcg: function call graph that is used as reference. +/// - parent: the item from which the analysis starts. +/// - defids: the HashSet where all the defids found in the tree are collected. + +fn check_defids(fcg: &HashMap>, parent: &DefId, defids: &mut HashSet) { + let children = fcg.get(parent); + if children.is_some() { + for c in children.unwrap() { + if !defids.contains(c) { + defids.insert(*c); + check_defids(fcg, c, defids); + } + } + } +} + +/// Used to verify if, starting from a specific parent in the call graph, an event is emitted at any point of the flow. +/// # Params: +/// - fcg: function call graph +/// - parent: the item from which the analysis starts. +/// - check_against: a HashSet that is used to compare the defids. This HashSet is supposed to contain all the defids of the functions that emit events (collected by the `visit_expr` and `check_func` functions). +fn check_events_children( + fcg: &HashMap>, + parent: &DefId, + check_against: &HashSet, +) -> bool { + if check_against.contains(parent) { + return true; + } + let children = fcg.get(parent); + if children.is_some() { + for c in children.unwrap() { + if check_against.contains(c) || check_events_children(fcg, c, check_against) { + return true; + } + } + } + false +} + +/// Used to verify if, starting from a specific parent in the call graph, a function that sets storage in a considered "unsafe" way is called in any part of its flow. +/// # Params: +/// - fcg: function call graph +/// - func: the defid from which the analysis starts. +/// - unsafe_set_storage: a HashSet that is used to compare the defids. This HashSet is supposed to contain all the defids of the functions that are considered "unsafe storage setters". +fn check_storage_setters_calls( + fcg: &HashMap>, + func: &DefId, + unsafe_set_storage: &HashSet, +) -> bool { + if unsafe_set_storage.contains(func) { + return true; + } + let children = fcg.get(func); + if children.is_some() { + for c in children.unwrap() { + if unsafe_set_storage.contains(c) + || check_storage_setters_calls(fcg, c, unsafe_set_storage) + { + return true; + } + } + } + false +} + +impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { + 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 all of the canonical functions have not been detected, we assume it is not. + if self.canonical_funcs_def_id.len() != CANONICAL_FUNCTIONS_AMOUNT + && !self.impl_token_interface_trait + { + return; + } + + // Get the defids of every function that is called either directly or indirectly by all of the functions that are a part of the token interface. + let mut called_by_canonical_functions: HashSet = HashSet::new(); + for cf in &self.canonical_funcs_def_id { + check_defids( + &self.function_call_graph, + cf, + &mut called_by_canonical_functions, + ) + } + + let can_funcs: HashSet = called_by_canonical_functions + .union(&self.canonical_funcs_def_id) + .cloned() + .collect(); + + // Get the functions that are called directly or indirectly by the token interface functions and, at the same time, are storage setters that do not emit an event. + let unsafe_set_storage: HashSet = can_funcs + .intersection(&self.eventless_storage_changers) + .cloned() + .collect(); + + // Emit the alerts for the considered "unsafe" functions. + for func in self.function_call_graph.keys() { + // Only take into account those functions that are public and exposed in a soroban contract (entrypoints that can be called externally). We do not advise on functions that are used auxiliarily. + if is_soroban_function(cx, &self.checked_functions, func) + || (self.impl_token_interface_trait && self.canonical_funcs_def_id.contains(func)) + { + // Verify if the function itself or the ones it calls (directly or indirectly) emit an event at any point of the flow. + let emits_event_in_flow = check_events_children( + &self.function_call_graph, + func, + &self.defids_with_events, + ); + + // Verify if the function itself or the ones it calls (directly or indirectly) call an unsafe storage setter at any point of the flow. + let calls_unsafe_storage_setter = check_storage_setters_calls( + &self.function_call_graph, + func, + &unsafe_set_storage, + ); + + // If both conditions are met, emit an warning. + if !emits_event_in_flow && calls_unsafe_storage_setter { + span_lint_and_help( + cx, + VERIFY_TRANSFER, + cx.tcx.hir().span_if_local(*func).unwrap(), + LINT_MESSAGE, + /* cx.tcx.hir().span_if_local(r) */ 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 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(fn_name.clone(), fn_decl.inputs, fn_decl.output) { + self.canonical_funcs_def_id.insert(def_id); + } + let mut verify_transfer_visitor = TokenInterfaceEventsVisitor { + cx, + is_storage_changer: false, + emits_event: false, + }; + + verify_transfer_visitor.visit_body(body); + + // If the function modifies the storage and does not emit event, we keep record of its defid as an eventless storage changer. + if verify_transfer_visitor.is_storage_changer && !verify_transfer_visitor.emits_event { + self.eventless_storage_changers.insert(def_id); + } + + // If the function emits an event, we storage its defid. + if verify_transfer_visitor.emits_event { + self.defids_with_events.insert(def_id); + } + } +} + +struct TokenInterfaceEventsVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + is_storage_changer: bool, + emits_event: bool, +} + +impl<'a, 'tcx> Visitor<'tcx> for TokenInterfaceEventsVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(path, receiver, _, _) = expr.kind { + let name = path.ident.name.as_str(); + + let receiver_type = self.cx.typeck_results().node_type(receiver.hir_id); + + // verify if it is an event emission + if name == "events" { + self.emits_event = true; + } + + // verify if it is a storage change + if (name == "set" || name == "update" || name == "remove") + && (receiver_type.to_string() == "soroban_sdk::storage::Instance" + || receiver_type.to_string() == "soroban_sdk::storage::Persistent" + || receiver_type.to_string() == "soroban_sdk::storage::Temporary") + { + self.is_storage_changer = true; + } + } + walk_expr(self, expr); + } +} diff --git a/test-cases/token-interface-events/Cargo.toml b/test-cases/token-interface-events/Cargo.toml new file mode 100644 index 00000000..2810766c --- /dev/null +++ b/test-cases/token-interface-events/Cargo.toml @@ -0,0 +1,28 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["token-interface-events-*/*"] +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" + + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..188c4ef9 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml @@ -0,0 +1,39 @@ +[package] +name = "token-interface-events-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 + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..0fcdac77 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-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: amount, + expiration_ledger: expiration_ledger.clone(), + }, + ); + + 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-events/token-interface-events-1/vulnerable-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..eaa34e51 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml @@ -0,0 +1,39 @@ +[package] +name = "token-interface-events-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 + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..78939a1d --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs @@ -0,0 +1,201 @@ +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, token, Address, Env, String, +}; + +use soroban_sdk::token::TokenInterface; + +#[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, spender), + &AllowanceFromSpender { + 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), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to), &(to_balance + 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, spender), &allowance); + } + + 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)); + } + + 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, spender), &allowance); + } + 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 + } +} + +#[cfg(test)] +mod test; diff --git a/test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..64f6dfc5 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml @@ -0,0 +1,39 @@ +[package] +name = "token-interface-events-remediated-2" +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 + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..ec1d2e07 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs @@ -0,0 +1,233 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; +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() + } + + fn decrease_balance(env: Env, to: Address, amount: i128) { + let current_balance = Self::balance(env.clone(), to.clone()); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(current_balance - amount)); + } + + 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 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); + } + + pub fn balance(env: Env, id: Address) -> i128 { + env.storage() + .instance() + .get(&DataKey::Balance(id)) + .unwrap_or(0) + } + + 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.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + 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.clone(), spender.clone()), + &allowance, + ); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + 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); + TokenUtils::new(&env) + .events() + .burn(from.clone(), amount.clone()); + Self::decrease_balance(env, from, amount); + } + + pub 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.clone()), + &allowance, + ); + + TokenUtils::new(&env).events().burn(from, amount); + } + pub fn decimals(env: Env) -> u32 { + Self::get_metadata(env).decimals + } + pub fn name(env: Env) -> String { + Self::get_metadata(env).name + } + pub fn symbol(env: Env) -> String { + Self::get_metadata(env).symbol + } + + pub fn transfer_to_admin(env: Env, from: Address, amount: i128) { + from.require_auth(); + let balance_from = Self::balance(env.clone(), from.clone()); + assert!(amount <= balance_from); + let admin = Self::get_metadata(env.clone()).admin; + let admin_balance = Self::balance(env.clone(), admin.clone()); + env.storage() + .instance() + .set(&DataKey::Balance(admin.clone()), &(admin_balance + amount)); + TokenUtils::new(&env) + .events() + .transfer(from.clone(), admin.clone(), amount); + Self::decrease_balance(env, from, amount); + } +} diff --git a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..fb073c7c --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml @@ -0,0 +1,39 @@ +[package] +name = "token-interface-events-vulnerable-2" +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 + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..1c1ad5a7 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs @@ -0,0 +1,230 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; +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() + } + + fn decrease_balance(env: Env, to: Address, amount: i128) { + let current_balance = Self::balance(env.clone(), to.clone()); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(current_balance - amount)); + } + + 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 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); + } + + pub fn balance(env: Env, id: Address) -> i128 { + env.storage() + .instance() + .get(&DataKey::Balance(id)) + .unwrap_or(0) + } + + 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.clone()), &(from_balance - amount)); + env.storage() + .instance() + .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + 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.clone(), spender.clone()), + &allowance, + ); + + TokenUtils::new(&env).events().transfer(from, to, amount); + } + + 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); + TokenUtils::new(&env) + .events() + .burn(from.clone(), amount.clone()); + Self::decrease_balance(env, from, amount); + } + + pub 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.clone()), + &allowance, + ); + + TokenUtils::new(&env).events().burn(from, amount); + } + pub fn decimals(env: Env) -> u32 { + Self::get_metadata(env).decimals + } + pub fn name(env: Env) -> String { + Self::get_metadata(env).name + } + pub fn symbol(env: Env) -> String { + Self::get_metadata(env).symbol + } + + pub fn transfer_to_admin(env: Env, from: Address, amount: i128) { + from.require_auth(); + let balance_from = Self::balance(env.clone(), from.clone()); + assert!(amount <= balance_from); + let admin = Self::get_metadata(env.clone()).admin; + let admin_balance = Self::balance(env.clone(), admin.clone()); + env.storage() + .instance() + .set(&DataKey::Balance(admin), &(admin_balance + amount)); + Self::decrease_balance(env, from, amount); + } +} diff --git a/utils/src/lib.rs b/utils/src/lib.rs index d119cbad..7d1e5cf0 100644 --- a/utils/src/lib.rs +++ b/utils/src/lib.rs @@ -14,3 +14,6 @@ pub use constant_analyzer::*; mod type_utils; pub use type_utils::*; + +mod token_interface_utils; +pub use token_interface_utils::*; diff --git a/utils/src/token_interface_utils/mod.rs b/utils/src/token_interface_utils/mod.rs new file mode 100644 index 00000000..5589c392 --- /dev/null +++ b/utils/src/token_interface_utils/mod.rs @@ -0,0 +1,112 @@ +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_hir::FnRetTy; +use rustc_hir::QPath; +use rustc_hir::Ty; +use rustc_hir::TyKind; + +// Used to check if the parameters for a function match the data types for a specific token interface function. +pub fn check_params( + fn_params: &[Ty], + expected_types: Vec, + fn_return: FnRetTy, + expected_return: Option, +) -> bool { + let mut param_types: Vec = Vec::new(); + for i in fn_params { + if let TyKind::Path(QPath::Resolved(_, path)) = i.kind { + let param_type = path.segments[0].ident.to_string(); + param_types.push(param_type.clone()); + } + } + if expected_return.is_none() { + if let FnRetTy::DefaultReturn(_) = fn_return { + return param_types == expected_types; + } + } else { + if let FnRetTy::Return(ty) = fn_return { + if let TyKind::Path(qpath) = &ty.kind { + if let QPath::Resolved(_, path) = qpath { + if let Some(first_segment) = path.segments.first() { + return first_segment.ident.to_string() == expected_return.unwrap() + && param_types == expected_types; + } + } + } + } + } + false +} + +// Used to verify if a function matches a token interface standard function. +pub fn verify_token_interface_function( + fn_name: String, + fn_params: &[Ty], + fn_return: FnRetTy, +) -> bool { + let function = fn_name.split("::").last().unwrap(); + let (types, expected_return): (Vec, Option) = match function { + "allowance" => ( + ["Env", "Address", "Address"] + .iter() + .map(|&s| s.to_string()) + .collect(), + Some("i128".to_string()), + ), + "approve" => ( + ["Env", "Address", "Address", "i128", "u32"] + .iter() + .map(|&s| s.to_string()) + .collect(), + None, + ), + "balance" => ( + ["Env", "Address"].iter().map(|&s| s.to_string()).collect(), + Some("i128".to_string()), + ), + "transfer" => ( + ["Env", "Address", "Address", "i128"] + .iter() + .map(|&s| s.to_string()) + .collect(), + None, + ), + "transfer_from" => ( + ["Env", "Address", "Address", "Address", "i128"] + .iter() + .map(|&s| s.to_string()) + .collect(), + None, + ), + "burn" => ( + ["Env", "Address", "i128"] + .iter() + .map(|&s| s.to_string()) + .collect(), + None, + ), + "burn_from" => ( + ["Env", "Address", "Address", "i128"] + .iter() + .map(|&s| s.to_string()) + .collect(), + None, + ), + "decimals" => ( + ["Env"].iter().map(|&s| s.to_string()).collect(), + Some("u32".to_string()), + ), + "name" => ( + ["Env"].iter().map(|&s| s.to_string()).collect(), + Some("String".to_string()), + ), + "symbol" => ( + ["Env"].iter().map(|&s| s.to_string()).collect(), + Some("String".to_string()), + ), + _ => return false, + }; + check_params(fn_params, types, fn_return, expected_return) +} From 2700617cfe01a55e0140a6eacef0940d51e5c8cf Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Mon, 12 Aug 2024 15:12:54 -0300 Subject: [PATCH 2/8] name change --- detectors/token-interface-events/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/detectors/token-interface-events/src/lib.rs b/detectors/token-interface-events/src/lib.rs index ddef6874..8fa2d115 100644 --- a/detectors/token-interface-events/src/lib.rs +++ b/detectors/token-interface-events/src/lib.rs @@ -26,7 +26,7 @@ const LINT_MESSAGE: &str = const CANONICAL_FUNCTIONS_AMOUNT: usize = 10; dylint_linting::impl_late_lint! { - pub VERIFY_TRANSFER, + pub TOKEN_INTERFACE_EVENTS, Warn, "", TokenInterfaceEvents::default(), @@ -184,7 +184,7 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { if !emits_event_in_flow && calls_unsafe_storage_setter { span_lint_and_help( cx, - VERIFY_TRANSFER, + TOKEN_INTERFACE_EVENTS, cx.tcx.hir().span_if_local(*func).unwrap(), LINT_MESSAGE, /* cx.tcx.hir().span_if_local(r) */ None, @@ -221,21 +221,23 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { if verify_token_interface_function(fn_name.clone(), fn_decl.inputs, fn_decl.output) { self.canonical_funcs_def_id.insert(def_id); } - let mut verify_transfer_visitor = TokenInterfaceEventsVisitor { + let mut token_interface_events_visitor = TokenInterfaceEventsVisitor { cx, is_storage_changer: false, emits_event: false, }; - verify_transfer_visitor.visit_body(body); + token_interface_events_visitor.visit_body(body); // If the function modifies the storage and does not emit event, we keep record of its defid as an eventless storage changer. - if verify_transfer_visitor.is_storage_changer && !verify_transfer_visitor.emits_event { + if token_interface_events_visitor.is_storage_changer + && !token_interface_events_visitor.emits_event + { self.eventless_storage_changers.insert(def_id); } // If the function emits an event, we storage its defid. - if verify_transfer_visitor.emits_event { + if token_interface_events_visitor.emits_event { self.defids_with_events.insert(def_id); } } From 2d2d2a7042918acfc360771809b0e91f961a79e6 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Mon, 12 Aug 2024 18:23:58 -0300 Subject: [PATCH 3/8] tests --- .../vulnerable-example/src/lib.rs | 2 +- .../vulnerable-example/src/test.rs | 151 ++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs index 78939a1d..eb218ab1 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs @@ -6,7 +6,7 @@ use soroban_sdk::{ use soroban_sdk::token::TokenInterface; -#[derive(Clone)] +#[derive(Clone, Debug)] #[contracttype] pub struct TokenMetadata { pub decimals: u32, diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs new file mode 100644 index 00000000..403b6341 --- /dev/null +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs @@ -0,0 +1,151 @@ +#![cfg(test)] +extern crate std; + +use super::*; +use soroban_sdk::testutils::{Address as _, Ledger}; +use soroban_sdk::{Address, Env}; + +fn initialize_env<'a>() -> (Env, TokenInterfaceEventsClient<'a>, Address, [Address; 5]) { + let env = Env::default(); + let token_contract = TokenInterfaceEventsClient::new( + &env, + &env.register_contract(None, TokenInterfaceEvents {}), + ); + let admin = Address::generate(&env); + let decimals: u32 = 3; + let name: String = String::from_str(&env, "TestToken"); + let symbol: String = String::from_str(&env, "TTK"); + let users = [ + Address::generate(&env), + Address::generate(&env), + Address::generate(&env), + Address::generate(&env), + Address::generate(&env), + ]; + + token_contract.initialize(&admin, &decimals, &name, &symbol); + (env, token_contract, admin, users) +} +#[test] +fn test_init_token() { + let env = Env::default(); + let token_contract = TokenInterfaceEventsClient::new( + &env, + &env.register_contract(None, TokenInterfaceEvents {}), + ); + let admin = Address::generate(&env); + let decimals: u32 = 9; + let name: String = String::from_str(&env, "TestToken"); + let symbol: String = String::from_str(&env, "TTK"); + + token_contract.initialize(&admin, &decimals, &name, &symbol); + let token_metadata = token_contract.get_metadata(); + assert_eq!(token_metadata.admin, admin); + assert_eq!(token_metadata.decimals, 9); + assert_eq!(token_metadata.name, name); + assert_eq!(token_metadata.symbol, symbol); +} + +#[test] + +fn test_mint_token() { + let (env, token_contract, _admin, users) = initialize_env(); + env.mock_all_auths(); + + let mut balance = token_contract.balance(&users[0]); + assert_eq!(balance, 0); + token_contract.mint(&users[0], &100000); + + balance = token_contract.balance(&users[0]); + assert_eq!(balance, 100000); +} + +#[test] + +fn test_transfer_burn_token() { + let (env, token_contract, _admin, users) = initialize_env(); + env.mock_all_auths(); + token_contract.mint(&users[0], &100_000); + let previous_balance_user_0 = token_contract.balance(&users[0]); + let previous_balance_user_1 = token_contract.balance(&users[1]); + assert_eq!(previous_balance_user_0, 100_000); + assert_eq!(previous_balance_user_1, 0); + + let transfer_amount = 50_000; + token_contract.transfer(&users[0], &users[1], &transfer_amount); + let mut balance_user_0 = token_contract.balance(&users[0]); + let balance_user_1 = token_contract.balance(&users[1]); + assert_eq!(balance_user_0, previous_balance_user_0 - transfer_amount); + assert_eq!(balance_user_1, previous_balance_user_1 + transfer_amount); + + token_contract.burn(&users[0], &10_000); + balance_user_0 = token_contract.balance(&users[0]); + assert_eq!( + balance_user_0, + previous_balance_user_0 - transfer_amount - 10_000 + ); +} + +#[test] + +fn test_allowance() { + let (env, token_contract, _admin, users) = initialize_env(); + env.mock_all_auths(); + token_contract.mint(&users[0], &500_000); // 500 tokens (3 decimals) + let from = users[0].clone(); + let spender = users[1].clone(); + let to = users[2].clone(); + + let mut current_allowance = token_contract.allowance(&from, &spender); + assert_eq!(current_allowance, 0); + let allowance_amount = 100_000; + let expiration_ledger = 300; + token_contract.approve(&from, &spender, &allowance_amount, &expiration_ledger); + current_allowance = token_contract.allowance(&from, &spender); + assert_eq!(current_allowance, 100_000); + + let transfer_amount = 20_000; + // transfer from 20 tokens - sequence is still 0, allowance should be valid + token_contract.transfer_from(&spender, &from, &to, &transfer_amount); + + let mut from_balance = token_contract.balance(&from); + let to_balance = token_contract.balance(&to); + assert_eq!(from_balance, 500_000 - 20_000); + assert_eq!(to_balance, transfer_amount); + + current_allowance = token_contract.allowance(&from, &spender); + + assert_eq!(current_allowance, allowance_amount - transfer_amount); + + token_contract.burn_from(&spender, &from, &10_000); + from_balance = token_contract.balance(&from); + assert_eq!(from_balance, 470_000); + + current_allowance = token_contract.allowance(&from, &spender); + assert_eq!(current_allowance, 70_000); + + // advance time to verify allowance is now invalid + env.ledger().with_mut(|info| { + info.sequence_number += 500; + }); + + current_allowance = token_contract.allowance(&users[0], &users[1]); + assert_eq!(current_allowance, 0); +} + +#[should_panic] +#[test] + +fn test_no_allowance() { + let (env, token_contract, _admin, users) = initialize_env(); + env.mock_all_auths(); + token_contract.mint(&users[0], &500_000); // 500 tokens (3 decimals) + let from = users[0].clone(); + let spender = users[1].clone(); + + token_contract.burn_from(&spender, &from, &100_000); + + let from_balance = token_contract.balance(&from); + + assert_eq!(from_balance, 500_000); +} From f4aa21ec1bc711d0ed30648527377dd99be0e70e Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Fri, 16 Aug 2024 10:38:11 -0300 Subject: [PATCH 4/8] events related detectors and test cases wip --- detectors/storage-change-events/Cargo.toml | 16 ++ detectors/storage-change-events/src/lib.rs | 204 +++++++++++++++ detectors/token-interface-events/src/lib.rs | 134 ++-------- test-cases/storage-change-events/Cargo.toml | 28 +++ .../remediated-example/Cargo.toml | 2 +- .../remediated-example/src/lib.rs | 0 .../vulnerable-example/Cargo.toml | 4 +- .../vulnerable-example/src/lib.rs | 62 +++++ .../vulnerable-example/src/lib.rs | 17 +- .../remediated-example/src/lib.rs | 233 ------------------ .../vulnerable-example/src/lib.rs | 230 ----------------- 11 files changed, 356 insertions(+), 574 deletions(-) create mode 100644 detectors/storage-change-events/Cargo.toml create mode 100644 detectors/storage-change-events/src/lib.rs create mode 100644 test-cases/storage-change-events/Cargo.toml rename test-cases/{token-interface-events/token-interface-events-2 => storage-change-events/storage-change-events-1}/remediated-example/Cargo.toml (94%) create mode 100644 test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs rename test-cases/{token-interface-events/token-interface-events-2 => storage-change-events/storage-change-events-1}/vulnerable-example/Cargo.toml (90%) create mode 100644 test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs delete mode 100644 test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs delete mode 100644 test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs diff --git a/detectors/storage-change-events/Cargo.toml b/detectors/storage-change-events/Cargo.toml new file mode 100644 index 00000000..97569970 --- /dev/null +++ b/detectors/storage-change-events/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "storage-change-events" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/storage-change-events/src/lib.rs b/detectors/storage-change-events/src/lib.rs new file mode 100644 index 00000000..234d550f --- /dev/null +++ b/detectors/storage-change-events/src/lib.rs @@ -0,0 +1,204 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint_and_help; + +use rustc_hir::{ + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, +}; +use rustc_lint::{LateContext, LateLintPass}; + +use rustc_span::Span; + +use std::collections::HashMap; +use std::collections::HashSet; +use std::vec; +use utils::{is_soroban_function, FunctionCallVisitor}; + +use rustc_span::def_id::DefId; + +const LINT_MESSAGE: &str = "Consider emiting an event when storage is modified"; + +dylint_linting::impl_late_lint! { + pub STORAGE_CHANGE_EVENTS, + Warn, + "", + StorageChangeEvents::default(), + { + name: "Storage Changed without Emiting an Event in Token Interface implementations", + long_message: " It can originate a problem when a canonical function does not emit an event expected by the contract's clients.", + severity: "", + help: "", + vulnerability_class: "", + } +} + +#[derive(Default)] +struct StorageChangeEvents { + function_call_graph: HashMap>, + checked_functions: HashSet, + eventless_storage_changers: HashSet, + defids_with_events: HashSet, +} + +/// Used to verify if, starting from a specific parent in the call graph, an event is emitted at any point of the flow. +/// # Params: +/// - fcg: function call graph +/// - parent: the item from which the analysis starts. +/// - check_against: a HashSet that is used to compare the defids. This HashSet is supposed to contain all the defids of the functions that emit events (collected by the `visit_expr` and `check_func` functions). +fn check_events_children( + fcg: &HashMap>, + parent: &DefId, + check_against: &HashSet, +) -> bool { + if check_against.contains(parent) { + return true; + } + let children = fcg.get(parent); + if children.is_some() { + for c in children.unwrap() { + if check_against.contains(c) || check_events_children(fcg, c, check_against) { + return true; + } + } + } + false +} + +/// Used to verify if, starting from a specific parent in the call graph, a function that sets storage in a considered "unsafe" way is called in any part of its flow. +/// # Params: +/// - fcg: function call graph +/// - func: the defid from which the analysis starts. +/// - unsafe_set_storage: a HashSet that is used to compare the defids. This HashSet is supposed to contain all the defids of the functions that are considered "unsafe storage setters". +fn check_storage_setters_calls( + fcg: &HashMap>, + func: &DefId, + unsafe_set_storage: &HashSet, +) -> bool { + if unsafe_set_storage.contains(func) { + return true; + } + let children = fcg.get(func); + if children.is_some() { + for c in children.unwrap() { + if unsafe_set_storage.contains(c) + || check_storage_setters_calls(fcg, c, unsafe_set_storage) + { + return true; + } + } + } + false +} + +impl<'tcx> LateLintPass<'tcx> for StorageChangeEvents { + fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { + // Emit the alerts for the considered "unsafe" functions. + for func in self.function_call_graph.keys() { + // Only take into account those functions that are public and exposed in a soroban contract (entrypoints that can be called externally). We do not advise on functions that are used auxiliarily. + if is_soroban_function(cx, &self.checked_functions, func) { + // Verify if the function itself or the ones it calls (directly or indirectly) emit an event at any point of the flow. + let emits_event_in_flow = check_events_children( + &self.function_call_graph, + func, + &self.defids_with_events, + ); + + // Verify if the function itself or the ones it calls (directly or indirectly) call an unsafe storage setter at any point of the flow. + let calls_unsafe_storage_setter = check_storage_setters_calls( + &self.function_call_graph, + func, + &self.eventless_storage_changers, + ); + + // If both conditions are met, emit an warning. + if !emits_event_in_flow && calls_unsafe_storage_setter { + span_lint_and_help( + cx, + STORAGE_CHANGE_EVENTS, + cx.tcx.hir().span_if_local(*func).unwrap(), + LINT_MESSAGE, + /* cx.tcx.hir().span_if_local(r) */ 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 mut function_call_visitor = + FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); + function_call_visitor.visit_body(body); + + let mut storage_change_events_visitor = StorageChangeEventsVisitor { + cx, + is_storage_changer: false, + emits_event: false, + }; + + storage_change_events_visitor.visit_body(body); + + // If the function modifies the storage and does not emit event, we keep record of its defid as an eventless storage changer. + if storage_change_events_visitor.is_storage_changer + && !storage_change_events_visitor.emits_event + { + self.eventless_storage_changers.insert(def_id); + } + + // If the function emits an event, we storage its defid. + if storage_change_events_visitor.emits_event { + self.defids_with_events.insert(def_id); + } + } +} + +struct StorageChangeEventsVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + is_storage_changer: bool, + emits_event: bool, +} + +impl<'a, 'tcx> Visitor<'tcx> for StorageChangeEventsVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(path, receiver, _, _) = expr.kind { + let name = path.ident.name.as_str(); + + let receiver_type = self.cx.typeck_results().node_type(receiver.hir_id); + + // verify if it is an event emission + if name == "events" { + self.emits_event = true; + } + + // verify if it is a storage change + if (name == "set" || name == "update" || name == "remove") + && (receiver_type.to_string() == "soroban_sdk::storage::Instance" + || receiver_type.to_string() == "soroban_sdk::storage::Persistent" + || receiver_type.to_string() == "soroban_sdk::storage::Temporary") + { + self.is_storage_changer = true; + } + } + walk_expr(self, expr); + } +} diff --git a/detectors/token-interface-events/src/lib.rs b/detectors/token-interface-events/src/lib.rs index 8fa2d115..08de5878 100644 --- a/detectors/token-interface-events/src/lib.rs +++ b/detectors/token-interface-events/src/lib.rs @@ -17,13 +17,13 @@ use rustc_span::Span; use std::collections::HashMap; use std::collections::HashSet; use std::vec; -use utils::{is_soroban_function, verify_token_interface_function, FunctionCallVisitor}; +use utils::{verify_token_interface_function, FunctionCallVisitor}; use rustc_span::def_id::DefId; -const LINT_MESSAGE: &str = - "Storage change that involves token manipulation is performed without emiting an event"; -const CANONICAL_FUNCTIONS_AMOUNT: usize = 10; +const LINT_MESSAGE: &str = "This function belongs to the Token Interface and should emit an event"; + +//const CANONICAL_FUNCTIONS_AMOUNT: usize = 10; dylint_linting::impl_late_lint! { pub TOKEN_INTERFACE_EVENTS, @@ -43,30 +43,12 @@ dylint_linting::impl_late_lint! { struct TokenInterfaceEvents { function_call_graph: HashMap>, checked_functions: HashSet, - eventless_storage_changers: HashSet, + //eventless_storage_changers: HashSet, defids_with_events: HashSet, canonical_funcs_def_id: HashSet, impl_token_interface_trait: bool, } -/// Used to add to a HashSet all the DefIds of the functions that are called starting from a specific parent in the function call graph. -/// # Params: -/// - fcg: function call graph that is used as reference. -/// - parent: the item from which the analysis starts. -/// - defids: the HashSet where all the defids found in the tree are collected. - -fn check_defids(fcg: &HashMap>, parent: &DefId, defids: &mut HashSet) { - let children = fcg.get(parent); - if children.is_some() { - for c in children.unwrap() { - if !defids.contains(c) { - defids.insert(*c); - check_defids(fcg, c, defids); - } - } - } -} - /// Used to verify if, starting from a specific parent in the call graph, an event is emitted at any point of the flow. /// # Params: /// - fcg: function call graph @@ -91,32 +73,6 @@ fn check_events_children( false } -/// Used to verify if, starting from a specific parent in the call graph, a function that sets storage in a considered "unsafe" way is called in any part of its flow. -/// # Params: -/// - fcg: function call graph -/// - func: the defid from which the analysis starts. -/// - unsafe_set_storage: a HashSet that is used to compare the defids. This HashSet is supposed to contain all the defids of the functions that are considered "unsafe storage setters". -fn check_storage_setters_calls( - fcg: &HashMap>, - func: &DefId, - unsafe_set_storage: &HashSet, -) -> bool { - if unsafe_set_storage.contains(func) { - return true; - } - let children = fcg.get(func); - if children.is_some() { - for c in children.unwrap() { - if unsafe_set_storage.contains(c) - || check_storage_setters_calls(fcg, c, unsafe_set_storage) - { - return true; - } - } - } - false -} - impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) { if let rustc_hir::ItemKind::Impl(impl_block) = item.kind { @@ -132,39 +88,30 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { } fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { - // Verify if the contract implements the token interface. If all of the canonical functions have not been detected, we assume it is not. - if self.canonical_funcs_def_id.len() != CANONICAL_FUNCTIONS_AMOUNT - && !self.impl_token_interface_trait - { + let functions_that_emit_events: [String; 5] = [ + "burn".to_string(), + "approve".to_string(), + "transfer_from".to_string(), + "burn_from".to_string(), + "transfer".to_string(), + ]; + // Verify if the contract implements the token interface trait. + if !self.impl_token_interface_trait { return; } - // Get the defids of every function that is called either directly or indirectly by all of the functions that are a part of the token interface. - let mut called_by_canonical_functions: HashSet = HashSet::new(); - for cf in &self.canonical_funcs_def_id { - check_defids( - &self.function_call_graph, - cf, - &mut called_by_canonical_functions, - ) - } - - let can_funcs: HashSet = called_by_canonical_functions - .union(&self.canonical_funcs_def_id) - .cloned() - .collect(); - - // Get the functions that are called directly or indirectly by the token interface functions and, at the same time, are storage setters that do not emit an event. - let unsafe_set_storage: HashSet = can_funcs - .intersection(&self.eventless_storage_changers) - .cloned() - .collect(); - // Emit the alerts for the considered "unsafe" functions. for func in self.function_call_graph.keys() { // Only take into account those functions that are public and exposed in a soroban contract (entrypoints that can be called externally). We do not advise on functions that are used auxiliarily. - if is_soroban_function(cx, &self.checked_functions, func) - || (self.impl_token_interface_trait && self.canonical_funcs_def_id.contains(func)) + if self.canonical_funcs_def_id.contains(func) + && functions_that_emit_events.contains( + &cx.tcx + .def_path_str(func) + .split("::") + .last() + .unwrap() + .to_string(), + ) { // Verify if the function itself or the ones it calls (directly or indirectly) emit an event at any point of the flow. let emits_event_in_flow = check_events_children( @@ -173,15 +120,8 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { &self.defids_with_events, ); - // Verify if the function itself or the ones it calls (directly or indirectly) call an unsafe storage setter at any point of the flow. - let calls_unsafe_storage_setter = check_storage_setters_calls( - &self.function_call_graph, - func, - &unsafe_set_storage, - ); - // If both conditions are met, emit an warning. - if !emits_event_in_flow && calls_unsafe_storage_setter { + if !emits_event_in_flow { span_lint_and_help( cx, TOKEN_INTERFACE_EVENTS, @@ -222,20 +162,12 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { self.canonical_funcs_def_id.insert(def_id); } let mut token_interface_events_visitor = TokenInterfaceEventsVisitor { - cx, - is_storage_changer: false, + _cx: cx, emits_event: false, }; token_interface_events_visitor.visit_body(body); - // If the function modifies the storage and does not emit event, we keep record of its defid as an eventless storage changer. - if token_interface_events_visitor.is_storage_changer - && !token_interface_events_visitor.emits_event - { - self.eventless_storage_changers.insert(def_id); - } - // If the function emits an event, we storage its defid. if token_interface_events_visitor.emits_event { self.defids_with_events.insert(def_id); @@ -244,31 +176,19 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { } struct TokenInterfaceEventsVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - is_storage_changer: bool, + _cx: &'a LateContext<'tcx>, emits_event: bool, } impl<'a, 'tcx> Visitor<'tcx> for TokenInterfaceEventsVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if let ExprKind::MethodCall(path, receiver, _, _) = expr.kind { + if let ExprKind::MethodCall(path, _receiver, _, _) = expr.kind { let name = path.ident.name.as_str(); - let receiver_type = self.cx.typeck_results().node_type(receiver.hir_id); - // verify if it is an event emission if name == "events" { self.emits_event = true; } - - // verify if it is a storage change - if (name == "set" || name == "update" || name == "remove") - && (receiver_type.to_string() == "soroban_sdk::storage::Instance" - || receiver_type.to_string() == "soroban_sdk::storage::Persistent" - || receiver_type.to_string() == "soroban_sdk::storage::Temporary") - { - self.is_storage_changer = true; - } } walk_expr(self, expr); } diff --git a/test-cases/storage-change-events/Cargo.toml b/test-cases/storage-change-events/Cargo.toml new file mode 100644 index 00000000..1f31ff64 --- /dev/null +++ b/test-cases/storage-change-events/Cargo.toml @@ -0,0 +1,28 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["storage-change-events-*/*"] +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" + + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/storage-change-events" }, +] diff --git a/test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml b/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml similarity index 94% rename from test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml rename to test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml index 64f6dfc5..960dc044 100644 --- a/test-cases/token-interface-events/token-interface-events-2/remediated-example/Cargo.toml +++ b/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "token-interface-events-remediated-2" +name = "storage-change-events-remediated-1" version = "0.1.0" edition = "2021" diff --git a/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs b/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..e69de29b diff --git a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml similarity index 90% rename from test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml rename to test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml index fb073c7c..3101463b 100644 --- a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/Cargo.toml +++ b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "token-interface-events-vulnerable-2" +name = "storage-change-events-vulnerable-1" version = "0.1.0" edition = "2021" @@ -35,5 +35,5 @@ debug-assertions = true [workspace.metadata.dylint] libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, + { path="/home/sofia/Documentos/scout/scout-soroban/detectors/storage-change-events" }, ] diff --git a/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..351d1e37 --- /dev/null +++ b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs @@ -0,0 +1,62 @@ +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Symbol, +}; + +#[derive(Clone, Debug)] +#[contracttype] +pub struct CounterState { + admin: Address, + count: u32, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum SCError { + AlreadyInitialized = 1, + NotInitialized = 2, + FailedToRetrieveState = 3, +} + +#[contract] +pub struct StorageChangeEvents; + +#[contractimpl] +impl StorageChangeEvents { + pub fn initialize(env: Env, admin: Address) -> Result<(), SCError> { + let current_state = Self::get_state(env.clone()); + if current_state.is_ok() { + return Err(SCError::AlreadyInitialized); + } + + env.storage() + .instance() + .set(&STATE, &CounterState { admin, count: 0 }); + + Ok(()) + } + + pub fn increase_counter(env: Env) -> Result<(), SCError> { + let mut counter = Self::get_state(env.clone())?; + + counter.count += 1; + + env.storage().instance().set(&STATE, &counter); + Ok(()) + } + + pub fn get_state(env: Env) -> Result { + let state_op: Option = env.storage().instance().get(&STATE); + if let Some(state) = state_op { + Ok(state) + } else { + Err(SCError::FailedToRetrieveState) + } + } + + fn internal_fn(env: Env) {} +} diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs index eb218ab1..d9322a04 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs @@ -5,6 +5,7 @@ use soroban_sdk::{ }; use soroban_sdk::token::TokenInterface; +use soroban_token_sdk::TokenUtils; #[derive(Clone, Debug)] #[contracttype] @@ -95,6 +96,18 @@ impl TokenInterfaceEvents { .get(&DataKey::AllowanceFromSpender(from, spender)) .unwrap_or_default() } + + fn internal_emit_event( + env: Env, + from: Address, + spender: Address, + amount: i128, + expiration_ledger: u32, + ) { + TokenUtils::new(&env) + .events() + .approve(from, spender, amount, expiration_ledger); + } } #[contractimpl] @@ -112,12 +125,14 @@ impl token::TokenInterface for TokenInterfaceEvents { from.require_auth(); assert!(env.ledger().sequence() < expiration_ledger || amount == 0); env.storage().instance().set( - &DataKey::AllowanceFromSpender(from, spender), + &DataKey::AllowanceFromSpender(from.clone(), spender.clone()), &AllowanceFromSpender { amount, expiration_ledger, }, ); + + Self::internal_emit_event(env, from, spender, amount, expiration_ledger); } fn balance(env: Env, id: Address) -> i128 { diff --git a/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs deleted file mode 100644 index ec1d2e07..00000000 --- a/test-cases/token-interface-events/token-interface-events-2/remediated-example/src/lib.rs +++ /dev/null @@ -1,233 +0,0 @@ -#![no_std] - -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; -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() - } - - fn decrease_balance(env: Env, to: Address, amount: i128) { - let current_balance = Self::balance(env.clone(), to.clone()); - env.storage() - .instance() - .set(&DataKey::Balance(to.clone()), &(current_balance - amount)); - } - - 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 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); - } - - pub fn balance(env: Env, id: Address) -> i128 { - env.storage() - .instance() - .get(&DataKey::Balance(id)) - .unwrap_or(0) - } - - 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.clone()), &(from_balance - amount)); - env.storage() - .instance() - .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); - - TokenUtils::new(&env).events().transfer(from, to, amount); - } - - 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.clone(), spender.clone()), - &allowance, - ); - - TokenUtils::new(&env).events().transfer(from, to, amount); - } - - 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); - TokenUtils::new(&env) - .events() - .burn(from.clone(), amount.clone()); - Self::decrease_balance(env, from, amount); - } - - pub 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.clone()), - &allowance, - ); - - TokenUtils::new(&env).events().burn(from, amount); - } - pub fn decimals(env: Env) -> u32 { - Self::get_metadata(env).decimals - } - pub fn name(env: Env) -> String { - Self::get_metadata(env).name - } - pub fn symbol(env: Env) -> String { - Self::get_metadata(env).symbol - } - - pub fn transfer_to_admin(env: Env, from: Address, amount: i128) { - from.require_auth(); - let balance_from = Self::balance(env.clone(), from.clone()); - assert!(amount <= balance_from); - let admin = Self::get_metadata(env.clone()).admin; - let admin_balance = Self::balance(env.clone(), admin.clone()); - env.storage() - .instance() - .set(&DataKey::Balance(admin.clone()), &(admin_balance + amount)); - TokenUtils::new(&env) - .events() - .transfer(from.clone(), admin.clone(), amount); - Self::decrease_balance(env, from, amount); - } -} diff --git a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs deleted file mode 100644 index 1c1ad5a7..00000000 --- a/test-cases/token-interface-events/token-interface-events-2/vulnerable-example/src/lib.rs +++ /dev/null @@ -1,230 +0,0 @@ -#![no_std] - -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; -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() - } - - fn decrease_balance(env: Env, to: Address, amount: i128) { - let current_balance = Self::balance(env.clone(), to.clone()); - env.storage() - .instance() - .set(&DataKey::Balance(to.clone()), &(current_balance - amount)); - } - - 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 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); - } - - pub fn balance(env: Env, id: Address) -> i128 { - env.storage() - .instance() - .get(&DataKey::Balance(id)) - .unwrap_or(0) - } - - 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.clone()), &(from_balance - amount)); - env.storage() - .instance() - .set(&DataKey::Balance(to.clone()), &(to_balance + amount)); - - TokenUtils::new(&env).events().transfer(from, to, amount); - } - - 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.clone(), spender.clone()), - &allowance, - ); - - TokenUtils::new(&env).events().transfer(from, to, amount); - } - - 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); - TokenUtils::new(&env) - .events() - .burn(from.clone(), amount.clone()); - Self::decrease_balance(env, from, amount); - } - - pub 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.clone()), - &allowance, - ); - - TokenUtils::new(&env).events().burn(from, amount); - } - pub fn decimals(env: Env) -> u32 { - Self::get_metadata(env).decimals - } - pub fn name(env: Env) -> String { - Self::get_metadata(env).name - } - pub fn symbol(env: Env) -> String { - Self::get_metadata(env).symbol - } - - pub fn transfer_to_admin(env: Env, from: Address, amount: i128) { - from.require_auth(); - let balance_from = Self::balance(env.clone(), from.clone()); - assert!(amount <= balance_from); - let admin = Self::get_metadata(env.clone()).admin; - let admin_balance = Self::balance(env.clone(), admin.clone()); - env.storage() - .instance() - .set(&DataKey::Balance(admin), &(admin_balance + amount)); - Self::decrease_balance(env, from, amount); - } -} From 67d9d4b03de78af3d64d474569410503e72af9ca Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 20 Aug 2024 15:06:51 -0300 Subject: [PATCH 5/8] storage change events and token interface events detectors and test cases first version --- detectors/storage-change-events/src/lib.rs | 2 +- detectors/token-interface-events/src/lib.rs | 2 - test-cases/storage-change-events/Cargo.toml | 5 -- .../remediated-example/Cargo.toml | 4 - .../remediated-example/src/lib.rs | 82 +++++++++++++++++++ .../vulnerable-example/Cargo.toml | 4 - .../vulnerable-example/src/lib.rs | 18 +++- test-cases/token-interface-events/Cargo.toml | 5 -- .../remediated-example/Cargo.toml | 4 - .../remediated-example/src/lib.rs | 34 +++++--- .../vulnerable-example/Cargo.toml | 4 - .../vulnerable-example/src/lib.rs | 14 ---- 12 files changed, 121 insertions(+), 57 deletions(-) diff --git a/detectors/storage-change-events/src/lib.rs b/detectors/storage-change-events/src/lib.rs index 234d550f..6002efce 100644 --- a/detectors/storage-change-events/src/lib.rs +++ b/detectors/storage-change-events/src/lib.rs @@ -191,7 +191,7 @@ impl<'a, 'tcx> Visitor<'tcx> for StorageChangeEventsVisitor<'a, 'tcx> { } // verify if it is a storage change - if (name == "set" || name == "update" || name == "remove") + if (name == "set" || name == "update" || name == "remove" || name == "try_update") && (receiver_type.to_string() == "soroban_sdk::storage::Instance" || receiver_type.to_string() == "soroban_sdk::storage::Persistent" || receiver_type.to_string() == "soroban_sdk::storage::Temporary") diff --git a/detectors/token-interface-events/src/lib.rs b/detectors/token-interface-events/src/lib.rs index 08de5878..c811c5b1 100644 --- a/detectors/token-interface-events/src/lib.rs +++ b/detectors/token-interface-events/src/lib.rs @@ -23,8 +23,6 @@ use rustc_span::def_id::DefId; const LINT_MESSAGE: &str = "This function belongs to the Token Interface and should emit an event"; -//const CANONICAL_FUNCTIONS_AMOUNT: usize = 10; - dylint_linting::impl_late_lint! { pub TOKEN_INTERFACE_EVENTS, Warn, diff --git a/test-cases/storage-change-events/Cargo.toml b/test-cases/storage-change-events/Cargo.toml index 1f31ff64..c2fabcfe 100644 --- a/test-cases/storage-change-events/Cargo.toml +++ b/test-cases/storage-change-events/Cargo.toml @@ -21,8 +21,3 @@ strip = "symbols" debug-assertions = true inherits = "release" - -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/storage-change-events" }, -] diff --git a/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml b/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml index 960dc044..dd120b7f 100644 --- a/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml +++ b/test-cases/storage-change-events/storage-change-events-1/remediated-example/Cargo.toml @@ -33,7 +33,3 @@ lto = true inherits = "release" debug-assertions = true -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, -] diff --git a/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs b/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs index e69de29b..dd92fafe 100644 --- a/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs +++ b/test-cases/storage-change-events/storage-change-events-1/remediated-example/src/lib.rs @@ -0,0 +1,82 @@ +#![no_std] + +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Symbol, +}; + +#[derive(Clone, Debug)] +#[contracttype] +pub struct CounterState { + admin: Address, + count: u32, +} + +const STATE: Symbol = symbol_short!("STATE"); +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum SCError { + AlreadyInitialized = 1, + NotInitialized = 2, + FailedToRetrieveState = 3, +} + +#[contract] +pub struct StorageChangeEvents; + +#[contractimpl] +impl StorageChangeEvents { + pub fn initialize(env: Env, admin: Address) -> Result<(), SCError> { + let current_state = Self::get_state(env.clone()); + if current_state.is_ok() { + return Err(SCError::AlreadyInitialized); + } + + env.storage().instance().set( + &STATE, + &CounterState { + admin: admin.clone(), + count: 0, + }, + ); + + env.events() + .publish((COUNTER, symbol_short!("init")), admin); + Ok(()) + } + + pub fn increase_counter(env: Env) -> Result<(), SCError> { + let mut counter = Self::get_state(env.clone())?; + counter.count += 1; + env.storage().instance().set(&STATE, &counter); + env.events() + .publish((COUNTER, symbol_short!("increase")), counter.count); + Ok(()) + } + + pub fn set_counter_indirectly(env: Env, number: u32) -> Result<(), SCError> { + let mut counter = Self::get_state(env.clone())?; + counter.admin.require_auth(); + counter.count = number; + Self::set_counter(env, counter); + + Ok(()) + } + + fn set_counter(env: Env, counter: CounterState) { + env.storage().instance().set(&STATE, &counter); + env.events() + .publish((COUNTER, symbol_short!("set")), counter.count); + } + + pub fn get_state(env: Env) -> Result { + let state_op: Option = env.storage().instance().get(&STATE); + if let Some(state) = state_op { + Ok(state) + } else { + Err(SCError::FailedToRetrieveState) + } + } +} diff --git a/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml index 3101463b..e459c2e3 100644 --- a/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml +++ b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/Cargo.toml @@ -33,7 +33,3 @@ lto = true inherits = "release" debug-assertions = true -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/storage-change-events" }, -] diff --git a/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs index 351d1e37..97079247 100644 --- a/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs +++ b/test-cases/storage-change-events/storage-change-events-1/vulnerable-example/src/lib.rs @@ -42,13 +42,25 @@ impl StorageChangeEvents { pub fn increase_counter(env: Env) -> Result<(), SCError> { let mut counter = Self::get_state(env.clone())?; - counter.count += 1; - env.storage().instance().set(&STATE, &counter); + + Ok(()) + } + + pub fn set_counter_indirectly(env: Env, number: u32) -> Result<(), SCError> { + let mut counter = Self::get_state(env.clone())?; + counter.admin.require_auth(); + counter.count = number; + Self::set_counter(env, counter); + Ok(()) } + fn set_counter(env: Env, counter: CounterState) { + env.storage().instance().set(&STATE, &counter); + } + pub fn get_state(env: Env) -> Result { let state_op: Option = env.storage().instance().get(&STATE); if let Some(state) = state_op { @@ -57,6 +69,4 @@ impl StorageChangeEvents { Err(SCError::FailedToRetrieveState) } } - - fn internal_fn(env: Env) {} } diff --git a/test-cases/token-interface-events/Cargo.toml b/test-cases/token-interface-events/Cargo.toml index 2810766c..257b9d87 100644 --- a/test-cases/token-interface-events/Cargo.toml +++ b/test-cases/token-interface-events/Cargo.toml @@ -21,8 +21,3 @@ strip = "symbols" debug-assertions = true inherits = "release" - -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, -] diff --git a/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml index 188c4ef9..cd132465 100644 --- a/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml +++ b/test-cases/token-interface-events/token-interface-events-1/remediated-example/Cargo.toml @@ -33,7 +33,3 @@ lto = true inherits = "release" debug-assertions = true -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, -] diff --git a/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs index 0fcdac77..99c947da 100644 --- a/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs @@ -96,6 +96,25 @@ impl TokenInterfaceEvents { .get(&DataKey::AllowanceFromSpender(from, spender)) .unwrap_or_default() } + + fn set_allowance( + env: Env, + from: Address, + spender: Address, + amount: i128, + expiration_ledger: u32, + ) { + env.storage().instance().set( + &DataKey::AllowanceFromSpender(from.clone(), spender.clone()), + &AllowanceFromSpender { + amount: amount, + expiration_ledger: expiration_ledger.clone(), + }, + ); + TokenUtils::new(&env) + .events() + .approve(from, spender, amount, expiration_ledger); + } } #[contractimpl] @@ -112,17 +131,9 @@ impl token::TokenInterface for TokenInterfaceEvents { 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: amount, - expiration_ledger: expiration_ledger.clone(), - }, - ); - TokenUtils::new(&env) - .events() - .approve(from, spender, amount, expiration_ledger); + // This function emits the event, so the warning will not come up + Self::set_allowance(env, from, spender, amount, expiration_ledger); } fn balance(env: Env, id: Address) -> i128 { @@ -199,12 +210,15 @@ impl token::TokenInterface for TokenInterfaceEvents { ); 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-events/token-interface-events-1/vulnerable-example/Cargo.toml b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml index eaa34e51..8fd75f45 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/Cargo.toml @@ -33,7 +33,3 @@ lto = true inherits = "release" debug-assertions = true -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofia/Documentos/scout/scout-soroban/detectors/token-interface-events" }, -] diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs index d9322a04..cdd84d32 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs @@ -96,18 +96,6 @@ impl TokenInterfaceEvents { .get(&DataKey::AllowanceFromSpender(from, spender)) .unwrap_or_default() } - - fn internal_emit_event( - env: Env, - from: Address, - spender: Address, - amount: i128, - expiration_ledger: u32, - ) { - TokenUtils::new(&env) - .events() - .approve(from, spender, amount, expiration_ledger); - } } #[contractimpl] @@ -131,8 +119,6 @@ impl token::TokenInterface for TokenInterfaceEvents { expiration_ledger, }, ); - - Self::internal_emit_event(env, from, spender, amount, expiration_ledger); } fn balance(env: Env, id: Address) -> i128 { From 52225376e0efb4e58a2748338256e92b38fa20dc Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 20 Aug 2024 15:12:13 -0300 Subject: [PATCH 6/8] remove previous long message --- detectors/storage-change-events/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/detectors/storage-change-events/src/lib.rs b/detectors/storage-change-events/src/lib.rs index 6002efce..b119ff21 100644 --- a/detectors/storage-change-events/src/lib.rs +++ b/detectors/storage-change-events/src/lib.rs @@ -29,8 +29,8 @@ dylint_linting::impl_late_lint! { "", StorageChangeEvents::default(), { - name: "Storage Changed without Emiting an Event in Token Interface implementations", - long_message: " It can originate a problem when a canonical function does not emit an event expected by the contract's clients.", + name: "Storage Changed without Emiting an Event", + long_message: "", severity: "", help: "", vulnerability_class: "", From dac2ca4648237b12567862ebc016ea312585b254 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Wed, 21 Aug 2024 11:38:24 -0300 Subject: [PATCH 7/8] fix clippy lints errors --- .../token-interface-events-1/remediated-example/src/lib.rs | 4 ++-- .../token-interface-events-1/vulnerable-example/src/lib.rs | 1 - .../token-interface-events-1/vulnerable-example/src/test.rs | 3 --- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs index 99c947da..5417416e 100644 --- a/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/remediated-example/src/lib.rs @@ -107,8 +107,8 @@ impl TokenInterfaceEvents { env.storage().instance().set( &DataKey::AllowanceFromSpender(from.clone(), spender.clone()), &AllowanceFromSpender { - amount: amount, - expiration_ledger: expiration_ledger.clone(), + amount, + expiration_ledger, }, ); TokenUtils::new(&env) diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs index cdd84d32..44f752e0 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/lib.rs @@ -5,7 +5,6 @@ use soroban_sdk::{ }; use soroban_sdk::token::TokenInterface; -use soroban_token_sdk::TokenUtils; #[derive(Clone, Debug)] #[contracttype] diff --git a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs index 403b6341..a55f91c4 100644 --- a/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs +++ b/test-cases/token-interface-events/token-interface-events-1/vulnerable-example/src/test.rs @@ -1,6 +1,3 @@ -#![cfg(test)] -extern crate std; - use super::*; use soroban_sdk::testutils::{Address as _, Ledger}; use soroban_sdk::{Address, Env}; From 6bcc04a25452d9d573c670015733703f6e40b57e Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 27 Aug 2024 13:19:38 -0300 Subject: [PATCH 8/8] add mint event inside token interface considerations --- detectors/token-interface-events/src/lib.rs | 3 ++- utils/src/token_interface_utils/mod.rs | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/detectors/token-interface-events/src/lib.rs b/detectors/token-interface-events/src/lib.rs index c811c5b1..69e3d264 100644 --- a/detectors/token-interface-events/src/lib.rs +++ b/detectors/token-interface-events/src/lib.rs @@ -86,12 +86,13 @@ impl<'tcx> LateLintPass<'tcx> for TokenInterfaceEvents { } fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { - let functions_that_emit_events: [String; 5] = [ + let functions_that_emit_events: [String; 6] = [ "burn".to_string(), "approve".to_string(), "transfer_from".to_string(), "burn_from".to_string(), "transfer".to_string(), + "mint".to_string(), ]; // Verify if the contract implements the token interface trait. if !self.impl_token_interface_trait { diff --git a/utils/src/token_interface_utils/mod.rs b/utils/src/token_interface_utils/mod.rs index 5589c392..1cfb03f5 100644 --- a/utils/src/token_interface_utils/mod.rs +++ b/utils/src/token_interface_utils/mod.rs @@ -47,6 +47,9 @@ pub fn verify_token_interface_function( fn_return: FnRetTy, ) -> bool { let function = fn_name.split("::").last().unwrap(); + if function.eq("mint") { + return true; + } let (types, expected_return): (Vec, Option) = match function { "allowance" => ( ["Env", "Address", "Address"]