All three contracts: fee_bps has no sanity ceiling below 10000 (100%) — a valid-but-predatory full-fee config silently zeros every payout

オープン
  • 何か問題がありますか？
#40 コメント 2 件 リアクション 0 件 担当者 1 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

@ZuLu0890 がすでに取り組んでいます。

2026年8月17日 から。

  • #71 @ZuLu0890 による — オープン

評価

この issue はまだ評価されていません。

説明

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

Overview

All three contracts validate fee_bps at initialize with the exact same check: if fee_bps as i128 > BPS_DENOMINATOR { return Err(Error::InvalidFee); } (contracts/escrow/src/lib.rs:51-53, contracts/milestones/src/lib.rs:43-45, contracts/maintenance-pool/src/lib.rs:43-45). Note the comparison is strictly >, not >= — meaning fee_bps == 10_000 (i.e. exactly 100%) is accepted, not rejected, in all three contracts. fee_bps is also permanently immutable after initialize (tracked separately by #20), so whatever value passes this check governs every payout for the contract's entire lifetime.

A fee_bps of 10_000 is mathematically valid (it doesn't cause any overflow or panic) but is a fully predatory configuration: in compute_split (contracts/escrow/src/lib.rs:256-320, byte-identical in contracts/milestones/src/lib.rs:240-304), distributable = total - fee becomes total - total == 0, so every recipient's share computes to exactly 0 and the largest-remainder dust loop has nothing to distribute (dust = distributable - allocated = 0 - 0 = 0). The if share > 0 guard (contracts/escrow/src/lib.rs:133, contracts/milestones/src/lib.rs:172) means the transfer to every recipient is silently skipped — no error, no revert, just a completely legitimate-looking release/release_issue call that pays the entire escrow/allocation to treasury and nothing to any contributor. maintenance-pool::withdraw's analogous fee math (contracts/maintenance-pool/src/lib.rs:139-140) produces the same outcome for a single recipient: payout = amount - fee = 0.

There is currently no test anywhere in the three test suites that exercises fee_bps at or near this boundary (grep -n "10_000u32\|10000u32" contracts/*/src/test.rs only matches basis-point split values in recipients vectors, never a fee_bps argument to initialize), so this boundary is both unvalidated in the contract and unlocked-in by any test.

This is a distinct, narrower gap from #20 ("fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism"). #20 is about the inability to change fee_bps later; this issue is about the complete absence of a sanity ceiling below the mathematical maximum on the value accepted in the first place, independent of whether it's ever changeable. Even if #20 is never implemented, this gap remains exploitable by whoever controls initialize (see #33's front-running analysis for who that could be) on day one.

Requirements

  • Introduce a MAX_FEE_BPS constant meaningfully below BPS_DENOMINATOR (10000) in all three contracts — chosen and documented with actual reasoning about what a reasonable maximum protocol fee is for a bounty-payout platform (e.g. an order of magnitude below what any legitimate treasury fee would realistically be, so a value anywhere near the ceiling is itself a red flag worth surfacing, following the same "derive, don't guess" rigor the SmartDrop reference issue used for its multiplier/rate ceilings).
  • Apply the same ceiling check consistently to initialize in all three contracts (the only place fee_bps is ever set, since #20's mutability isn't implemented yet).
  • If #20 lands concurrently or later, apply the identical ceiling to whatever setter it introduces, keeping the two issues' constants in sync.

Acceptance Criteria

  • MAX_FEE_BPS constant added and documented (with reasoning) in all three contracts
  • initialize in all three contracts rejects fee_bps > MAX_FEE_BPS with the existing InvalidFee error (no new variant needed)
  • test_initialize_rejects_fee_bps_above_ceiling added to all three test suites
  • test_initialize_accepts_fee_bps_at_ceiling (boundary-exact, one below/at the new max) added to lock in the intended boundary
  • cargo test --workspace passes

Additional Notes

  • Precise references confirmed: contracts/escrow/src/lib.rs:51, contracts/milestones/src/lib.rs:43, contracts/maintenance-pool/src/lib.rs:43 — all three use the identical fee_bps as i128 > BPS_DENOMINATOR check with no lower ceiling than the absolute maximum.
  • Test sketch: test_release_with_100_percent_fee_pays_nothing_to_recipients as a pre-fix reproduction — initialize with fee_bps: 10_000u32 (currently succeeds), fund, release with a normal recipients vector summing to 10000 bps, assert every recipient's token balance is 0 and treasury's balance equals the full escrow amount — demonstrating the exploit is real and silent (no error surfaced to the caller) before showing the ceiling now rejects it outright at initialize time.
  • Cross-references: #20 (immutability — orthogonal but related; this issue's ceiling should be reused by #20's eventual setter); the SmartDrop reference issue (SmartDropLabs/smartdrop-contracts#89) this whole audit batch was calibrated against, which flagged an identical "admin-settable value has a lower bound but no sane upper bound" pattern — worth linking in the PR as the direct inspiration for treating this as a first-class, very hard-caliber finding rather than a trivial validation nit.
主要言語
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 を短くまとめたダイジェスト。