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
- 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.
- 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).
Problem
fee_collector::set_treasury(fee_collector/src/lib.rslines 163-173) updatesKEY_TREASURYbut, unlikewithdraw(which publishes afee_wdrwevent, lines 154-157) andcollect_fee(which publishesfee_rcvd, lines 116-119), never publishes any event. There's also no validation thatnew_treasurydiffers 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 forwithdraw's default recipient framing per the module doc comment: "Default recipient of withdrawn fees").Note:
withdrawactually takes an explicitrecipientparameter (line 134) rather than always paying out to the storedKEY_TREASURY, soKEY_TREASURYis currently stored but not actually enforced as the mandatory withdrawal destination anywhere —withdrawcan send funds to anyrecipient, makingtreasurya 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 atreasuryconcept.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
treasuryisn't actually enforced aswithdraw's destination, its presence inContractConfig-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
env.events().publish((symbol_short!("trsry_upd"),), (old_treasury, new_treasury))toset_treasury, mirroring the pattern already used elsewhere in the same file.withdraw'srecipientparameter should be constrained to always equal the stored treasury (removing the parameter, always paying toKEY_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
withdraw'srecipientis constrained to the treasury going forward, that's a breaking API change for any existing integrator callingwithdrawwith an explicit different recipient — needs a clear migration note.set_treasuryshould probably rejectnew_treasury == current_treasuryas 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_eventasserting the event topic/data, and, depending on which direction (2) resolves, eithertest_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).