From 4edb8486fecc8f56b4c4ae50f1c2b317aec46734 Mon Sep 17 00:00:00 2001 From: phertyameen <81179612+phertyameen@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:58:07 +0100 Subject: [PATCH] fix(sweep_controller): stop moving tokens in sweep_account to match SDK custodial payout sweep_account called transfers::execute_transfers after EphemeralAccount::sweep, which invoked TokenClient::transfer(from = , ...). That can never succeed: a contract must authorize its own outgoing transfer (sweep() never does) and the funds sit on the ephemeral account's classic G... address, not on the contract, so a fixed authorization would still leave no balance to move. It would also double-pay the SDK's classic Horizon payout. Remove the transfer call and the now-unused transfers module; the controller verifies the signature/nonce, authorizes and marks the account swept, and emits SweepCompleted. Update integration tests to exercise the sweep path with explicit mock_auths only (no blanket mock_all_auths) and assert a missing controller authorization fails. Align docs/api-reference.md, docs/architecture.md and the README with the custodial payout model. Refs #758 --- README.md | 4 +- contracts/sweep_controller/src/lib.rs | 33 ++--- contracts/sweep_controller/src/transfers.rs | 34 ----- .../sweep_controller/tests/integration.rs | 133 ++++++++++++++++-- docs/api-reference.md | 10 +- docs/architecture.md | 34 +++-- 6 files changed, 157 insertions(+), 91 deletions(-) delete mode 100644 contracts/sweep_controller/src/transfers.rs diff --git a/README.md b/README.md index 776987a..f3c5466 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ The workspace contains **four** contracts, not two or three as earlier drafts of | Contract | Purpose | |---|---| | `ephemeral_account` | Enforces single-payment, expiry, and sweep-authorization state machine for a temporary account | -| `sweep_controller` | Validates Ed25519-signed sweep authorization and executes SEP-41 token transfers | +| `sweep_controller` | Validates Ed25519-signed sweep authorization and marks the account swept; the SDK performs the custodial payout | | `reserve_contract` | Stores/serves the network base-reserve amount (admin-set config value) used by `ephemeral_account` when reclaiming reserve | | `account_factory` | Batch-deploys and initializes many `ephemeral_account` instances in one transaction | @@ -25,7 +25,7 @@ The workspace contains **four** contracts, not two or three as earlier drafts of |----------|----------|--------|-------| | `verify_sweep_authorization` | `EphemeralAccount` | **Not a real signature check** | Ignores the `auth_signature` argument entirely (parameter is prefixed `_`). Authorization instead comes from `authorized_controller.require_auth()` - i.e. it trusts whichever address was set as the controller at `initialize()`. Calling `sweep()` directly (not via `SweepController`) will fail `require_auth` for anyone who isn't that controller, but it performs **no cryptographic verification of the signature itself**. | | `verify_sweep_auth` | `SweepController` | **Fully implemented** | Real Ed25519 verification (`env.crypto().ed25519_verify`) over `hash(destination + nonce + contract_id)`, with nonce-based replay protection. | -| `execute_transfers` | `SweepController` | **Fully implemented** | Calls SEP-41 `TokenClient::transfer()` for every recorded payment. | +| `sweep_account` | `SweepController` | **Fully implemented** | Verifies the signature/nonce, authorizes and marks the account swept, emits `SweepCompleted`. Does **not** move tokens - the SDK performs the custodial payout via Horizon. | | `batch_initialize` | `AccountFactory` | **Implemented, error detail dropped** | On per-account init failure it returns `error: None` instead of the actual error - see `lib.rs` comment `"In a real implementation, we'd serialize errors"`. Caller can see *that* an account failed but not *why*. | ### Implementation Notes diff --git a/contracts/sweep_controller/src/lib.rs b/contracts/sweep_controller/src/lib.rs index 133ae14..1f66bf3 100644 --- a/contracts/sweep_controller/src/lib.rs +++ b/contracts/sweep_controller/src/lib.rs @@ -3,7 +3,6 @@ mod authorization; mod errors; mod storage; -mod transfers; mod ephemeral_account_contract { soroban_sdk::contractimport!( @@ -18,7 +17,7 @@ use soroban_sdk::{ }; use authorization::AuthContext; -use bridgelet_shared::{AccountStatus, Payment}; +use bridgelet_shared::AccountStatus; pub use errors::Error; #[contract] @@ -76,7 +75,6 @@ impl SweepController { /// # Errors /// Returns Error::AuthorizationFailed if signature is invalid /// Returns Error::InvalidAccount if account is not in valid state - /// Returns Error::TransferFailed if token transfer fails /// Returns Error::UnauthorizedDestination if destination doesn't match authorized destination (when set) pub fn execute_sweep( env: Env, @@ -182,28 +180,15 @@ impl SweepController { return Err(Error::AccountNotReady); } - // Execute the actual token transfers for all recorded payments. + // No token transfer happens here. The controller only validates state + // and authorizes the sweep; the funds live on the ephemeral account's + // classic `G...` address (the sender paid it via Horizon), not on this + // contract. The SDK performs the custodial payout afterwards with a + // classic Horizon payment. Any SEP-41 transfer from here would both be + // unauthorized (the contract never authorizes its own outgoing + // transfer) and double-pay the SDK's payout. // - // info.payments yields ephemeral_account_contract::Payment (the - // contractimport!-generated type) — structurally identical to - // bridgelet_shared::Payment but a distinct Rust type, since - // contractimport! derives its own types from the wasm's interface - // metadata rather than reusing the shared crate. Convert explicitly - // field-by-field; transfers::execute_transfers expects the - // bridgelet_shared version. - let mut payments_vec = Vec::new(env); - for payment in info.payments.iter() { - payments_vec.push_back(Payment { - asset: payment.asset.clone(), - amount: payment.amount, - timestamp: payment.timestamp, - }); - } - - transfers::execute_transfers(env, &ephemeral_account, &destination, &payments_vec) - .map_err(|_| Error::TransferFailed)?; - - // Emit sweep completed event after successful transfer. + // Emit sweep completed event after the account state transition succeeds. emit_sweep_completed(env, ephemeral_account, destination, amount); Ok(()) diff --git a/contracts/sweep_controller/src/transfers.rs b/contracts/sweep_controller/src/transfers.rs deleted file mode 100644 index ae8b781..0000000 --- a/contracts/sweep_controller/src/transfers.rs +++ /dev/null @@ -1,34 +0,0 @@ -use crate::errors::Error; -use bridgelet_shared::Payment; -use soroban_sdk::token::TokenClient; -use soroban_sdk::{Address, Env, Vec}; - -/// Execute token transfers for all payments from the ephemeral account to the destination. -/// -/// Iterates over each recorded payment and calls the SEP-41 token contract's -/// `transfer()` function, moving funds from `from` to `destination`. -/// -/// The ephemeral account must have already authorized this contract to transfer -/// on its behalf — this is enforced by the Soroban auth model when `from.require_auth()` -/// is satisfied by the ephemeral account's invocation context. -/// -/// # Arguments -/// * `env` - Soroban environment -/// * `from` - Ephemeral account address (source of funds) -/// * `destination` - Recipient wallet address -/// * `payments` - All recorded payments to transfer -/// -/// # Errors -/// Returns `Error::TransferFailed` if any individual transfer fails -pub fn execute_transfers( - env: &Env, - from: &Address, - destination: &Address, - payments: &Vec, -) -> Result<(), Error> { - for payment in payments.iter() { - let token = TokenClient::new(env, &payment.asset); - token.transfer(from, destination, &payment.amount); - } - Ok(()) -} diff --git a/contracts/sweep_controller/tests/integration.rs b/contracts/sweep_controller/tests/integration.rs index 063e725..bec8bcc 100644 --- a/contracts/sweep_controller/tests/integration.rs +++ b/contracts/sweep_controller/tests/integration.rs @@ -7,7 +7,7 @@ use soroban_sdk::{ testutils::{Address as _, AuthorizedFunction, AuthorizedInvocation}, Address, BytesN, Env, IntoVal, }; -use sweep_controller::{Error, SweepController, SweepControllerClient}; +use sweep_controller::{SweepController, SweepControllerClient}; fn generate_test_keypair(env: &Env) -> (BytesN<32>, BytesN<64>) { let public_key = BytesN::from_array( @@ -81,9 +81,11 @@ fn setup_ready_account( ); let asset_id = Address::generate(&env); - env.mock_all_auths_allowing_non_root_auth(); + // `record_payment` is bookkeeping only and requires no authorization, so + // call it directly. The claim in each test below then runs with only the + // explicit `mock_auths` for the recipient - no blanket auth mocking that + // could hide a missing authorization in the sweep path. ephemeral_client.record_payment(&100, &asset_id); - env.set_auths(&[]); (controller_client, ephemeral_client, ephemeral_id) } @@ -111,34 +113,67 @@ fn test_initialize_prevents_double_init() { assert!(result.is_err()); } -/// Test that valid signatures are accepted +/// An invalid signature must be rejected by the sweep path even when every +/// other authorization is provided explicitly. No `mock_all_auths()` is used +/// here, so a missing authorization would surface as a failed test. #[test] -fn test_execute_sweep_with_valid_signature() { +fn test_execute_sweep_rejects_invalid_signature() { let env = Env::default(); - env.mock_all_auths(); - let _creator = Address::generate(&env); - // Deploy and initialize controller + // Deploy and initialize controller (creator authorizes `initialize` only). let controller_id = env.register(SweepController, ()); let controller_client = SweepControllerClient::new(&env, &controller_id); let creator = Address::generate(&env); let (authorized_signer, _) = generate_test_keypair(&env); - controller_client.initialize(&creator, &authorized_signer, &None); + controller_client + .mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &creator, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &controller_id, + fn_name: "initialize", + args: (&creator, &authorized_signer, &None::
).into_val(&env), + sub_invokes: &[], + }, + }]) + .initialize(&creator, &authorized_signer, &None); - // Deploy ephemeral account + // Deploy ephemeral account. let ephemeral_id = env.register(EphemeralAccountContract, ()); let ephemeral_client = EphemeralAccountContractClient::new(&env, &ephemeral_id); // Setup - let creator = Address::generate(&env); + let account_creator = Address::generate(&env); let recovery = Address::generate(&env); let destination = Address::generate(&env); let _asset = Address::generate(&env); let expiry = env.ledger().sequence() + 1000; // Initialize ephemeral account, authorizing this SweepController to call sweep() - ephemeral_client.initialize(&creator, &expiry, &recovery, &controller_id, &creator); + ephemeral_client + .mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &account_creator, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &ephemeral_id, + fn_name: "initialize", + args: ( + &account_creator, + &expiry, + &recovery, + &controller_id, + &account_creator, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .initialize( + &account_creator, + &expiry, + &recovery, + &controller_id, + &account_creator, + ); // Create an invalid signature (all zeros - different from valid signature) let invalid_sig = BytesN::from_array(&env, &[0u8; 64]); @@ -211,6 +246,80 @@ fn test_claim_succeeds_with_recipient_auth_and_relayable_flow() { assert_eq!(info.swept_to, Some(recipient)); } +/// The claim path must not proceed when the controller is not the account's +/// `authorized_controller`. Only the recipient's authorization is mocked here, +/// so the missing controller authorization has to fail the test. +#[test] +fn test_claim_fails_when_account_not_bound_to_controller() { + let env = Env::default(); + + let controller_id = env.register(SweepController, ()); + let controller_client = SweepControllerClient::new(&env, &controller_id); + + let creator = Address::generate(&env); + let (authorized_signer, _) = generate_test_keypair(&env); + controller_client + .mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &creator, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &controller_id, + fn_name: "initialize", + args: (&creator, &authorized_signer, &None::
).into_val(&env), + sub_invokes: &[], + }, + }]) + .initialize(&creator, &authorized_signer, &None); + + let ephemeral_id = env.register(EphemeralAccountContract, ()); + let ephemeral_client = EphemeralAccountContractClient::new(&env, &ephemeral_id); + + let account_creator = Address::generate(&env); + let recovery = Address::generate(&env); + let other_controller = Address::generate(&env); + let expiry = env.ledger().sequence() + 1_000; + ephemeral_client + .mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &account_creator, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &ephemeral_id, + fn_name: "initialize", + args: ( + &account_creator, + &expiry, + &recovery, + &other_controller, + &account_creator, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .initialize( + &account_creator, + &expiry, + &recovery, + &other_controller, + &account_creator, + ); + + let asset_id = Address::generate(&env); + ephemeral_client.record_payment(&100, &asset_id); + + let recipient = Address::generate(&env); + controller_client.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &recipient, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &controller_client.address, + fn_name: "claim", + args: (&recipient, &ephemeral_id).into_val(&env), + sub_invokes: &[], + }, + }]); + + let result = controller_client.try_claim(&recipient, &ephemeral_id); + assert!(result.is_err()); +} + #[test] fn test_claim_records_recipient_authorization_context() { let env = Env::default(); diff --git a/docs/api-reference.md b/docs/api-reference.md index 0d1e608..9e713b6 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -82,7 +82,7 @@ fn record_payment(env: Env, amount: i128, asset: Address) -> Result<(), Error> #### `sweep` -Marks the account as swept and authorizes fund transfers to `destination`. All recorded payments are included. The actual token transfers are executed by `SweepController` after this call completes. +Marks the account as swept and records `destination` as the payout target. All recorded payments are included. No token transfer happens here or in `SweepController`: the sender funds the ephemeral account's classic `G...` address via Horizon and the SDK performs the classic payment payout afterwards. ```rust fn sweep( @@ -389,7 +389,7 @@ fn simulate_sweep_paginated(env: Env, destination: Address, params: PaginationPa ## SweepController Contract -Orchestrates sweep authorization using Ed25519 signature verification and executes atomic token transfers. +Orchestrates sweep authorization using Ed25519 signature verification and authorizes the ephemeral account's sweep. It does not move tokens itself; the SDK performs the custodial payout. ### Functions @@ -428,7 +428,7 @@ fn initialize( #### `execute_sweep` -Verifies the Ed25519 authorization signature, then calls `EphemeralAccount::sweep()` and executes the token transfers to `destination`. +Verifies the Ed25519 authorization signature, then calls `EphemeralAccount::sweep()` and emits `SweepCompleted`. It does **not** transfer tokens: the funds live on the ephemeral account's classic `G...` address, not on any contract, and the SDK performs the classic Horizon payout afterwards. ```rust fn execute_sweep( @@ -456,7 +456,6 @@ fn execute_sweep( | `AuthorizedSignerNotSet` | Ed25519 public key has not been stored. | | `SignatureVerificationFailed` | Signature does not verify against the current nonce and destination. | | `AccountNotReady` | Ephemeral account has no recorded payments or zero total amount. | -| `TransferFailed` | A SEP-41 token `transfer()` call failed. | **Signature message format:** @@ -631,7 +630,8 @@ fn example_multi_asset( ephemeral.record_payment(&100_000_000, usdc_addr); ephemeral.record_payment(&5_000_000_000, xlm_addr); // 500 XLM in stroops - // Sweep transfers ALL recorded assets atomically + // Sweep marks the account swept for ALL recorded assets; the SDK pays the + // funds out from the ephemeral account's classic G... address afterwards. controller.execute_sweep(ephemeral_id, destination, &auth_sig); } ``` diff --git a/docs/architecture.md b/docs/architecture.md index c89e0d3..e99e305 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -48,7 +48,7 @@ The workspace (`Cargo.toml`) declares five members — four deployable contracts ``` contracts/ ├── ephemeral_account/ # Per-transfer temporary account + state machine -├── sweep_controller/ # Signature verification + token transfer execution +├── sweep_controller/ # Signature/claim verification + sweep authorization ├── reserve_contract/ # Standalone base-reserve config store ├── account_factory/ # Batch deployer/initializer for ephemeral_account └── shared/ # Common types (Payment, AccountStatus, AccountInfo, ...) @@ -73,12 +73,15 @@ contracts/ │ SweepController │ │ verifies Ed25519 sig, │ │ calls back into the │ - │ account, then executes │ - │ SEP-41 token transfers │ - └───────────┬────────────┘ - │ TokenClient.transfer() - ▼ - SEP-41 token contracts + │ account to mark it │ + │ swept; does NOT move │ + │ tokens itself │ + └───────────────────────┘ + + Funds sit on each ephemeral account's classic `G...` address (the sender + paid it via Horizon); after the controller marks the account swept, the + SDK performs the custodial payout with a classic Horizon payment. The + controller never calls `TokenClient` and holds no balance. ┌─────────────────────┐ │ ReserveContract │ standalone config contract — stores an @@ -103,7 +106,8 @@ contracts/ #### SweepController (On-Chain, implemented) - Independently verifies Ed25519 signatures over `hash(destination + nonce + contract_id)` - Enforces nonce-based replay protection -- Executes the actual SEP-41 `transfer()` calls for every recorded payment +- Authorizes the sweep and marks the account swept; it does **not** move tokens + (the SDK performs the classic Horizon payout — see Transfer Mechanism below) - Optionally locks all sweeps to one pre-set destination address #### ReserveContract (On-Chain, implemented, currently standalone) @@ -195,7 +199,7 @@ Note: `InvalidSignature` (9) exists in the error enum but `sweep()`'s current im ### SweepController Contract -**Source:** `contracts/sweep_controller/src/lib.rs`, `authorization.rs`, `transfers.rs` +**Source:** `contracts/sweep_controller/src/lib.rs`, `authorization.rs` This is where real cryptographic authorization lives. @@ -231,9 +235,11 @@ fn update_authorized_destination(env: Env, new_destination: Address) -> Result<( 3. On success, `execute_sweep()` increments the nonce (replay protection) *before* calling into `EphemeralAccount`. 4. Uses `env.authorize_as_current_contract()` with a `SubContractInvocation` context so that the downstream `EphemeralAccount::sweep()` call satisfies its `authorized_controller.require_auth()` check. -#### Transfer Mechanism (implemented, not planned) +#### Payout Mechanism (custodial, off-chain) + +The controller does **not** transfer tokens. `sweep_account()` verifies the signature and nonce, calls `EphemeralAccount::sweep()`, emits `SweepCompleted`, and returns. The recorded payments are bookkeeping only: the sender funds each ephemeral account's classic `G...` address via Horizon, so the contract holds no balance. The SDK's `SweepsService` performs the actual payout with a classic Horizon payment after the sweep is confirmed. -`transfers::execute_transfers()` iterates every `Payment` returned by `EphemeralAccount::get_info()` and calls `TokenClient::new(env, &payment.asset).transfer(from, destination, &payment.amount)` for each — atomic multi-asset sweep in one call. +This is why the controller can never call `TokenClient::transfer(from = , ...)`: a contract must authorize its own outgoing transfer (which `EphemeralAccount::sweep()` never does) and the funds are not on the contract anyway. Doing so would also double-pay the SDK's payout. #### `claim()` — gas-free path `recipient.require_auth()` (Soroban native auth on the outer transaction) replaces the Ed25519 signature entirely; the controller then authorizes itself as invoker of `EphemeralAccount::sweep_claim()`. This lets a relayer submit and pay fees while only the recipient signs. @@ -242,7 +248,7 @@ fn update_authorized_destination(env: Env, new_destination: Address) -> Result<( If `authorized_destination` was set at `initialize()`, every `execute_sweep`/`claim` call is checked against it (`validate_destination`) and can be changed via `update_authorized_destination()` — but only before any sweep has occurred (`nonce == 0` check). #### Errors -`InvalidAccount, TransferFailed, AuthorizationFailed, InsufficientBalance, AccountNotReady, AccountExpired, AccountAlreadySwept, InvalidSignature, SignatureVerificationFailed, AuthorizedSignerNotSet, InvalidNonce, UnauthorizedDestination` (discriminant `12` is unused/skipped — likely a removed variant; harmless in Rust but worth a cleanup pass). +`InvalidAccount, TransferFailed, AuthorizationFailed, InsufficientBalance, AccountNotReady, AccountExpired, AccountAlreadySwept, InvalidSignature, SignatureVerificationFailed, AuthorizedSignerNotSet, InvalidNonce, UnauthorizedDestination` (discriminant `12` is unused/skipped — likely a removed variant; harmless in Rust but worth a cleanup pass). `TransferFailed` is no longer produced by any code path now that the controller does not move tokens. --- @@ -304,10 +310,10 @@ Deploys a new `ephemeral_account` instance per request via `env.deployer().with_ SDK observes inbound payment via Horizon → calls `record_payment(amount, asset)` → status becomes `PaymentReceived`. ### Sweep (signed path) -SDK/relayer builds `hash(destination ++ nonce ++ sweep_controller_address)`, signs with the private key matching `authorized_signer` → calls `SweepController::execute_sweep(ephemeral_account, destination, signature)` → controller verifies signature, authorizes itself as invoker, calls `EphemeralAccount::sweep()` → controller reads `get_info()`, executes token transfers → `EphemeralAccount` reclaims its internal reserve tracking. +SDK/relayer builds `hash(destination ++ nonce ++ sweep_controller_address)`, signs with the private key matching `authorized_signer` → calls `SweepController::execute_sweep(ephemeral_account, destination, signature)` → controller verifies signature, authorizes itself as invoker, calls `EphemeralAccount::sweep()` → controller reads `get_info()` and emits `SweepCompleted` (no token transfer) → `EphemeralAccount` reclaims its internal reserve tracking. The SDK then pays out the funds from the ephemeral account's classic `G...` address via a classic Horizon payment. ### Sweep (gas-free claim path) -Recipient signs a Soroban auth entry for `SweepController::claim(recipient, ephemeral_account)` → relayer submits and pays fees → controller authorizes itself as invoker of `EphemeralAccount::sweep_claim()` → same transfer/reserve-reclaim tail as above. +Recipient signs a Soroban auth entry for `SweepController::claim(recipient, ephemeral_account)` → relayer submits and pays fees → controller authorizes itself as invoker of `EphemeralAccount::sweep_claim()` → same no-token-transfer, reserve-reclaim, then off-chain payout tail as above. ### Expiration Past `expiry_ledger` with no sweep → anyone calls `expire()` (or `recover()`) → funds path returns to `recovery_address`.