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 个 reaction 已指派 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 分钟
30 天内合并 PR
30

环境准备

  • 没有 Dockerfile 或 Docker Compose 文件
  • 有 Pull Request 模板
  • 没有贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

MergeFi/contracts 的其他 Issue

查看 MergeFi/contracts 的全部 Issue

相似的 Issue

更多 Rust Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。