token-wrapper: no test covers transfer_from when the underlying token transfer itself fails after allowance state is already persisted #38

Context

transfer_from (contracts/token-wrapper/src/lib.rs, ~line 94) updates and persists the caller's allowance before invoking the underlying token contract:

let new_allowance = Allowance { amount: current.amount - amount, expiry_ledger: current.expiry_ledger };
env.storage().persistent().set(&key, &new_allowance);
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);   // <-- external call, after state write
env.events().publish(...);
Ok(())

Problem

If token_client.transfer panics or traps (e.g., from has insufficient balance in the underlying SAC/token contract, or token_id isn't a valid token contract, or the token contract itself has a bug/trap), Soroban's transaction-level atomicity means the entire host transaction — including the env.storage().persistent().set(&key, &new_allowance) a few lines earlier — is rolled back, since Soroban transactions are all-or-nothing at the ledger level. So in practice this is very likely safe by construction. However, there is no test in this file that actually exercises the failing-transfer case to confirm that expectation — every existing test (test_transfer_from_happy_path, test_transfer_from_insufficient_allowance_fails, etc.) either succeeds end-to-end or fails before reaching the token-transfer call (insufficient/expired allowance, checked inside this contract). None of them let the allowance check pass and then have the underlying token transfer itself fail.

Impact

The state-ordering (write-before-external-call) is a well-known checks-effects-interactions-adjacent pattern that's usually fine under Soroban's atomicity model, but "usually fine because of platform guarantees I'm relying on but never tested" is exactly the kind of assumption that's worth locking in with an explicit test — especially in a contract literally handling token movement, where CONTRIBUTING.md sets an unusually high bar for evidence over claims.

Suggested fix

Add a test that mints the owner enough to pass the allowance check but arranges for the underlying token.transfer call to fail (e.g., request a transfer amount within the allowance but exceeding the owner's actual SAC balance), then assert that client.try_transfer_from(...) fails as expected and that a subsequent client.allowance(&owner, &spender) call still reports the original, pre-attempt allowance (proving the storage write was indeed rolled back, not just that the call itself returned an error).

Definition of done

• New test reproducing a failing underlying-token-transfer scenario after the allowance check passes.
• Test asserts allowance storage is unchanged after the failed attempt.
• If the assumption turns out to be wrong (allowance is NOT rolled back), this becomes a real bug fix (reorder the storage write to after the external call) rather than just a coverage gap — the PR should say explicitly which case was found.

Deeper Technical Analysis

The reasoning that this is "very likely safe by construction" deserves to be stated precisely, because it
rests on a platform guarantee this file never explicitly cites or tests against: Soroban (like most smart
contract platforms with atomic transaction execution) rolls back all state changes made during a host
function invocation if that invocation ultimately traps/fails, including storage writes that happened
earlier in the same call, before the failing operation. That means the ordering in transfer_from — persist
new_allowance first (lines ~118-124), then call token_client.transfer(&from, &to, &amount) (line 126)
— should be safe under that guarantee: if token_client.transfer traps (insufficient underlying balance,
invalid token_id, a bug in the token contract itself), the entire transaction rolls back, undoing the
allowance write along with it, so the allowance debit and the actual token movement stay atomically coupled
even though they're two separate contract calls under the hood.

But "should be safe under a platform guarantee this file never states or tests" is exactly the gap worth
closing. Contrast with how carefully record_spend's sibling contract documents its reliance on subtle
platform behavior — the extended comment at globe-wallet lines 49-56 explicitly walks through the
persistent-vs-temporary storage TTL distinction and why it matters, specifically because relying on
implicit platform behavior without stating and testing the assumption was judged (correctly) to be too
risky to leave undocumented for a security-relevant invariant. transfer_from's write-before-external-call
ordering is arguably in the same risk category — allowance accounting is precisely the kind of state a bug
here would silently corrupt (a spender could end up debited with no successful transfer, or the reverse) —
yet it currently has zero test coverage of the failure path, and zero comment explaining why the ordering is
believed safe.

There's also a secondary, narrower risk worth flagging even if the atomicity assumption holds: token_id
is caller-supplied and never validated to be a legitimate token contract before token::Client::new(&env, &token_id) is constructed and invoked (line 125) — an attacker-supplied token_id pointing at an
arbitrary, non-token contract, or a malicious contract designed to behave unexpectedly, is a related but
distinct concern from the ordering question this issue is scoped to (worth its own follow-up if not already
covered elsewhere).

Reproduction Sketch

#[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
}

Related Issues

Related to #36 and #37 as a third token-wrapper hardening item, but higher-priority than either since it
touches a genuine (if likely-safe) assumption about cross-call atomicity rather than a pure documentation
gap — worth prioritizing this one first among the three if triaging by risk.