Skip to content

get_payment_record returns the misleading StellarSendError::NotInitialized for a simple not-found case #26

Description

@abayomicornelius

Problem

fee_collector::set_treasury (fee_collector/src/lib.rs lines 163-173) updates KEY_TREASURY but, unlike withdraw (which publishes a fee_wdrw event, lines 154-157) and collect_fee (which publishes fee_rcvd, lines 116-119), never publishes any event. There's also no validation that new_treasury differs from the current value, and no check against an obviously-wrong value (e.g. accidentally passing the admin's own address, which — while not necessarily invalid — is worth at least a sanity-check-style safeguard given how consequential the treasury address is for withdraw's default recipient framing per the module doc comment: "Default recipient of withdrawn fees").

Note: withdraw actually takes an explicit recipient parameter (line 134) rather than always paying out to the stored KEY_TREASURY, so KEY_TREASURY is currently stored but not actually enforced as the mandatory withdrawal destination anywhere — withdraw can send funds to any recipient, making treasury a purely informational/queryable field today rather than a binding constraint. That's arguably a second, more significant issue worth calling out here since it affects the whole point of having a treasury concept.

Why it matters

Silent admin-address-changing operations without an audit trail (via events) are a red flag in any contract handling third-party funds — anyone monitoring the contract via events (as the README's events table implies is the expected integration pattern) would have no way to detect a treasury change happened at all. Separately, if treasury isn't actually enforced as withdraw's destination, its presence in ContractConfig-equivalent storage here is misleading — it reads as a safety rail ("fees can only go to the designated treasury") but isn't one.

Proposed fix

  1. Add env.events().publish((symbol_short!("trsry_upd"),), (old_treasury, new_treasury)) to set_treasury, mirroring the pattern already used elsewhere in the same file.
  2. Decide and document explicitly whether withdraw's recipient parameter should be constrained to always equal the stored treasury (removing the parameter, always paying to KEY_TREASURY) or should remain fully admin-discretionary (in which case the doc comment's "default recipient" framing should be corrected to avoid implying an enforced constraint that doesn't exist).

Edge cases

  • If withdraw's recipient is constrained to the treasury going forward, that's a breaking API change for any existing integrator calling withdraw with an explicit different recipient — needs a clear migration note.
  • set_treasury should probably reject new_treasury == current_treasury as a no-op guard (cheap, avoids a spurious event) — minor but easy to include in the same fix.

Testing strategy

Add test_set_treasury_emits_event asserting the event topic/data, and, depending on which direction (2) resolves, either test_withdraw_always_pays_treasury_regardless_of_recipient_param (if constrained) or a corrected doc-comment-only change with no new test needed (if left discretionary).

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingcontractsSmart contract logicvery hardVery difficult / senior-level bounty issue

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions