fee_collector::collect_fee's obvious access-control fix would break its real caller — no allowlist primitive exists anywhere

[abayomicornelius]

Problem (spike / open-ended)

Open issue #1 in this repository correctly identifies that collect_fee has no access control:

// fee_collector/src/lib.rs:92-96
pub fn collect_fee(
    env: Env,
    token: Address,
    amount: i128,
) -> Result<(), FeeCollectorError> {
    Self::assert_initialized(&env)?;
    // ... no require_auth() of any kind, on anyone, anywhere in this function

The obvious, minimal-looking fix that comes to mind for "add access control" is the same pattern used everywhere else in this contract: fetch the stored admin and call admin.require_auth(), exactly like withdraw (fee_collector/src/lib.rs:138-143) and set_treasury (:165-170) already do. But that fix is architecturally wrong for collect_fee specifically, and shipping it naively would break the contract's actual integration, not just tighten it.

collect_fee is documented as being called by stellar_send, not by fee_collector's own admin:

"Called by StellarSend (or any authorised caller) ... The actual token transfer must have already occurred (StellarSend transfers the fee to this contract's address before calling this)." — fee_collector/src/lib.rs:83-91

stellar_send does not hold fee_collector's admin key, has no reason to, and shouldn't need to — stellar_send's own admin and fee_collector's admin are two entirely separate Address values set independently at each contract's own initialize call (stellar_send/src/lib.rs:113-142 vs fee_collector/src/lib.rs:67-81), with no relationship enforced or even checkable between them anywhere in the code. If collect_fee is "fixed" by adding admin.require_auth() against fee_collector's stored admin, then stellar_send's calls to collect_fee (once the separate, already-open "stellar_send never invokes fee_collector::collect_fee" issue is also fixed) would need fee_collector's admin to co-sign every single payment stellar_send processes — which is obviously not the intended integration and would make the two contracts unusable together.

What collect_fee actually needs is a caller-allowlist primitive — some notion of "which contract address(es) are permitted to report fee collections" — distinct from the admin concept that governs withdraw/set_treasury. That primitive doesn't exist today in any form: not in the real code, and not even as a stub in fee_collector/src/stubs.rs's backlog of planned features (which lists two-step admin rotation, a version query, a per-epoch withdrawal limit, multi-token withdrawal, and several test-coverage items — but nothing resembling a caller allowlist or authorized-reporter concept).

Why this is open-ended rather than a fixed-scope fix

This needs real design work, not just a one-line require_auth() insertion, because of several genuinely unresolved questions: (1) Should the allowlist be a single Address (the one stellar_send deployment this fee_collector instance is meant to serve) set at initialize time, or a general Vec<Address>/Map<Address, bool> supporting multiple authorized callers (relevant if this fee_collector is ever meant to serve more than one stellar_send-like integration, or a future contract that also needs to forward fees)? (2) If it's a settable allowlist rather than a single fixed value, who can add/remove entries — the admin, presumably, but does adding a caller need the SAME two-step-safety treatment as admin rotation itself (see the companion "no admin rotation" issue in this batch), since a wrongly-added caller could inflate fee totals just as effectively as the current fully-open access? (3) Does the allowlisted caller's identity need to be verified as the contract's own invoking address (i.e. env.current_contract_address() as seen from the perspective of a cross-contract call, which Soroban doesn't expose directly to the callee the way require_auth() exposes an authorizing account/contract) — Soroban's authorization model is built around Address::require_auth(), which works naturally for an account authorizing an action, but stellar_send calling collect_fee isn't naturally an "authorization" in that sense, it's a plain cross-contract function call; the right primitive might actually be closer to "verify the caller invoked this via a cross-contract call from a specific, allowlisted contract address," which needs research into what Soroban's SDK actually exposes for verifying the immediate caller of a cross-contract invocation (env.current_contract_address() inside the caller's frame vs. any caller-identity primitive exposed to the callee — this is worth verifying against current soroban-sdk 21.x capabilities before committing to a design, since APIs in this area have evolved across Soroban protocol versions).

Investigation steps

Research what soroban-sdk 21.x actually exposes for a callee to verify "which contract invoked me" (as opposed to "which account/contract authorized this specific action via require_auth"), since these are two different Soroban concepts and the right fix likely depends on which one is actually available and idiomatic for this use case.

Testing strategy

Once a design direction is chosen, the deliverable test is a genuine cross-contract integration test (in the spirit of the already-open "no cross-contract integration tests exist" issue #29, and directly reusable by it) that deploys both fee_collector and a caller contract in the same test Env, wires the caller as the allowlisted reporter, and proves: (a) the allowlisted caller's collect_fee invocation succeeds without needing fee_collector's admin to co-sign, and (b) a non-allowlisted third contract's collect_fee invocation is rejected — this is the concrete artifact proving whichever allowlist design is chosen actually solves both halves of the problem (blocking arbitrary callers per #1, without breaking the real stellar_send integration).