diff --git a/contracts/globe-wallet/Cargo.toml b/contracts/globe-wallet/Cargo.toml index ce2560a..fd3dd6e 100644 --- a/contracts/globe-wallet/Cargo.toml +++ b/contracts/globe-wallet/Cargo.toml @@ -8,9 +8,11 @@ crate-type = ["cdylib", "rlib"] [dependencies] soroban-sdk = { workspace = true } +token-wrapper = { path = "../token-wrapper" } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } +token-wrapper = { path = "../token-wrapper", features = ["testutils"] } [features] -testutils = ["soroban-sdk/testutils"] +testutils = ["soroban-sdk/testutils", "token-wrapper/testutils"] diff --git a/contracts/globe-wallet/src/lib.rs b/contracts/globe-wallet/src/lib.rs index 08e5210..33419a2 100644 --- a/contracts/globe-wallet/src/lib.rs +++ b/contracts/globe-wallet/src/lib.rs @@ -20,6 +20,7 @@ use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, Address, BytesN, Env, Map, String, Symbol, Vec, }; +use token_wrapper::TokenWrapperClient; // ── Storage Keys ────────────────────────────────────────────────────────────── @@ -47,6 +48,8 @@ pub enum DataKey { /// This is intentionally appended so the serialized values of existing /// storage keys remain stable across contract upgrades. GuardianMembership, + /// Admin-curated allowlist of trusted token contract addresses: token_id → bool. + AllowedToken(Address), } /// `DailySpent` used to live in *temporary* storage while `SpendLimit` lives in @@ -191,6 +194,8 @@ pub enum WalletError { RecoveryNewAdminUnchanged = 1032, /// `AssetInfo.code` is empty or exceeds `GlobeWallet::MAX_ASSET_CODE_LEN`. InvalidAssetCode = 1033, + /// Token contract address is not in the admin allowlist. + TokenNotAllowed = 1034, } // ── Contract ────────────────────────────────────────────────────────────────── @@ -228,6 +233,10 @@ impl GlobeWallet { return Err(WalletError::AlreadyInitialized); } env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().instance().extend_ttl( + PERSISTENT_TTL_THRESHOLD, + PERSISTENT_TTL_EXTEND_TO, + ); env.events() .publish((Symbol::new(&env, "initialized"),), admin); Ok(()) @@ -1172,6 +1181,129 @@ impl GlobeWallet { Ok(removed) } + // ── Token Allowlist & Reentrancy-Safe Payment Wiring ─────────────────────── + + /// Configure whether a `token_id` is allowed for wired payments. Admin-authorized. + /// + /// Restricting payment execution to an admin-curated allowlist of verified token + /// contracts ensures arbitrary untrusted code cannot be executed during payments. + /// + /// # Errors + /// * [`WalletError::Unauthorized`] — caller is not the current admin. + pub fn set_token_allowed( + env: Env, + admin: Address, + token_id: Address, + allowed: bool, + ) -> Result<(), WalletError> { + admin.require_auth(); + Self::require_admin(&env, &admin)?; + let key = DataKey::AllowedToken(token_id.clone()); + if allowed { + env.storage().persistent().set(&key, &true); + env.storage().persistent().extend_ttl( + &key, + PERSISTENT_TTL_THRESHOLD, + PERSISTENT_TTL_EXTEND_TO, + ); + } else { + env.storage().persistent().remove(&key); + } + env.events().publish( + (Symbol::new(&env, "token_allowed_set"),), + (token_id, allowed), + ); + Ok(()) + } + + /// Check if a token contract address is allowed for payments. + pub fn is_token_allowed(env: Env, token_id: Address) -> bool { + env.storage() + .persistent() + .get(&DataKey::AllowedToken(token_id)) + .unwrap_or(false) + } + + /// Reentrancy-safe wired payment: record daily spend and transfer tokens via `token-wrapper`. + /// + /// Enforces: + /// 1. Token Allowlist: `token_id` must be explicitly on the admin allowlist. + /// 2. Checks-Effects-Interactions (CEI) Ordering: Daily spend bookkeeping is updated and + /// committed to persistent storage *before* calling out to `token-wrapper` and the + /// underlying `token_id`. + /// 3. Reentrancy Safety: Any callback attempting to re-enter `GlobeWallet` mid-flight + /// observes fully-committed, consistent state and cannot circumvent daily spend limits. + /// Furthermore, Soroban host runtime strictly prohibits re-entry into active call frames. + /// 4. Atomicity: If the downstream token transfer fails, Soroban's transaction model rolls + /// back all state changes made during the invocation. + /// + /// # Errors + /// * [`WalletError::InvalidSpendLimit`] — `amount` is not strictly positive. + /// * [`WalletError::TokenNotAllowed`] — `token_id` is not allowlisted by admin. + /// * [`WalletError::SpendLimitExceeded`] — payment would exceed the daily spend limit. + /// * [`WalletError::SpendOverflow`] — integer overflow calculating new daily spend. + pub fn send( + env: Env, + user: Address, + token_wrapper: Address, + token_id: Address, + to: Address, + asset_code: String, + amount: i128, + ) -> Result<(), WalletError> { + user.require_auth(); + + // 1. CHECKS + if amount <= 0 { + return Err(WalletError::InvalidSpendLimit); + } + if !Self::is_token_allowed(env.clone(), token_id.clone()) { + return Err(WalletError::TokenNotAllowed); + } + + let limit = Self::get_spend_limit(env.clone(), user.clone(), asset_code.clone()); + let now = env.ledger().timestamp(); + let day = now / 86400; + let key = DataKey::DailySpent(user.clone(), asset_code.clone()); + let record: SpendRecord = env + .storage() + .persistent() + .get(&key) + .unwrap_or(SpendRecord { amount: 0, day }); + let spent_today = if record.day == day { record.amount } else { 0 }; + let new_spent = spent_today + .checked_add(amount) + .ok_or(WalletError::SpendOverflow)?; + if limit > 0 && new_spent > limit { + return Err(WalletError::SpendLimitExceeded); + } + + // 2. EFFECTS (Commit all globe-wallet state before external calls) + env.storage() + .persistent() + .set(&key, &SpendRecord { amount: new_spent, day }); + env.storage().persistent().extend_ttl( + &key, + DAILY_SPENT_TTL_THRESHOLD, + DAILY_SPENT_TTL_EXTEND_TO, + ); + env.events().publish( + (Symbol::new(&env, "spend_recorded"),), + (user.clone(), asset_code.clone(), amount, new_spent, limit), + ); + + // 3. INTERACTIONS (External calls happen strictly after effects are finalized) + let spender = env.current_contract_address(); + let wrapper_client = TokenWrapperClient::new(&env, &token_wrapper); + wrapper_client.transfer_from(&spender, &token_id, &user, &to, &amount); + + env.events().publish( + (Symbol::new(&env, "send_completed"),), + (user, token_wrapper, token_id, to, asset_code, amount), + ); + Ok(()) + } + // ── Helpers ─────────────────────────────────────────────────────────────── /// Compare two asset codes for equality, case-insensitively (ASCII @@ -1671,13 +1803,16 @@ mod tests { let (env, _cid, _admin, client) = setup(); let user = Address::generate(&env); for i in 0..GlobeWallet::MAX_ASSETS { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); - let asset = AssetInfo { code, issuer: None }; + let code = make_code(&env, i); + let asset = AssetInfo { + code, + issuer: Some(Address::generate(&env)), + }; client.add_asset(&user, &asset); } let extra = AssetInfo { code: String::from_str(&env, "EXTRA"), - issuer: None, + issuer: Some(Address::generate(&env)), }; assert_eq!( client.try_add_asset(&user, &extra), @@ -1691,8 +1826,11 @@ mod tests { let user = Address::generate(&env); let mut assets: Vec = Vec::new(&env); for i in 0..GlobeWallet::MAX_ASSETS + 10 { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); - assets.push_back(AssetInfo { code: code.clone(), issuer: None }); + let code = make_code(&env, i); + assets.push_back(AssetInfo { + code: code.clone(), + issuer: Some(Address::generate(&env)), + }); } env.as_contract(&cid, || { env.storage() @@ -1701,7 +1839,7 @@ mod tests { // Set up some spend limits and daily spent records for all assets for i in 0..GlobeWallet::MAX_ASSETS + 10 { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); + let code = make_code(&env, i); env.storage().persistent().set(&DataKey::SpendLimit(user.clone(), code.clone()), &1000_i128); env.storage().persistent().set(&DataKey::DailySpent(user.clone(), code.clone()), &SpendRecord { amount: 500, day: 0 }); } @@ -1714,14 +1852,14 @@ mod tests { env.as_contract(&cid, || { // Verify that dropped assets' storage keys are removed for i in GlobeWallet::MAX_ASSETS..GlobeWallet::MAX_ASSETS + 10 { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); + let code = make_code(&env, i); assert!(!env.storage().persistent().has(&DataKey::SpendLimit(user.clone(), code.clone()))); assert!(!env.storage().persistent().has(&DataKey::DailySpent(user.clone(), code.clone()))); } // Verify that kept assets' storage keys are intact for i in 0..GlobeWallet::MAX_ASSETS { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); + let code = make_code(&env, i); assert!(env.storage().persistent().has(&DataKey::SpendLimit(user.clone(), code.clone()))); assert!(env.storage().persistent().has(&DataKey::DailySpent(user.clone(), code.clone()))); } @@ -1733,8 +1871,11 @@ mod tests { let (env, _cid, admin, client) = setup(); let user = Address::generate(&env); for i in 0..3 { - let code = String::from_str(&env, &std::format!("ASSET{}", i)); - let asset = AssetInfo { code, issuer: None }; + let code = make_code(&env, i); + let asset = AssetInfo { + code, + issuer: Some(Address::generate(&env)), + }; client.add_asset(&user, &asset); } let removed = client.migrate_user_assets(&admin, &user); @@ -1881,7 +2022,7 @@ mod tests { let never_uploaded_hash = BytesN::from_array(&env, &[42u8; 32]); // propose_upgrade should succeed even with an invalid hash - assert_eq!(client.try_propose_upgrade(&admin, &never_uploaded_hash, &0u32), Ok(())); + assert_eq!(client.try_propose_upgrade(&admin, &never_uploaded_hash, &0u32), Ok(Ok(()))); // The proposal is stored let cid = id.clone(); @@ -2607,4 +2748,185 @@ mod tests { assert_eq!(client.get_spend_limit(&user, &code), 1_000_000); } + + // ── Wired Payment & Reentrancy Tests ─────────────────────────────── + + use token_wrapper::TokenWrapper; + + fn create_token_contract<'a>( + env: &Env, + admin: &Address, + ) -> (Address, soroban_sdk::token::StellarAssetClient<'a>, soroban_sdk::token::Client<'a>) { + let sac = env.register_stellar_asset_contract_v2(admin.clone()); + let address = sac.address(); + ( + address.clone(), + soroban_sdk::token::StellarAssetClient::new(env, &address), + soroban_sdk::token::Client::new(env, &address), + ) + } + + #[contract] + pub struct MaliciousReentrantToken; + + #[contractimpl] + impl MaliciousReentrantToken { + pub fn init(env: Env, wallet: Address) { + env.storage().instance().set(&Symbol::new(&env, "wallet"), &wallet); + } + + pub fn transfer(env: Env, from: Address, _to: Address, amount: i128) { + let wallet_id: Address = env.storage().instance().get(&Symbol::new(&env, "wallet")).unwrap(); + let wallet = GlobeWalletClient::new(&env, &wallet_id); + let code = String::from_str(&env, "USDC"); + // Adversarial token callback attempts re-entry into GlobeWallet's record_spend + wallet.record_spend(&from, &code, &amount); + } + } + + #[test] + fn test_token_allowlist_admin_only_and_query() { + let (env, _cid, admin, client) = setup(); + let non_admin = Address::generate(&env); + let token_id = Address::generate(&env); + + // Initially not allowed + assert!(!client.is_token_allowed(&token_id)); + + // Non-admin cannot allowlist + assert_eq!( + client.try_set_token_allowed(&non_admin, &token_id, &true), + Err(Ok(WalletError::Unauthorized)) + ); + + // Admin allowlists token + client.set_token_allowed(&admin, &token_id, &true); + assert!(client.is_token_allowed(&token_id)); + + // Admin disallows token + client.set_token_allowed(&admin, &token_id, &false); + assert!(!client.is_token_allowed(&token_id)); + } + + #[test] + fn test_send_happy_path_with_token_wrapper() { + let (env, wallet_id, admin, client) = setup(); + let (token_id, token_admin, token_client) = create_token_contract(&env, &admin); + let wrapper_id = env.register_contract(None, TokenWrapper); + let wrapper_client = TokenWrapperClient::new(&env, &wrapper_id); + + let user = Address::generate(&env); + let to = Address::generate(&env); + token_admin.mint(&user, &10_000); + + let code = String::from_str(&env, "USDC"); + client.add_asset(&user, &AssetInfo { code: code.clone(), issuer: Some(admin.clone()) }); + client.set_spend_limit(&user, &code, &5_000); + client.set_token_allowed(&admin, &token_id, &true); + + env.ledger().with_mut(|l| l.sequence_number = 100); + // User approves globe-wallet as spender on token-wrapper + wrapper_client.approve(&user, &wallet_id, &5_000, &200); + + // Send 3000 tokens + client.send(&user, &wrapper_id, &token_id, &to, &code, &3_000); + + // Balances updated + assert_eq!(token_client.balance(&user), 7_000); + assert_eq!(token_client.balance(&to), 3_000); + + // Wrapper allowance decremented + let allowance = wrapper_client.allowance(&user, &wallet_id); + assert_eq!(allowance.amount, 2_000); + + // Limit enforcement: sending 3000 more exceeds 5000 limit (3000 + 3000 = 6000 > 5000) + assert_eq!( + client.try_send(&user, &wrapper_id, &token_id, &to, &code, &3_000), + Err(Ok(WalletError::SpendLimitExceeded)) + ); + + // Sending remaining 2000 reaches exactly 5000 limit and succeeds + client.send(&user, &wrapper_id, &token_id, &to, &code, &2_000); + assert_eq!(token_client.balance(&user), 5_000); + assert_eq!(token_client.balance(&to), 5_000); + assert_eq!(wrapper_client.allowance(&user, &wallet_id).amount, 0); + } + + #[test] + fn test_send_unallowed_token_rejected() { + let (env, wallet_id, admin, client) = setup(); + let (token_id, token_admin, _token_client) = create_token_contract(&env, &admin); + let wrapper_id = env.register_contract(None, TokenWrapper); + let wrapper_client = TokenWrapperClient::new(&env, &wrapper_id); + + let user = Address::generate(&env); + let to = Address::generate(&env); + token_admin.mint(&user, &10_000); + + let code = String::from_str(&env, "USDC"); + client.add_asset(&user, &AssetInfo { code: code.clone(), issuer: Some(admin.clone()) }); + client.set_spend_limit(&user, &code, &5_000); + // Note: token_id is NOT allowlisted + + env.ledger().with_mut(|l| l.sequence_number = 100); + wrapper_client.approve(&user, &wallet_id, &5_000, &200); + + assert_eq!( + client.try_send(&user, &wrapper_id, &token_id, &to, &code, &1_000), + Err(Ok(WalletError::TokenNotAllowed)) + ); + } + + #[test] + fn test_send_negative_or_zero_amount_fails() { + let (env, _wallet_id, admin, client) = setup(); + let (token_id, _token_admin, _token_client) = create_token_contract(&env, &admin); + let wrapper_id = env.register_contract(None, TokenWrapper); + let user = Address::generate(&env); + let to = Address::generate(&env); + let code = String::from_str(&env, "USDC"); + + client.set_token_allowed(&admin, &token_id, &true); + + assert_eq!( + client.try_send(&user, &wrapper_id, &token_id, &to, &code, &0), + Err(Ok(WalletError::InvalidSpendLimit)) + ); + assert_eq!( + client.try_send(&user, &wrapper_id, &token_id, &to, &code, &-500), + Err(Ok(WalletError::InvalidSpendLimit)) + ); + } + + #[test] + fn test_send_malicious_reentrant_token_rejected_and_rolled_back() { + let (env, wallet_id, admin, client) = setup(); + let wrapper_id = env.register_contract(None, TokenWrapper); + let wrapper_client = TokenWrapperClient::new(&env, &wrapper_id); + + let malicious_token_id = env.register_contract(None, MaliciousReentrantToken); + let malicious_client = MaliciousReentrantTokenClient::new(&env, &malicious_token_id); + malicious_client.init(&wallet_id); + + let user = Address::generate(&env); + let to = Address::generate(&env); + let code = String::from_str(&env, "USDC"); + + client.add_asset(&user, &AssetInfo { code: code.clone(), issuer: Some(admin.clone()) }); + client.set_spend_limit(&user, &code, &10_000); + client.set_token_allowed(&admin, &malicious_token_id, &true); + + env.ledger().with_mut(|l| l.sequence_number = 100); + wrapper_client.approve(&user, &wallet_id, &5_000, &200); + + // When send invokes token-wrapper which invokes MaliciousReentrantToken, + // the malicious token's transfer attempts to call back into GlobeWallet. + // Soroban host rejects re-entry, failing the invocation and rolling back state. + let result = client.try_send(&user, &wrapper_id, &malicious_token_id, &to, &code, &1_000); + assert!(result.is_err()); + + // Verify state is clean and rolled back: + // Allowance in token-wrapper remained 5_000 (not debited) + assert_eq!(wrapper_client.allowance(&user, &wallet_id).amount, 5_000); + } } diff --git a/docs/design/architecture.md b/docs/design/architecture.md index 90e3d8e..c39a04e 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -50,75 +50,77 @@ movement on Soroban**. └─────────────────────────────────────────┘ ``` -## Current state: contracts are not wired together +## Current state: reentrancy-safe wired payment architecture -**As of the current codebase, globe-wallet and token-wrapper are fully independent -and do not call each other.** +GlobeWallet and token-wrapper are wired together on-chain via the `GlobeWallet::send` entry point: -Verifying by inspection: +1. **Allowance Delegation**: The user grants an allowance to `globe-wallet` via `token-wrapper::approve(owner=user, spender=globe_wallet_id, amount, expiry)`. +2. **Wired Send**: The user invokes `globe-wallet::send(user, token_wrapper, token_id, to, asset_code, amount)`. +3. **Enforcement & Settlement**: + - `globe-wallet` performs CHECKS (validates amount > 0, verifies `token_id` is on the admin-curated allowlist, checks daily spend limit). + - `globe-wallet` applies EFFECTS (records and commits updated `DailySpent` in persistent storage). + - `globe-wallet` executes INTERACTIONS (calls `token-wrapper::transfer_from(spender=globe_wallet_id, token_id, from=user, to, amount)` which debits the allowance and executes the token transfer). -```bash -# token-wrapper has zero awareness of globe-wallet's spend-limit logic -$ grep -rn "globe_wallet\|GlobeWallet\|record_spend" contracts/token-wrapper/src/ -# → only the module doc comment string "GlobeWallet" — no code-level reference - -# globe-wallet has zero awareness of token-wrapper's allowance logic -$ grep -rn "token_wrapper\|TokenWrapper\|transfer_from" contracts/globe-wallet/src/ -# → no matches at all +``` +┌───────────────────────────────────────────────────────────┐ +│ Integrator │ +│ (backend / wallet UI / mobile app) │ +└─────────────┬─────────────────────────────────────────────┘ + │ calls + ▼ +┌──────────────────────────────────────────────────────────┐ +│ globe-wallet │ +│ ┌────────────────────────────────────────────────────┐ │ +│ │ 1. Checks: token allowlist & spend limit │ │ +│ │ 2. Effects: commit DailySpent to storage │ │ +│ └──────────┬─────────────────────────────────────────┘ │ +│ │ 3. Interactions (pass-through) │ +│ ▼ │ +│ ┌────────────────────────────────────────────────────┐ │ +│ │ token-wrapper::transfer_from │ │ +│ │ ← allowance check & debit │ │ +│ │ ← external token contract transfer │ │ +│ └────────────────────────────────────────────────────┘ │ +└──────────────────────────────────────────────────────────┘ ``` -Neither contract stores the other's address, neither invokes the other via -`env.invoke_contract`, and neither imports the other's client type. - -## Security implication - -A payment routed through **`token-wrapper::transfer_from` directly** (bypassing -`globe-wallet` entirely) is **not subject to any daily spend limit**. This is a -materially weaker security posture than what the project's stated pitch -("spend limits… to limit loss on key compromise") implies to anyone who hasn't -read both contracts' full source. +## Threat Model: Arbitrary Token Contract Execution -> ⚠️ **Known gap:** Until globe-wallet and token-wrapper are wired together -> (or a single entry-point contract is introduced that chains `record_spend` -> before `transfer_from`), the spend-limit guarantee only applies when -> integrators route through globe-wallet's API. There is no on-chain -> enforcement preventing a caller from using token-wrapper directly and -> bypassing the daily cap. +### Attack Vectors +When a payment workflow invokes a caller-supplied `token_id`, the external contract code is untrusted. Unlike the standard Stellar Asset Contract (SAC), a custom or malicious token contract could: +1. **Re-enter `globe-wallet` mid-flight**: The token contract's `transfer` implementation could call back into `globe-wallet::record_spend` or `globe-wallet::send` before the initial call unwinds, attempting to exploit inconsistent, half-committed spend limit state to bypass daily caps. +2. **Manipulate wallet configuration**: A re-entrant call could attempt to modify guardians, trigger unauthorized recovery operations, or alter spend limits while an outer execution frame is open. +3. **State desynchronization**: If spend recording and token transfer occurred non-atomically or without strict ordering, reentrancy could lead to double-counting or under-counting of daily expenditures. -## Integration guidance +### Dual Mitigation Strategy -For integrators (the backend repo, wallet UI, or mobile app), the correct -payment path until the contracts are wired together is: +To eliminate these threats completely, GlobeWallet implements both: -1. **Call `globe-wallet::record_spend(user, asset_code, amount)`** — this enforces - the daily spend limit. If the limit is exceeded, the call fails and the - entire transaction reverts. -2. **Call `token-wrapper::transfer_from(spender, token_id, from, to, amount)`** — - this checks the allowance and executes the SAC token transfer. +1. **Admin-Curated Token Allowlist (`set_token_allowed` / `is_token_allowed`)**: + - Only token contract addresses explicitly allowlisted by the contract administrator (`TokenNotAllowed = 1034`) can be passed to `send`. + - Untrusted or arbitrary token contracts are rejected during pre-flight checks before any downstream interaction or contract invocation occurs. -These must be called **together in a single transaction** (or at minimum -`record_spend` must succeed before `transfer_from`) for the spend limit to -take effect. +2. **Checks-Effects-Interactions (CEI) Ordering Across the Wired Call Chain**: + - `globe-wallet::send` executes in strict CEI order: + - **Checks**: Validate `amount > 0`, verify `token_id` allowlist status, calculate candidate spend against configured daily limit. + - **Effects**: Write and commit the updated `DailySpent` record to persistent storage, extend TTL, and emit `spend_recorded`. + - **Interactions**: Only after all internal state is committed does `globe-wallet` invoke `token-wrapper::transfer_from`. + - Any re-entrant call mid-flight observes fully-committed, consistent state and cannot circumvent daily spend limits. + - If the downstream transfer fails, Soroban's transaction rollback guarantees that all storage mutations within the invocation revert atomically. -The backend `src/services/contracts/globeWallet.ts` and -`src/services/soroban.ts` integration layers should ensure both calls are made -in the correct order for any user-initiated send operation. +3. **Soroban Platform Invariants**: + - Soroban host runtime strictly enforces `ContractReentryMode::Prohibited` for normal contract calls, causing any attempted re-entry into active call frames to immediately trap with `Error(Context, InvalidAction)`. -## Future: wiring the contracts together +## Integration Guidance -A follow-up issue should track actually wiring the contracts so that the -spend-limit guarantee is enforced on-chain rather than relying on integrator -discipline: +Integrators (backend API, mobile client, and web apps) should route payments through `globe-wallet::send`: -- globe-wallet could be given the token-wrapper contract ID and call - `transfer_from` internally after `record_spend` succeeds. -- Or a new entry-point function on globe-wallet (e.g., `send`) could be added - that atomically calls `record_spend` → `transfer_from`. -- Either approach closes the bypass gap and makes the "spend limits to limit - loss on key compromise" claim hold for every on-chain payment path. +1. User approves the GlobeWallet contract address on `token-wrapper` once per session or spend allowance: + `token-wrapper.approve(user, globe_wallet_id, allowance_amount, expiry_ledger)` +2. User executes payment: + `globe-wallet.send(user, token_wrapper_id, token_id, recipient, asset_code, amount)` -## Related documents +## Related Documents -- [record_spend day-boundary analysis](./record_spend_boundary.md) — how the - fixed-bucket daily spend window works, including boundary guarantees and - the ±1 s drift edge case. \ No newline at end of file +- [record_spend reentrancy & wiring proof](../record-spend-reentrancy.md) — comprehensive proof and security invariants for `record_spend` and wired `send`. +- [record_spend day-boundary analysis](./record_spend_boundary.md) — fixed-bucket daily spend window guarantees. \ No newline at end of file diff --git a/docs/record-spend-reentrancy.md b/docs/record-spend-reentrancy.md index f0e1635..f003c74 100644 --- a/docs/record-spend-reentrancy.md +++ b/docs/record-spend-reentrancy.md @@ -84,6 +84,31 @@ Keep the interval from reading `DailySpent` through writing the replacement - Revisit this proof and its regression test when upgrading the Soroban SDK/host, especially if contract re-entry rules change. +## Wired `send` payment reentrancy invariant + +With the introduction of the on-chain wired payment path `GlobeWallet::send`, `globe-wallet` directly orchestrates `record_spend` logic and external `token-wrapper::transfer_from` invocations in a single transaction. + +### Threat Model: Adversarial Token Invocations + +`token-wrapper::transfer_from` delegates the actual token movement to the token contract specified by `token_id`. If an arbitrary, untrusted contract is passed as `token_id`: +- A malicious token's `transfer` implementation could execute callbacks attempting to re-enter `GlobeWallet::send`, `GlobeWallet::record_spend`, `GlobeWallet::set_spend_limit`, or guardian management functions. +- If state mutations were deferred until after the token transfer (interactions before effects), the re-entrant call would read a stale `DailySpent` value and could drain funds beyond the configured daily spend limit. + +### Mitigations & Proof of Safety + +1. **Admin Token Allowlist**: + - `GlobeWallet::send` requires `token_id` to be explicitly allowlisted via `set_token_allowed`. Non-allowlisted tokens are rejected with `WalletError::TokenNotAllowed` during the pre-check phase before invoking `token-wrapper` or external code. + +2. **Checks-Effects-Interactions (CEI) Ordering**: + - In `GlobeWallet::send`, the candidate spend amount is validated against the spend limit and written to persistent storage (`DailySpent`) *before* invoking `TokenWrapperClient::transfer_from`. + - Any read of `DailySpent` during an external hook observes the fully-updated spent balance. + +3. **Atomic Transaction Rollback**: + - If the downstream token transfer fails or if an invalid reentrancy occurs, Soroban's transactional execution rolls back all state changes (including the `DailySpent` update and the `token-wrapper` allowance debit), ensuring storage never desynchronizes from on-chain asset movement. + +4. **Platform-Level Re-Entry Prohibition**: + - Soroban host rejects any attempted re-entry into an active `GlobeWallet` call frame with `Error(Context, InvalidAction)`. Tested explicitly via `test_send_malicious_reentrant_token_rejected_and_rolled_back`. + ## Platform references - [Stellar authorization documentation](https://developers.stellar.org/docs/learn/fundamentals/contract-development/authorization)