StellarSendError::InvalidFeeCollector is dead code — initialize() never validates the fee_collector address

[abayomicornelius]

Overview

StellarSendError declares an error variant specifically for a bad fee_collector address:

// stellar_send/src/error.rs:26-27
/// The fee-collector address stored in config is invalid.
InvalidFeeCollector = 10,

It is never constructed anywhere. Repo-wide grep -n "InvalidFeeCollector" stellar_send/src/*.rs matches exactly one line — the declaration itself in error.rs. Nothing in lib.rs, batch.rs, payment_request.rs, or subscription.rs ever returns it.

Consistent with that, initialize performs zero validation on the fee_collector: Address parameter it's given:

// stellar_send/src/lib.rs:113-142
pub fn initialize(
    env: Env,
    admin: Address,
    fee_bps: u32,
    fee_collector: Address,
) -> Result<(), StellarSendError> {
    if env.storage().instance().has(&KEY_CONFIG) {
        return Err(StellarSendError::AlreadyInitialized);
    }
    if fee_bps > 10_000 {
        return Err(StellarSendError::InvalidFeeBps);
    }
    admin.require_auth();
    let config = ContractConfig {
        admin,
        fee_bps,
        fee_collector,
        active: true,
    };
    env.storage().instance().set(&KEY_CONFIG, &config);
    env.storage().instance().set(&KEY_SEQ, &0u64);
    Ok(())
}

fee_collector is stored verbatim, with no check that it's a live, functioning fee_collector-shaped contract, and no check that it isn't, say, the zero-equivalent address, admin's own address, or a plain classic Stellar account that has never established a trustline for whatever asset ends up being sent as a fee. initialize can only ever be called once per contract instance (guarded by KEY_CONFIG, no re-initialization or set_fee_collector path exists at all — set_fee_collector is listed only as an unshipped backlog item in stellar_send/src/stubs.rs:5), so a bad fee_collector set at deploy time is permanent for that deployment.

The practical consequence: every payment path that charges a nonzero fee (send_payment, send_batch_payment, fulfill_payment_request, execute_subscription — all four call token_client.transfer(..., &config.fee_collector, &fee_amount) or the transfer_from equivalent whenever fee_amount > 0) will start failing the moment fee_bps > 0, if fee_collector can't actually receive the token being transferred (e.g. a classic Stellar G-account without an established trustline for a non-SAC-native classic asset, or simply an address nobody controls). Since fee_bps starts at whatever initialize was given and can be 0 initially, this failure mode can lie completely dormant through every test and through initial low-fee operation, only to start reverting every single payment the instant the admin later raises fee_bps above zero via set_fee — a self-inflicted, contract-wide DoS with no clean error message pointing at the actual root cause (a bad fee_collector set once, at deploy time, and never revisited).

Requirements

• Add validation in initialize that rejects an obviously-invalid fee_collector (at minimum: not equal to env.current_contract_address(), not equal to admin, and ideally a lightweight liveness probe such as attempting to call a cheap read-only function on it if fee_collector's expected interface is known at this point in the design) — returning the already-declared InvalidFeeCollector error.
• Since Soroban can't cheaply verify "this address is a functioning fee_collector contract implementing collect_fee" without an actual cross-contract call (which itself has failure modes worth reasoning about, e.g. a call to an address with no code at all), document what level of validation is actually feasible versus what has to remain a deployment-time operational responsibility.
• Now that InvalidFeeCollector is dead code, either wire it up as above, or if truly no validation is feasible, remove it and document why in the error type, rather than leaving a variant that implies a validation which doesn't exist.

Acceptance Criteria

• initialize performs at least basic sanity validation on fee_collector and returns StellarSendError::InvalidFeeCollector when it fails.
• test_initialize_rejects_invalid_fee_collector (or equivalently named) added to stellar_send/src/test.rs.
• If full liveness validation isn't feasible at initialize time, the module doc comment and initialize's own doc comment clearly state the remaining operational responsibility for deployers to verify fee_collector correctness out-of-band.
• InvalidFeeCollector is either genuinely reachable via a test, or removed with a documented rationale — it should not remain a declared-but-unreachable variant either way.

Additional Notes

Precise references: stellar_send/src/error.rs:26-27 (InvalidFeeCollector declaration, the only place it appears in the entire stellar_send crate), stellar_send/src/lib.rs:113-142 (initialize, zero validation on fee_collector), stellar_send/src/stubs.rs:5 (feat(stellar_send): add set_fee_collector — confirms even a post-deployment correction path is unshipped backlog, not existing functionality), and the four fee-forwarding call sites: stellar_send/src/lib.rs:211 (send_payment), stellar_send/src/batch.rs:60 (send_batch_payment), stellar_send/src/payment_request.rs:124 (fulfill_payment_request), stellar_send/src/subscription.rs:234 (execute_subscription).

How this is a distinct pattern from, not a duplicate of, existing "dead code" issues in this repo (ContractConfig.active in issue #12, get_payment_record's error conflation in issue #26): those are about state that exists but is never read/enforced or the wrong error being returned. This is the third and different variant of the same underlying code-hygiene theme in this codebase — an error variant that exists and is correctly documented, but is never constructed anywhere because the validation it was meant to guard was never written — worth flagging on its own since fixing it requires adding real validation logic, not just wiring up an existing check.

Test/reproduction plan: the unambiguous, easily-reproducible core of this issue doesn't depend on exactly how a bad fee_collector fails downstream — it's simply that initialize accepts one with zero checks. In stellar_send/src/test.rs, using the existing setup() helper: call client.initialize(&admin, &0u32, &admin), i.e. passing admin's own address as fee_collector, and confirm it succeeds cleanly on main today (test_initialize_accepts_self_as_fee_collector_today, documenting the current, unvalidated behavior). Separately, test_initialize_accepts_non_contract_address_as_fee_collector using a bare Address::generate(&env) with no deployed contract behind it at all should also succeed today. After the fix, both should be rejected with StellarSendError::InvalidFeeCollector (or whatever subset of these checks the chosen validation strategy actually covers — the point of these tests is to pin down exactly which invalid shapes are caught, since a lightweight sanity check may not catch every case, e.g. a syntactically valid but genuinely non-functioning classic account without the right trustline may remain undetectable on-chain at initialize time regardless of the fix, which is worth stating explicitly in the PR rather than over-promising full validation).