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

Đang mở
#53 1 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức phù hợp với người mới
25/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Ít trao đổi
Công nghệ
rust
Lĩnh vực
blockchain

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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).
Ngôn ngữ chính
Rust
Star
1
Fork
102
Merge trung bình
4 giờ 12 phút
Pull request đã merge (30 ngày)
30

Chuẩn bị môi trường

  • Không có Dockerfile hay tệp Docker Compose
  • Có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của MergeFi/contracts

Tất cả issue của MergeFi/contracts

Issue tương tự

Thêm issue về Rust

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.