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

Đang mở
  • Có gì không ổn?
#40 2 bình luận 0 reaction 1 người được giao Xem trên GitHub

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

@ZuLu0890 đang làm issue này rồi.

Từ ngày 17/8/2026.

  • #71 của @ZuLu0890 — đang mở

Đánh giá

Issue này chưa được đánh giá.

Mô tả

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.
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.