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

オープン
#53 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 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 の本文から書いたものです。

説明

architecture GrantFox OSS Maybe Rewarded Official Campaign | FWC26 security Third Campaign very hard

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 #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-pool specifically: since a single pool accumulates deposits from multiple sponsors, each potentially made under a different historical fee_bps if #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 #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 at fund time, 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 --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 #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 ファイルなし
  • プルリクエストのテンプレートあり
  • コントリビューションガイドなし

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

MergeFi/contracts のほかの issue

MergeFi/contracts の issue をすべて見る

似ている issue

Rust の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。