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

[chonilius]

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 fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20's mutability isn't implemented yet).
• If fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #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: fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20 (immutability — orthogonal but related; this issue's ceiling should be reused by fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #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.

[ZuLu0890]

I would love to work on this issue

[grantfox-oss]

🦊 GrantFox — @ZuLu0890 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #40)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.