Reentrancy-safe wiring of globe-wallet::send to token-wrapper::transfer_from

[yosemite01]

Closes #92

Root cause

docs/design/architecture.md correctly flags that globe-wallet and token-wrapper don't call each other yet, and sketches two ways to wire them. Neither sketch — nor record_spend's own reentrancy proof in docs/record-spend-reentrancy.md — says anything about what happens once the wiring calls an arbitrary, caller-supplied token_id. record_spend's proof is scoped to its own function body, which never makes an external call; the wiring's whole point is to make one. That's a new reentrancy surface with no existing analysis, and issue #92 is the request to close that gap before shipping the wiring, not after.

The full analysis — including exactly what Soroban's platform reentry protection does and does not guarantee for the new 3-hop chain (GlobeWallet::send → token-wrapper::transfer_from → arbitrary token_id) — is written up in docs/design/wiring-reentrancy-threat-model.md. Please read that first; this description summarizes it but the doc has the full reasoning, alternatives considered, and citations.

Short version: Soroban's ContractReentryMode::Prohibited default means an adversarial token_id genuinely cannot call back into GlobeWallet (or into token-wrapper) while their frames are active on the stack — this repo already proved that for the single-contract case, and this PR proves it empirically for the new multi-hop case with a real mock malicious contract (see below). What reentry protection does not cover is a token_id that simply isn't real — a fake token whose transfer returns success without moving any value would still let record_spend's daily counter and token-wrapper's allowance both decrement against nothing. That's a trust problem, not a reentrancy problem, and it's why the fix is two mitigations, not one.

Design decisions

Both of the issue's suggested mitigations are implemented, because they close two different halves of the problem — full rationale in §4 of the threat-model doc:

1. Token allowlist. GlobeWallet::send rejects any token_id that isn't on a new admin-curated allowlist (add_allowed_token / remove_allowed_token / is_token_allowed, backed by DataKey::AllowedToken(Address)), before touching any state. This is what actually stops the fake-token/griefing class of problem, which reentry protection can't.
2. CEI ordering across the whole wired call, not just inside record_spend. send calls record_spend (all of globe-wallet's own state changes) as a direct in-frame Rust call — not a cross-contract invocation, so record_spend's existing proof is untouched — and only then calls out to token-wrapper::transfer_from, last. This is defense-in-depth on top of (1): even in a hypothetical future where Soroban's reentry rules changed, every one of globe-wallet's own writes for this operation is already committed before any external code runs.

Implementation

• contracts/globe-wallet/src/lib.rs:
  • New DataKey::TokenWrapperId and DataKey::AllowedToken(Address), appended to the end of the enum per the file's existing storage-key-stability convention.
  • New WalletError variants — TokenNotAllowed = 1034, TokenWrapperNotSet = 1035, TokenTransferFailed = 1036 — continuing the contiguous numbering PR Fix asset validation, dedupe constants/imports, and cover upgrade + transfer_from tests #81 established.
  • New admin functions: set_token_wrapper, get_token_wrapper, add_allowed_token, remove_allowed_token, is_token_allowed.
  • New send(user, token_id, asset_code, to, amount) — the actual wiring. Doc comment covers the error cases and the atomicity guarantee (a failure anywhere reverts the whole call, including record_spend's write).
  • globe-wallet now depends on the token-wrapper crate directly for a real, typed TokenWrapperClient, rather than hand-rolling env.invoke_contract. Verified empirically, not assumed: built globe-wallet to wasm32-unknown-unknown --release with the dependency in place and parsed the resulting binary's export section directly (hand-rolled a ~40-line WASM export-table parser — no wasm-inspection tooling was available in this environment) to confirm approve/transfer_from/allowance do not leak into globe-wallet's compiled .wasm. This workspace's release profile (lto = true, codegen-units = 1) correctly dead-code-eliminates them. Documented in a comment on the dependency in contracts/globe-wallet/Cargo.toml, with a note to re-verify if the release profile's LTO settings ever change.

The reentrancy test — the actual proof

test_send_rejects_reentrant_malicious_token deploys a real, separate mock contract (MaliciousToken, defined right above the test) implementing just the standard token interface's transfer function — enough for token-wrapper's token::Client::transfer call to route to it exactly as it would for any real SAC, since token_id is unconstrained beyond the allowlist check. Its transfer implementation immediately tries to call back into GlobeWalletClient::record_spend for the same (victim, asset_code) pair send is already processing — the exact double-count scenario the issue describes.

The test asserts:

1. send fails as a whole (try_send(...).is_err()) — the reentrant callback does not let it silently succeed.
2. No DailySpent entry survives at all (read directly via env.as_contract) — proving the transaction fully reverted, so there's no partial state where the legitimate spend committed but the reentrant one didn't, or vice versa.

Seven more tests cover the rest: the happy path (real SAC token actually moves, allowance decrements, daily spend records), rejection when token_id isn't allowlisted, rejection when no token-wrapper is configured, and — importantly — that a send rejected for exceeding the daily limit moves zero tokens and leaves the allowance untouched, demonstrating the CEI-ordering guarantee in practice. Plus two admin-gating tests (set_token_wrapper, add_allowed_token both reject non-admin callers).

No regression to record_spend's existing reentrancy invariant

record_spend itself is untouched — send calls it as a plain associated-function call, same as any other internal caller. All existing tests (test_record_spend_*, two_spends_in_one_host_invocation_accumulate in the separate tests/record_spend_reentrancy.rs integration test) pass unmodified.

A note on this branch's history

This branch has been rebased twice onto main during review-prep as other PRs merged ahead of it:

• First rebase (through PR Fix asset validation, dedupe constants/imports, and cover upgrade + transfer_from tests #81): picked up two already-fixed build issues I'd independently hit and fixed myself (token-wrapper's orphaned enum fragment — fix(token-wrapper): remove orphaned enum fragment, fix WrapperError discriminants #71 — and the WalletError discriminant renumbering — fix(globe-wallet): renumber WalletError to a single contiguous 1001+ scheme #72) rather than reintroducing conflicting duplicate fixes.
• Second rebase (through PR feat(globe-wallet): recovery_completed event + fix workspace test suite (was fully broken) + fix missing instance-TTL protection #105, feat/recovery-completed-event): that PR also independently hit and fixed the same ed25519-dalek build-blocking dependency conflict I'd found (see below) — via vendoring rather than my original git-tag-patch approach. I deferred to their solution (it's more robust: no network dependency at all) rather than keeping mine. That same rebase also picked up fixes for what were, as of my last update to this PR, 4 failing pre-existing tests unrelated to this change — the suite is now fully green (see Evidence below).

Flagging this so reviewers aren't confused if the diff or description looks different from an earlier glance.

Evidence (cargo test --workspace)

$ cargo check --workspace --all-targets
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 16.02s
(only pre-existing warnings — deprecated methods — no errors)

$ cargo test --workspace --no-fail-fast
     Running unittests src/lib.rs (globe-wallet)
running 80 tests
test tests::test_send_happy_path_moves_tokens_and_records_spend ... ok
test tests::test_send_rejects_disallowed_token ... ok
test tests::test_send_rejects_when_token_wrapper_not_set ... ok
test tests::test_send_over_daily_limit_fails_and_moves_no_tokens ... ok
test tests::test_send_rejects_reentrant_malicious_token ... ok
test tests::test_add_allowed_token_requires_admin ... ok
test tests::test_remove_allowed_token ... ok
test tests::test_set_token_wrapper_requires_admin ... ok
... [72 more pre-existing tests, all passing] ...
test result: ok. 80 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.34s

     Running tests/record_spend_reentrancy.rs
running 1 test
test two_spends_in_one_host_invocation_accumulate ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

     Running unittests src/lib.rs (token-wrapper)
running 11 tests
... all 11 ok ...
test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.12s

Fully green — 80/80, 11/11, 1/1. (An earlier revision of this PR description noted 4 pre-existing, unrelated test failures at the time; those have since been resolved by other merges this branch picked up on rebase, so there's nothing left to call out as a known-unrelated gap.)

Files touched

• docs/design/wiring-reentrancy-threat-model.md — new, the design-decision write-up.
• contracts/globe-wallet/src/lib.rs — the wiring (send + allowlist + admin setters), the mock malicious contract, 8 new tests.
• contracts/globe-wallet/Cargo.toml — the token-wrapper dependency, commented with the wasm-export verification.
• New test_snapshots/ fixture files for the 8 new tests. Pre-existing snapshot files for untouched tests are not included in this diff — they regenerate with cosmetic/non-deterministic differences (trailing newline, freshly-Address::generated values) on every cargo test run regardless of this change, so committing them would just be noise unrelated to this PR.