All three contracts: fee_bps is read fresh at payout time, not snapshotted at deposit — #20's mutable-fee design would enable retroactive fee changes

[chonilius]

Overview

In all three contracts, fee_bps is stored exactly once, in instance storage, and read fresh, at payout time, not captured per-record at deposit time. compute_split (contracts/escrow/src/lib.rs:256-320, identical in contracts/milestones/src/lib.rs:240-304) reads it via env.storage().instance().get(&DataKey::FeeBps) (line 273-277 in escrow, line 257-261 in milestones) at the moment release/release_issue is called — not at fund/create_milestone/allocate time. maintenance-pool::withdraw does the identical thing (contracts/maintenance-pool/src/lib.rs:134-138). Neither Escrow, Milestone, nor MaintenancePool (contracts/escrow/src/types.rs:13-20, contracts/milestones/src/types.rs:9-19, contracts/maintenance-pool/src/types.rs:10-17) has any field capturing "what fee_bps was in effect when this record was created."

Today, this is harmless, because fee_bps is permanently immutable after initialize (as #20 tracks) — there is genuinely only ever one value it could be, for the entire life of a given contract instance, so "fresh at payout time" and "snapshotted at deposit time" are indistinguishable in practice.

This issue exists because #20 ("fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism") is an open, accepted-as-worth-doing issue, and its naive implementation would silently introduce a real bait-and-switch bug that doesn't exist today. The moment a set_fee_bps-style function lands (per #20's own stated ask), any Escrow/Milestone/pool Deposit that was funded under one fee_bps value but hasn't yet been released/withdrawn would, under the current "read fresh at payout time" design, be silently charged whatever fee_bps happens to be in effect at release time — not what the sponsor saw and agreed to when they deposited. Concretely: a sponsor funds an escrow expecting a 2.5% protocol fee (the value visible via get_fee_bps at the time they called fund); before the bounty is resolved, the admin raises fee_bps to, say, 8%, via #20's new setter; when release is eventually called, the recipient's payout is computed against the new, higher fee — a fee change applied retroactively to a deposit that already happened, with no mechanism for the sponsor to have anticipated or consented to it.

This is exactly the kind of "the currently-open issue's naive implementation introduces a new bug" finding this batch's review methodology is meant to surface — #20's own issue body (as currently scoped) doesn't mention this consequence, and a straightforward implementation of "add a set_fee_bps function" would land it without anyone noticing until a sponsor complains.

Requirements

• Add a fee_bps: u32 field to Escrow, Milestone, and MaintenancePool's per-Deposit (or per-pool, depending on the chosen granularity — see note below) records, captured at deposit/creation time from the current instance-storage value.
• Update compute_split/withdraw's fee calculation in all three contracts to use the record's captured fee_bps, not a fresh instance-storage read, closing the retroactivity gap before fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 ever ships a way to actually trigger it.
• This issue's fix should land before or alongside fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20, not after, for the same reason the deallocate/deadline-ordering issues filed elsewhere in this batch need to precede the issues whose naive fixes they protect against — implementing fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 first would create a real window where the bug is live in production between the two PRs landing.
• For maintenance-pool specifically: since a single pool accumulates deposits from multiple sponsors, each potentially made under a different historical fee_bps if fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 ships a mutable fee, withdraw's fee calculation cannot correspond to any single "the fee_bps for this pool" the way escrow/milestones can — the pool-level design needs its own explicit decision (e.g. fee_bps applied at withdrawal time is arguably more defensible for a pool than for a single-sponsor escrow, since a pool's balance is already a blended pool of contributions with no per-token-unit sponsor attribution at withdrawal time regardless). Document this contract-specific reasoning explicitly rather than applying the same fix uniformly without justification.

Acceptance Criteria

• fee_bps snapshotted at deposit/creation time in Escrow and Milestone records, used by compute_split instead of a fresh instance-storage read
• Explicit, reasoned decision for maintenance-pool (snapshot per-deposit despite the blended-balance complication, or deliberately keep it fresh-at-withdrawal with the tradeoff documented)
• Test: test_release_uses_fee_bps_from_fund_time_not_current_value — fund an escrow, change fee_bps via whatever mechanism exists (today, only possible in a test via direct storage manipulation, since fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 isn't implemented; this test is forward-looking and should be written to be meaningful once fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 lands), release, assert the fee charged matches the value at fund time, not the changed value
• Coordinated with fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 so the two land in an order that never exposes the retroactive-fee-change window in production
• cargo test --workspace passes

Additional Notes

• Precise references: compute_split's fresh instance-storage read at contracts/escrow/src/lib.rs:273-277 / contracts/milestones/src/lib.rs:257-261; withdraw's at contracts/maintenance-pool/src/lib.rs:134-138; the three types.rs files confirming no per-record fee_bps field exists today anywhere.
• This issue is filed now, ahead of fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20's implementation, specifically so the two are designed together rather than fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 landing first and this becoming a live, exploitable retroactive-fee bug in production before anyone notices — the whole point of filing it is to be proactive rather than reactive.
• Cross-references: fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 (the issue whose naive implementation this issue's analysis protects against — should be read together, and ideally implemented together); the fee_bps-ceiling issue filed earlier in this batch (orthogonal but related — both are about fee_bps's trustworthiness as a value sponsors implicitly rely on).

[Ugooweb]

Hello, I have reviewed the issue and understand what needs to be done. I would appreciate being assigned to it.

[grantfox-oss]

🦊 GrantFox — @Ugooweb 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 #53)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.