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

Abierto
  • ¿Algo no está bien?
#53 1 comentario 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
5/5
Tiempo estimado
Más de una semana
Aptitud para principiantes
25/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Tranquilo
Stack tecnológico
rust
Área
blockchain

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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).
Lenguaje dominante
Rust
Estrellas
1
Forks
102
Merge medio
4 h 12 min
PR fusionados (30 d)
30

Preparar el entorno

  • Sin Dockerfile ni archivo de Docker Compose
  • Tiene una plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de MergeFi/contracts

Todos los issues de MergeFi/contracts

Issues similares

Más issues de Rust

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.