Skip to content

fix(globe-wallet): implement reentrancy-safe wiring to token-wrapper with token allowlist and CEI ordering (#92) - #104

Closed
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-92
Closed

fix(globe-wallet): implement reentrancy-safe wiring to token-wrapper with token allowlist and CEI ordering (#92)#104
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-92

Conversation

@s6pa1rta3n-lab

Copy link
Copy Markdown

Summary

Resolves #92.

This PR implements a reentrancy-safe on-chain wiring between globe-wallet and token-wrapper via a new entrypoint GlobeWallet::send, fulfilling the architectural design outlined in docs/design/architecture.md and addressing the threat model of arbitrary/malicious token contracts.

Definition of Done Checklist

  • Threat model written out: what a malicious/non-standard token_id could do if allowed to reach the wiring, and why the chosen mitigation closes it.
  • Both a token-contract allowlist mechanism (set_token_allowed/is_token_allowed/TokenNotAllowed) and a CEI-ordering guarantee across the wired call implemented as part of the wiring work.
  • Test using a mock malicious token contract that attempts to re-enter globe-wallet during transfer, proving the wiring is not exploitable (test_send_malicious_reentrant_token_rejected_and_rolled_back).
  • No regression to record_spend's existing single-function reentrancy invariant/tests.
  • cargo test --workspace output pasted below.

Threat Model & Mitigations

Threat Model

When invoking a caller-supplied token_id, the contract code executed on-chain is untrusted. A custom or malicious token contract could:

  1. Mid-flight Reentrancy: During its transfer execution, the token contract could attempt to call back into GlobeWallet::send, GlobeWallet::record_spend, GlobeWallet::set_spend_limit, or guardian management functions before the initial invocation unwinds.
  2. Spend Limit Circumvention: If state mutations were deferred until after the external transfer (interactions before effects), a re-entrant call would observe stale DailySpent state and could drain funds beyond the configured daily limit.
  3. State Desynchronization: In the absence of strict ordering and atomicity, reentrancy could lead to double-counting or unrecorded asset movement.

Chosen Mitigations

  1. Admin-Curated Token Allowlist (set_token_allowed, is_token_allowed, TokenNotAllowed = 1034):
    • GlobeWallet::send rejects any token_id not explicitly approved by the admin with WalletError::TokenNotAllowed during the pre-checks phase, before invoking token-wrapper or external code.
  2. Checks-Effects-Interactions (CEI) Ordering Across the Entire Call Chain:
    • GlobeWallet::send executes in strict CEI order:
      • Checks: Authenticates user (require_auth), verifies amount > 0, validates token_id allowlist membership, checks spend limit against current ledger day bucket.
      • Effects: Writes and commits updated DailySpent in persistent storage, extends TTL, and publishes spend_recorded before making any external contract invocation.
      • Interactions: Calls TokenWrapperClient::transfer_from with globe-wallet's contract address as the authorized spender.
    • Any re-entrant read mid-flight sees the updated, post-spend DailySpent record and cannot bypass the daily cap.
    • If the downstream token transfer fails or if any invalid action occurs, Soroban's transaction rollback atomically reverts all state changes.
  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).

Test Output (cargo test --workspace)

warning: `globe-wallet` (lib test) generated 5 warnings (1 duplicate)
warning: `globe-wallet` (lib) generated 1 warning
    Finished `test` profile [unoptimized + debuginfo] target(s) in 2.09s
     Running unittests src/lib.rs (target/debug/deps/globe_wallet-a9ae6297d2eeca30)

running 74 tests
test tests::test_add_asset_empty_code_fails ... ok
test tests::test_add_asset_overlong_code_fails ... ok
test tests::test_accept_by_wrong_address_fails ... ok
test tests::test_add_duplicate_guardian_fails ... ok
test tests::test_add_duplicate_asset_fails ... ok
test tests::test_add_and_list_guardians ... ok
test tests::test_add_and_get_assets ... ok
test tests::test_add_asset_case_variant_duplicate_fails ... ok
test tests::test_initialize ... ok
test tests::test_cancel_admin_transfer ... ok
test tests::test_daily_spent_survives_temporary_ttl_eviction ... ok
test tests::test_cannot_initiate_second_recovery_while_one_pending ... ok
test tests::test_double_approval_rejected ... ok
test tests::test_admin_can_cancel_recovery_even_after_quorum ... ok
test tests::test_execute_recovery_rejects_new_admin_same_as_current_admin ... ok
test tests::test_migrate_user_assets_within_limit_does_nothing ... ok
test tests::test_no_limit_allows_any_spend ... ok
test tests::test_migrate_user_assets_trims_excess ... ok
test tests::test_non_native_code_without_issuer_is_underspecified_and_rejected ... ok
test tests::test_pending_admin_cleared_after_accept_admin ... ok
test tests::test_initialize_twice_fails ... ok
test tests::test_migrate_user_assets_requires_admin ... ok
test tests::test_native_code_with_issuer_is_contradictory_and_rejected ... ok
test tests::test_non_admin_cannot_add_guardian ... ok
test tests::test_propose_upgrade_accepts_any_hash_without_validation ... ok
test tests::test_execute_upgrade_with_never_uploaded_hash_traps - should panic ... ok
test tests::test_propose_upgrade_requires_admin ... ok
test tests::test_propose_without_accept_keeps_admin_unchanged ... ok
test tests::test_propose_and_execute_upgrade - should panic ... ok
test tests::test_record_spend_boundary_drift_awareness ... ok
test tests::test_record_spend_boundary_last_second_of_day_accumulates ... ok
test tests::test_record_spend_boundary_first_second_of_new_day_resets ... ok
test tests::test_record_spend_bucket_is_integer_division ... ok
test tests::test_record_spend_exact_day_boundary ... ok
test tests::test_raise_spend_then_lower_limit ... ok
test tests::test_record_spend_exceeds_limit_fails ... ok
test tests::test_record_spend_negative_amount_fails ... ok
test tests::test_record_spend_negative_amount_cannot_bypass_daily_limit ... ok
test tests::test_record_spend_within_limit ... ok
test tests::test_max_guardians_limit ... ok
test tests::test_record_spend_zero_amount_fails ... ok
test tests::test_record_spend_overflow_does_not_poison_later_calls ... ok
test tests::test_record_spend_rejected_negative_amount_does_not_mutate_state ... ok
test tests::test_remove_asset ... ok
test tests::test_recovery_rejects_non_guardian ... ok
test tests::test_remove_guardian_below_threshold_fails ... ok
test tests::test_remove_nonexistent_asset_fails ... ok
test tests::test_recovery_clears_any_in_flight_normal_admin_transfer ... ok
test tests::test_require_admin_not_initialized ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_remove_guardian_dequorated_proposal_can_requorum_with_fresh_timelock ... ok
test tests::test_remove_guardian_who_never_approved_leaves_proposal_untouched ... ok
test tests::test_removed_guardian_cannot_initiate_recovery ... ok
test tests::test_revoke_recovery_approval_rejects_non_guardian ... ok
test tests::test_removed_guardian_approval_no_longer_counts_toward_quorum ... ok
test tests::test_revoke_recovery_approval_rejects_removed_guardian ... ok
test tests::test_revoking_approval_below_threshold_resets_timelock ... ok
test tests::test_send_negative_or_zero_amount_fails ... ok
test tests::test_set_recovery_config_rejects_single_guardian_threshold ... ok
test tests::test_set_recovery_config_rejects_threshold_above_guardian_count ... ok
test tests::test_send_unallowed_token_rejected ... ok
test tests::test_spend_limit_set_and_get ... ok
test tests::test_set_recovery_config_rejected_while_recovery_pending ... ok
test tests::test_spend_limit_ttl_extension_after_long_idle_period ... ok
test tests::test_set_recovery_config_requires_min_guardians ... ok
test tests::test_send_malicious_reentrant_token_rejected_and_rolled_back ... ok
test tests::test_upgrade_rejects_hash_mismatch ... ok
test tests::test_transfer_admin ... ok
test tests::test_upgrade_propose_double_fails ... ok
test tests::test_token_allowlist_admin_only_and_query ... ok
test tests::test_send_happy_path_with_token_wrapper ... ok
test tests::test_upgrade_requires_admin_and_ready_time ... ok
test tests::test_user_assets_ttl_extension_after_long_idle_period ... ok
test tests::test_max_assets_limit ... ok

test result: ok. 74 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.09s

     Running tests/record_spend_reentrancy.rs (target/debug/deps/record_spend_reentrancy-2f1651a9b25a79e4)

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.05s

     Running unittests src/lib.rs (target/debug/deps/token_wrapper-d6eee2b300274196)

running 11 tests
test tests::test_allowance_unset_pair_returns_zero ... ok
test tests::test_approve_negative_amount_fails ... ok
test tests::test_approve_and_allowance ... ok
test tests::test_approve_past_expiry_fails ... ok
test tests::test_transfer_from_expired_allowance_fails ... ok
test tests::test_transfer_from_insufficient_allowance_fails ... ok
test tests::test_transfer_from_happy_path ... ok
test tests::test_approve_overwrites_previous_allowance ... ok
test tests::test_transfer_from_zero_amount_fails ... ok
test tests::test_transfer_from_succeeds_exactly_at_expiry_ledger ... ok
test tests::test_transfer_from_rolls_back_allowance_when_underlying_transfer_fails ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.73s

   Doc-tests globe_wallet

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests token_wrapper

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Payout Routing

  • EVM Address (Base/Arbitrum/Polygon/ETH): 0xF46C9F6d70C50BF81ef3588AB523a90a594a2F89
  • Stellar Address: GCL6OXAMLD75BMTINA6EMRUDWK5THQUSHMYNLSNBCJAPZJHNYJTUNIBC

…with token allowlist and CEI ordering

- Add admin-curated token allowlist via `set_token_allowed` and `is_token_allowed` (with `TokenNotAllowed = 1034`) to reject untrusted token contract addresses.
- Add `GlobeWallet::send` entrypoint wiring `globe-wallet` to `token-wrapper::transfer_from` with strict Checks-Effects-Interactions (CEI) ordering.
- Commit spend bookkeeping to persistent storage before calling external contract code.
- Add unit tests verifying token allowlist admin enforcement, wired transfer settlement, daily limit enforcement, and rollback on malicious re-entrant token callbacks.
- Update architecture design and reentrancy documentation with threat model analysis and platform invariants.

Closes Orbit-Wal#92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant