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
メンテナーはふだん 1 日以内に返信
まだ誰も着手していません。
評価
- 難易度
- 5/5
- 見積もり時間
- 1週間以上
- 初心者へのやさしさ
- 25/100
- issue の種類
- バグ
- 明瞭さ
- 明確に書かれている
- 活発さ
- 静か
- 技術スタック
- rust
- 領域
- blockchain
調査の方向性
Read #20 alongside contracts/escrow/src/lib.rs, contracts/milestones/src/lib.rs, and contracts/maintenance-pool/src/lib.rs, then inspect the corresponding types.rs files and existing contract tests. Trace when each record is created and how compute_split or withdraw obtains its fee; decide and document the maintenance-pool treatment explicitly. Done means tests cover the chosen snapshot behavior and the fee-change work is coordinated with #20 so no retroactive-fee window is exposed.
索引モデルが issue の本文から書いたものです。
説明
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: u32field toEscrow,Milestone, andMaintenancePool'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 capturedfee_bps, not a fresh instance-storage read, closing the retroactivity gap before #20 ever ships a way to actually trigger it. - This issue's fix should land before or alongside #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 #20 first would create a real window where the bug is live in production between the two PRs landing.
- For
maintenance-poolspecifically: since a single pool accumulates deposits from multiple sponsors, each potentially made under a different historicalfee_bpsif #20 ships a mutable fee,withdraw's fee calculation cannot correspond to any single "the fee_bps for this pool" the wayescrow/milestonescan — 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_bpssnapshotted at deposit/creation time inEscrowandMilestonerecords, used bycompute_splitinstead 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, changefee_bpsvia whatever mechanism exists (today, only possible in a test via direct storage manipulation, since #20 isn't implemented; this test is forward-looking and should be written to be meaningful once #20 lands), release, assert the fee charged matches the value atfundtime, not the changed value - Coordinated with #20 so the two land in an order that never exposes the retroactive-fee-change window in production
-
cargo test --workspacepasses
Additional Notes
- Precise references:
compute_split's fresh instance-storage read atcontracts/escrow/src/lib.rs:273-277/contracts/milestones/src/lib.rs:257-261;withdraw's atcontracts/maintenance-pool/src/lib.rs:134-138; the threetypes.rsfiles confirming no per-recordfee_bpsfield exists today anywhere. - This issue is filed now, ahead of #20's implementation, specifically so the two are designed together rather than #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: #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).
- 主要言語
- Rust
- スター
- 1
- フォーク
- 102
- 平均マージ
- 4時間 12分
- マージ済み PR(30日)
- 30
環境構築
- Dockerfile・Docker Compose ファイルなし
- プルリクエストのテンプレートあり
- コントリビューションガイドなし
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
MergeFi/contracts のほかの issue
-
allocations map retains released issues — phantom ledger drift再び着手できるかも @PINYOPATTANAWASANPORN が 12 日前に担当しましたが、オープン中のプルリクエストはありません。 オープン
難易度 2/5 1〜3時間 初心者へのやさしさ 25/100
MergeFi/contracts#608 · コメント 3 件 ·
メンテナーはふだん 1 日以内に返信
-
escrow::contribute and milestones::contribute accept new funds after deadline + GRACE_PERIOD, when anyone can already refund or cancel対応中かも @urielit1313-creator が 29 日前に担当しました。 オープンbug
難易度 3/5 1〜2日 初心者へのやさしさ 25/100
MergeFi/contracts#568 · コメント 3 件 ·
メンテナーはふだん 1 日以内に返信
-
Makefile deploy targets deploy the unoptimised .wasm — `stellar contract optimize` writes *.optimized.wasm対応中かも @urielit1313-creator が 29 日前に担当しました。 オープンbug good first issue
難易度 2/5 1〜3時間 初心者へのやさしさ 25/100
MergeFi/contracts#567 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
keep_alive never refreshes instance storage, so a contract kept alive only via keep_alive can still archive Admin/Oracle/Treasury対応中かも @urielit1313-creator が 29 日前に担当しました。 オープンbug
難易度 4/5 3〜5日 初心者へのやさしさ 25/100
MergeFi/contracts#566 · コメント 3 件 ·
メンテナーはふだん 1 日以内に返信
-
[Regression on closed #254] milestones::deallocate still has no test asserting admin authorization対応中かも @urielit1313-creator が 29 日前に担当しました。 オープンsecurity testing
難易度 2/5 1〜3時間 初心者へのやさしさ 25/100
MergeFi/contracts#565 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
MergeFi/contracts の issue をすべて見る
似ている issue
-
難易度 2/5 1〜3時間 初心者へのやさしさ 76/100
メンテナーはふだん 1 日以内に返信
-
design-data-spec
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
adobe/spectrum-design-data#1744 ·
メンテナーはふだん 1 日以内に返信
-
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
メンテナーはふだん 1 日以内に返信
-
USERID reads /etc/passwd directly instead of looking the user up対応中かも このイシューにリンクされたプルリクエストがオープン中、またはマージ済みです。 オープンdefect
難易度 2/5 1〜3時間 初心者へのやさしさ 67/100
メンテナーはふだん 1 日以内に返信
-
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
メンテナーはふだん 1 日以内に返信