fee_collector::collect_fee has no reconciliation between claimed amounts and actual balance, and no invariant-check function exists

[abayomicornelius]

Overview

collect_fee records a lifetime total purely from its caller-supplied amount parameter, with no verification against the contract's actual measured token balance:

// fee_collector/src/lib.rs:92-122
pub fn collect_fee(
    env: Env,
    token: Address,
    amount: i128,
) -> Result<(), FeeCollectorError> {
    Self::assert_initialized(&env)?;

    if amount <= 0 {
        return Err(FeeCollectorError::InvalidAmount);
    }

    // Update lifetime total for this token.
    let total_key = (KEY_TOTAL, token.clone());
    let current_total: i128 = env
        .storage()
        .persistent()
        .get(&total_key)
        .unwrap_or(0i128);
    let new_total = current_total
        .checked_add(amount)
        .ok_or(FeeCollectorError::ArithmeticOverflow)?;
    env.storage().persistent().set(&total_key, &new_total);
    ...

The doc comment above collect_fee says "The actual token transfer must have already occurred (StellarSend transfers the fee to this contract's address before calling this). This function merely updates the lifetime accounting counter." — i.e. this is explicitly documented as a trust-the-caller design, not an oversight in isolation. The existing "no access control" issue in this repository (open issue #1) already covers the fact that anyone can currently call this function to inflate the counter arbitrarily.

This issue is about what remains true even after #1 is fixed (i.e. even once collect_fee is restricted to a legitimate, authorized caller): the contract still has no mechanism to detect drift between get_total_collected (the claimed lifetime total, fee_collector/src/lib.rs:186-192) and get_balance (the actual live token balance, :179-183). These two numbers can legitimately and permanently diverge for entirely benign reasons that have nothing to do with malicious callers — e.g. withdraw reduces get_balance without touching get_total_collected (by design, since it's a lifetime total, not a live balance), a legitimate caller passing a slightly-off amount due to a rounding or unit bug on the caller's side, or the fee-on-transfer-token scenario described in this batch's stellar_send/escrow/token_bridge issues where a caller's amount parameter doesn't match what was actually received. There is no reconcile(), no per-token invariant check, and no event or alert emitted anywhere that would signal "the claimed lifetime total and the sum of everything ever withdrawn/still-held no longer add up."

For a contract whose entire purpose is being the protocol's single source of truth for "how many fees has this system collected," having zero built-in way to audit that claim against reality — even for legitimate, non-adversarial callers — is a real observability gap that's independent of, and will still exist after, the access-control fix.

Requirements

• Add a way (a view function, or an off-chain-computable invariant clearly documented from the existing events/state) to compare get_total_collected(token) minus everything ever withdrawn against get_balance(token), so a divergence is at least detectable, even if the contract itself can't auto-correct it.
• Consider tracking a lifetime_withdrawn counter per token (parallel to KEY_TOTAL) so get_total_collected - lifetime_withdrawn becomes a computable "expected current balance" that can be diffed against get_balance on-chain or off-chain, without needing to trust that the two independently-updated numbers stay honest with no cross-check.
• Document, in the module doc comment, that get_total_collected is a claimed/reported figure, not a value independently verified against real token movement — this is a meaningful caveat for anyone building treasury-reporting tooling on top of this contract.

Acceptance Criteria

• A lifetime_withdrawn-style counter (or equivalent) is tracked alongside the existing KEY_TOTAL lifetime-collected counter.
• A view function or documented formula lets a caller compute "expected current balance" from on-chain state and compare it against get_balance.
• Module doc comment updated to state plainly that get_total_collected is not independently verified against real balance movement.
• A test demonstrates a scenario where get_total_collected and get_balance diverge (e.g. via a withdrawal, or a caller passing an amount inconsistent with what was actually transferred) and shows the new reconciliation mechanism correctly surfaces the divergence.

Additional Notes

Precise references: fee_collector/src/lib.rs:87-91 (doc comment explicitly stating the trust-the-caller design), :92-122 (collect_fee's implementation), :130-160 (withdraw, which reduces get_balance without any corresponding update to KEY_TOTAL, confirming these two numbers are tracked completely independently by design), :179-192 (get_balance and get_total_collected, the two queries with no cross-check between them).

Explicit differentiation from existing issue #1 ("fee_collector::collect_fee has no access control — anyone can inflate lifetime fee totals"): #1 is about who can call collect_fee. This issue is about the fact that even a fully-authorized, well-behaved caller's amount parameter is trusted with zero reconciliation against real balance movement, and that there is no invariant-check surface at all — a distinct, additive concern that #1's fix (an allowlist, or the companion "collect_fee's fix path is architecturally unclear" spike issue in this batch) does not itself resolve.

Test/reproduction plan: in fee_collector/src/test.rs, extend test_withdraw_sends_tokens_to_recipient (fee_collector/src/test.rs:96-110): after collect_fee(&token, &300i128) followed by withdraw(&token, &200i128, &recipient), note that get_total_collected still correctly reports 300 (unaffected by the withdrawal, as documented) while get_balance now reports 100 — assert that, absent the new lifetime_withdrawn tracking, there's no on-chain way to derive "300 collected, 200 withdrawn, 100 should remain" without an off-chain event replay; then, once the fix lands, assert the new tracking correctly reconstructs that relationship on-chain.

[DavidAkere204]

I would like to work on this issue

[grantfox-oss]

🦊 GrantFox — @DavidAkere204 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #41)
2. Your PR will be reviewed by the StellarSend maintainers

Good luck! Track your progress on GrantFox.