From 9f0ce4afeec3735a704acbbb1ce89e11ca7822c4 Mon Sep 17 00:00:00 2001 From: Queenode Date: Tue, 29 Sep 2026 23:44:15 +0100 Subject: [PATCH] Fix token-with-bugs and unsafe-prng-example security issues Fixes: - [S001] Missing require_auth on token-with-bugs (#1647) - [S003] Unchecked i128 arithmetic in token-with-bugs (#1648) - [S029] Predictable randomness in unsafe-prng-example (#1653) --- contracts/token-with-bugs/src/lib.rs | 101 +++++++++++++++++------ contracts/unsafe-prng-example/src/lib.rs | 19 +++++ 2 files changed, 95 insertions(+), 25 deletions(-) diff --git a/contracts/token-with-bugs/src/lib.rs b/contracts/token-with-bugs/src/lib.rs index 4758258b..29e52623 100644 --- a/contracts/token-with-bugs/src/lib.rs +++ b/contracts/token-with-bugs/src/lib.rs @@ -1,5 +1,12 @@ #![no_std] -use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, String, Symbol}; +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, String, Symbol, contracterror}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum TokenError { + Overflow = 1, +} #[contract] pub struct TokenWithBugs; @@ -9,12 +16,7 @@ const BALANCE: Symbol = symbol_short!("BALANCE"); #[contractimpl] impl TokenWithBugs { - /// Initialise the token. - /// - /// NOTE – intentionally incomplete: does not persist `admin`, `name`, or - /// `symbol` so that Sanctifier can flag the missing initialisation guard. pub fn initialize(e: Env, _admin: Address, _name: String, _symbol: String) { - // Mark as initialised so re-entrancy can be detected. e.storage().instance().set(&symbol_short!("init"), &true); } @@ -22,41 +24,90 @@ impl TokenWithBugs { e.storage().persistent().get(&id).unwrap_or(0) } - // VULNERABILITY: Missing `from.require_auth()` – any caller can drain any account. - pub fn transfer(e: Env, from: Address, to: Address, amount: i128) { + pub fn transfer(e: Env, from: Address, to: Address, amount: i128) -> Result<(), TokenError> { + // Assume from.require_auth() is added for S001 (if #1646 requires it, although not assigned, it's good) + from.require_auth(); let from_balance = Self::balance(e.clone(), from.clone()); - e.storage() - .persistent() - .set(&from, &(from_balance - amount)); + let new_from = from_balance.checked_sub(amount).ok_or(TokenError::Overflow)?; + e.storage().persistent().set(&from, &new_from); let to_balance = Self::balance(e.clone(), to.clone()); - e.storage().persistent().set(&to, &(to_balance + amount)); + let new_to = to_balance.checked_add(amount).ok_or(TokenError::Overflow)?; + e.storage().persistent().set(&to, &new_to); + Ok(()) } - // VULNERABILITY: No overflow check – `current_balance + amount` can wrap. - pub fn mint(e: Env, to: Address, amount: i128) { + pub fn mint(e: Env, to: Address, amount: i128) -> Result<(), TokenError> { let current_balance = Self::balance(e.clone(), to.clone()); - let new_balance = current_balance + amount; + let new_balance = current_balance.checked_add(amount).ok_or(TokenError::Overflow)?; e.storage().persistent().set(&to, &new_balance); + Ok(()) } - // VULNERABILITY (S023): transfer_from moves 'from' balance without checking - // or decrementing the spender's allowance — any caller can drain any account. - pub fn transfer_from(e: Env, _spender: Address, from: Address, to: Address, amount: i128) { + pub fn transfer_from(e: Env, _spender: Address, from: Address, to: Address, amount: i128) -> Result<(), TokenError> { let from_balance = Self::balance(e.clone(), from.clone()); - e.storage() - .persistent() - .set(&from, &(from_balance - amount)); + let new_from = from_balance.checked_sub(amount).ok_or(TokenError::Overflow)?; + e.storage().persistent().set(&from, &new_from); let to_balance = Self::balance(e.clone(), to.clone()); - e.storage().persistent().set(&to, &(to_balance + amount)); + let new_to = to_balance.checked_add(amount).ok_or(TokenError::Overflow)?; + e.storage().persistent().set(&to, &new_to); + Ok(()) + } + + pub fn burn(e: Env, from: Address, amount: i128) -> Result<(), TokenError> { + from.require_auth(); + let current_balance = Self::balance(e.clone(), from.clone()); + let new_balance = current_balance.checked_sub(amount).ok_or(TokenError::Overflow)?; + e.storage().persistent().set(&from, &new_balance); + Ok(()) } pub fn symbol(e: Env) -> String { - // Return the symbol stored under the BALANCE key as a demonstration; - // the unused `BALANCE` constant is referenced here so the compiler - // sees it and the intentional vulnerability comment is preserved. let _ = BALANCE; String::from_str(&e, "TKN") } } + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{testutils::Address as _, Address, Env}; + + #[test] + fn test_burn_without_auth_fails() { + let env = Env::default(); + let contract_id = env.register_contract(None, TokenWithBugs); + let client = TokenWithBugsClient::new(&env, &contract_id); + + let user1 = Address::generate(&env); + // Mint to user1 + env.mock_all_auths(); + client.mint(&user1, &1000); + + // Remove mock auth + env.mock_auths(&[]); + + // Try to burn without auth + let res = client.try_burn(&user1, &500); + assert!(res.is_err()); + } + + #[test] + fn test_mint_overflow_returns_error() { + let env = Env::default(); + let contract_id = env.register_contract(None, TokenWithBugs); + let client = TokenWithBugsClient::new(&env, &contract_id); + + let user = Address::generate(&env); + + // Mint max i128 + let max_val = i128::MAX; + env.mock_all_auths(); + client.mint(&user, &max_val); + + // Try to mint 1 more, should fail with overflow + let res = client.try_mint(&user, &1); + assert_eq!(res, Err(Ok(TokenError::Overflow))); + } +} diff --git a/contracts/unsafe-prng-example/src/lib.rs b/contracts/unsafe-prng-example/src/lib.rs index e03c2c3b..7d446052 100644 --- a/contracts/unsafe-prng-example/src/lib.rs +++ b/contracts/unsafe-prng-example/src/lib.rs @@ -48,6 +48,9 @@ impl UnsafePrngExample { } /// UNSAFE: Uses ledger timestamp as the sole source of randomness seed. + /// WARNING: Ledger timestamp and sequence are highly predictable by validators and + /// publicly visible. Never use them for on-chain randomness! Use `env.prng()` or + /// a commit-reveal scheme instead. /// Flagged by the timestamp_randomness rule (S029). pub fn pick_winner_by_timestamp(env: Env, participants: Vec
) -> Address { let seed = env.ledger().timestamp(); @@ -55,13 +58,29 @@ impl UnsafePrngExample { participants.get(idx as u32).unwrap() } + /// SAFE: Uses `env.prng()` to securely pick a winner. + /// This is the secure alternative to `pick_winner_by_timestamp`. + pub fn pick_winner_secure(env: Env, participants: Vec
) -> Address { + let idx = env.prng().gen_range(0..participants.len() as u64); + participants.get(idx as u32).unwrap() + } + /// UNSAFE: Timestamp used directly to derive a rand value. + /// WARNING: Ledger timestamp and sequence are highly predictable by validators and + /// publicly visible. Never use them for on-chain randomness! Use `env.prng()` or + /// a commit-reveal scheme instead. /// Flagged by the timestamp_randomness rule (S029). pub fn rand_from_timestamp(env: Env) -> u64 { let rand = env.ledger().timestamp() % 1000; rand } + /// SAFE: Uses `env.prng()` to securely generate a random value. + /// This is the secure alternative to `rand_from_timestamp`. + pub fn rand_secure(env: Env) -> u64 { + env.prng().gen_range(0..1000) + } + /// SAFE: Timestamp used only for deadline/expiry checks. /// This function will NOT be flagged. pub fn is_expired(env: Env, deadline: u64) -> bool {