From 72693cdcb0fe17378ab8821d6b8aa69d0339c60c Mon Sep 17 00:00:00 2001 From: Opulence Chuks Date: Wed, 26 Aug 2026 16:50:14 +0100 Subject: [PATCH 1/2] Refactor: extract pending-admin cleanup to shared helper Factored the 'clear any pending admin proposal for the outgoing admin' logic into a new internal helper set_admin_and_clear_pending to ensure structural enforcement of this invariant. Both existing admin-rotation paths (accept_admin and execute_recovery) now route through this helper. Also added an explicit regression test to assert that PendingAdmin(old_admin) is successfully removed after a normal accept_admin flow. --- contracts/globe-wallet/src/lib.rs | 37 ++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/contracts/globe-wallet/src/lib.rs b/contracts/globe-wallet/src/lib.rs index 1c15e34..64929ff 100644 --- a/contracts/globe-wallet/src/lib.rs +++ b/contracts/globe-wallet/src/lib.rs @@ -242,6 +242,15 @@ impl GlobeWallet { Ok(()) } + /// Internal helper to guarantee the structural invariant that any time the + /// admin changes, the pending-admin entry keyed to the old admin's address is cleaned up. + fn set_admin_and_clear_pending(env: &Env, old_admin: &Address, new_admin: &Address) { + env.storage() + .instance() + .remove(&DataKey::PendingAdmin(old_admin.clone())); + env.storage().instance().set(&DataKey::Admin, new_admin); + } + /// Accept a pending admin proposal. pub fn accept_admin(env: Env, candidate: Address) -> Result<(), WalletError> { candidate.require_auth(); @@ -258,10 +267,7 @@ impl GlobeWallet { if pending != candidate { return Err(WalletError::Unauthorized); } - env.storage().instance().set(&DataKey::Admin, &candidate); - env.storage() - .instance() - .remove(&DataKey::PendingAdmin(admin.clone())); + Self::set_admin_and_clear_pending(&env, &admin, &candidate); env.events().publish( (Symbol::new(&env, "admin_transferred"),), (admin, candidate), @@ -748,12 +754,7 @@ impl GlobeWallet { .instance() .get(&DataKey::Admin) .ok_or(WalletError::NotInitialized)?; - env.storage() - .instance() - .remove(&DataKey::PendingAdmin(old_admin.clone())); - env.storage() - .instance() - .set(&DataKey::Admin, &proposal.new_admin); + Self::set_admin_and_clear_pending(&env, &old_admin, &proposal.new_admin); env.storage().instance().remove(&DataKey::RecoveryProposal); // Same event name/shape as a normal transfer: downstream indexers // and the mobile app don't need to special-case recovery-driven @@ -1483,6 +1484,22 @@ mod tests { ); } + #[test] + fn test_pending_admin_entry_removed_after_normal_accept() { + let (env, _cid, admin, client) = setup(); + let candidate = Address::generate(&env); + client.propose_admin(&admin, &candidate); + client.accept_admin(&candidate); + + // Explicitly assert the old PendingAdmin(admin) key no longer resolves — a fresh propose + // targeting the SAME old admin address should find no leftover pending entry: + client.propose_admin(&candidate, &admin); // new admin proposes transferring back + assert_eq!( + client.try_accept_admin(&admin), // should require a fresh acceptance, not reuse stale state + Err(Ok(WalletError::NoPendingAdmin)) + ); + } + #[test] fn test_max_assets_limit() { let (env, _cid, _admin, client) = setup(); From 4ddc0468c3a8ae063e4b97425d782d076a9f5e9d Mon Sep 17 00:00:00 2001 From: Opulence Chuks Date: Wed, 26 Aug 2026 17:00:34 +0100 Subject: [PATCH 2/2] test(token-wrapper): Verify allowance rollback on transfer failure Adds test to reproduce a failing underlying token transfer after the allowance check passes, asserting that the allowance storage remains unchanged (rolled back). Also adds explanatory comments about atomicity expectations around the external transfer call. --- contracts/token-wrapper/src/lib.rs | 62 ++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/contracts/token-wrapper/src/lib.rs b/contracts/token-wrapper/src/lib.rs index d976467..5a69fc3 100644 --- a/contracts/token-wrapper/src/lib.rs +++ b/contracts/token-wrapper/src/lib.rs @@ -71,14 +71,20 @@ impl TokenWrapper { return Err(WrapperError::InvalidExpiry); } let key = DataKey::Allowance(owner.clone(), spender.clone()); - env.storage() - .persistent() - .set(&key, &Allowance { amount, expiry_ledger }); + env.storage().persistent().set( + &key, + &Allowance { + amount, + expiry_ledger, + }, + ); // Allowance carries an explicit `expiry_ledger` contract; storage TTL // must never expire *before* that ledger or the allowance would vanish // early through archival rather than through its own stated semantics. let extend_to = expiry_ledger.saturating_sub(env.ledger().sequence()); - env.storage().persistent().extend_ttl(&key, extend_to, extend_to); + env.storage() + .persistent() + .extend_ttl(&key, extend_to, extend_to); env.events().publish( (Symbol::new(&env, "approved"),), (owner, spender, amount, expiry_ledger), @@ -89,10 +95,10 @@ impl TokenWrapper { /// Return current allowance for (owner, spender). pub fn allowance(env: Env, owner: Address, spender: Address) -> Allowance { let key = DataKey::Allowance(owner, spender); - env.storage() - .persistent() - .get(&key) - .unwrap_or(Allowance { amount: 0, expiry_ledger: 0 }) + env.storage().persistent().get(&key).unwrap_or(Allowance { + amount: 0, + expiry_ledger: 0, + }) } /// Transfer tokens from `from` to `to` using a previously granted allowance. @@ -112,11 +118,10 @@ impl TokenWrapper { return Err(WrapperError::InvalidAmount); } let key = DataKey::Allowance(from.clone(), spender.clone()); - let current: Allowance = env - .storage() - .persistent() - .get(&key) - .unwrap_or(Allowance { amount: 0, expiry_ledger: 0 }); + let current: Allowance = env.storage().persistent().get(&key).unwrap_or(Allowance { + amount: 0, + expiry_ledger: 0, + }); if current.expiry_ledger < env.ledger().sequence() { return Err(WrapperError::AllowanceExpired); } @@ -127,9 +132,17 @@ impl TokenWrapper { amount: current.amount - amount, expiry_ledger: current.expiry_ledger, }; + // Persist the new allowance before calling the external token contract. + // Soroban's atomicity model guarantees that if the subsequent `transfer` call + // fails (e.g. insufficient underlying balance, trap), the entire transaction + // is rolled back, safely undoing this allowance debit. env.storage().persistent().set(&key, &new_allowance); - let extend_to = current.expiry_ledger.saturating_sub(env.ledger().sequence()); - env.storage().persistent().extend_ttl(&key, extend_to, extend_to); + let extend_to = current + .expiry_ledger + .saturating_sub(env.ledger().sequence()); + env.storage() + .persistent() + .extend_ttl(&key, extend_to, extend_to); let token_client = token::Client::new(&env, &token_id); token_client.transfer(&from, &to, &amount); env.events().publish( @@ -326,4 +339,23 @@ mod tests { env.ledger().with_mut(|l| l.sequence_number = 200); // exactly at expiry_ledger client.transfer_from(&spender, &token_id, &owner, &to, &100); // currently succeeds — lock this in explicitly } + + #[test] + fn test_allowance_state_rolls_back_if_underlying_transfer_fails() { + let (env, _id, client) = setup(); + let admin = Address::generate(&env); + let owner = Address::generate(&env); + let spender = Address::generate(&env); + let to = Address::generate(&env); + let (token_id, token_admin, _token) = create_token_contract(&env, &admin); + token_admin.mint(&owner, &100); // owner has only 100 real tokens + env.ledger().with_mut(|l| l.sequence_number = 100); + client.approve(&owner, &spender, &500, &200); // allowance says 500 is fine + // Attempt to move 300 — passes the allowance check (500 >= 300) but exceeds owner's real balance (100) + assert!(client + .try_transfer_from(&spender, &token_id, &owner, &to, &300) + .is_err()); + let a = client.allowance(&owner, &spender); + assert_eq!(a.amount, 500); // must still read the ORIGINAL allowance, proving the write was rolled back + } }