All three contracts: fee_bps has no sanity ceiling below 10000 (100%) — a valid-but-predatory full-fee config silently zeros every payout
维护者通常 1 天内回复
评估
这个 Issue 还没有评估数据。
描述
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_BPSconstant meaningfully belowBPS_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
initializein all three contracts (the only placefee_bpsis 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_BPSconstant added and documented (with reasoning) in all three contracts -
initializein all three contracts rejectsfee_bps > MAX_FEE_BPSwith the existingInvalidFeeerror (no new variant needed) -
test_initialize_rejects_fee_bps_above_ceilingadded 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 --workspacepasses
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 identicalfee_bps as i128 > BPS_DENOMINATORcheck with no lower ceiling than the absolute maximum. - Test sketch:
test_release_with_100_percent_fee_pays_nothing_to_recipientsas a pre-fix reproduction —initializewithfee_bps: 10_000u32(currently succeeds),fund,releasewith a normal recipients vector summing to 10000 bps, assert every recipient's token balance is0andtreasury'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 atinitializetime. - 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 模板
- 没有贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 Pull Request,并在描述里引用这个 Issue 编号。
MergeFi/contracts 的其他 Issue
-
allocations map retains released issues — phantom ledger drift可能重新可做 @PINYOPATTANAWASANPORN 于 12 天前认领,目前没有进行中的 PR。 未关闭
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#608 · 3 条评论 ·
维护者通常 1 天内回复
-
escrow::contribute and milestones::contribute accept new funds after deadline + GRACE_PERIOD, when anyone can already refund or cancel可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug
难度 3/5 1-2 天 新手友好度 25/100
MergeFi/contracts#568 · 3 条评论 ·
维护者通常 1 天内回复
-
Makefile deploy targets deploy the unoptimised .wasm — `stellar contract optimize` writes *.optimized.wasm可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug good first issue
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#567 · 1 条评论 ·
维护者通常 1 天内回复
-
keep_alive never refreshes instance storage, so a contract kept alive only via keep_alive can still archive Admin/Oracle/Treasury可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug
难度 4/5 3-5 天 新手友好度 25/100
MergeFi/contracts#566 · 3 条评论 ·
维护者通常 1 天内回复
-
[Regression on closed #254] milestones::deallocate still has no test asserting admin authorization可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭security testing
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#565 · 1 条评论 ·
维护者通常 1 天内回复
查看 MergeFi/contracts 的全部 Issue
相似的 Issue
-
难度 2/5 1-3 小时 新手友好度 72/100
aws/aws-emulator#30 · 1 条评论 ·
-
area:post bug effort:low P2 source:agent
难度 1/5 1-3 小时 新手友好度 82/100
维护者通常 1 天内回复
-
难度 2/5 1-3 小时 新手友好度 76/100
维护者通常 1 天内回复
-
Chat images never show without ImageMagick: ffprobe fallback uses an option string ffprobe 9 rejects未关闭
难度 2/5 1-3 小时 新手友好度 79/100
eliasstravik/herdr-projects#1011 ·
-
design-data-spec
难度 2/5 1-3 小时 新手友好度 78/100
adobe/spectrum-design-data#1744 ·
维护者通常 1 天内回复