Fix critical security issues #3, #5, #17, #20

[gideononiru]

Summary

This PR addresses four critical security and testing issues identified in the MergeFi contracts audit.

Changes

Issue #3: Token Accounting Protection

Problem: Trusting caller-supplied amounts could lead to accounting desync with fee-on-transfer, rebasing, or malicious tokens.

Solution:

• Implemented measure_transfer_delta() in common lib to measure actual balance changes
• Updated fund() and contribute() in both escrow and milestones contracts
• Prevents bookkeeping from diverging from actual token holdings

Files Changed:

• contracts/common/src/lib.rs
• contracts/escrow/src/lib.rs
• contracts/milestones/src/lib.rs

Issue #5: State Machine Bug Fix

Problem: release_issue() could be called after cancel_milestone(), leading to inconsistent state.

Solution:

• Added milestone.closed check to release_issue()
• Returns Error::MilestoneClosed if milestone is already closed
• Updated deallocate() documentation for proper cancellation workflow

Files Changed:

• contracts/milestones/src/lib.rs

Issue #20: Fee Update Mechanism with Safety Limits

Problem: No mechanism to update protocol fees, and unlimited changes could harm sponsors.

Solution:

• Added set_fee_bps() admin function to escrow and milestones contracts
• Implemented validate_fee_change() with 5% (500 bps) max change per call
• Point-in-time fee locking: sponsors know the fee when committing funds
• New escrows/milestones use current fee; existing ones retain their original fee

Files Changed:

• contracts/common/src/lib.rs (validation logic)
• contracts/escrow/src/lib.rs
• contracts/milestones/src/lib.rs

Issue #17: Property-Based Fuzz Testing Infrastructure

Problem: Need more rigorous testing to catch edge cases in split calculations.

Solution:

• Added proptest dependency to workspace
• Created test_fuzz.rs with comprehensive property tests
• 7 property tests verify mathematical invariants:
  • Sum preservation (total - fee)
  • Zero fee behavior
  • 100% fee behavior
  • Proportional distribution
  • Monotonic BPS (increasing share never decreases amount)
  • No amount exceeds total
  • All amounts non-negative
• 3 edge case tests for boundary conditions

Files Changed:

• Cargo.toml
• contracts/common/Cargo.toml
• contracts/common/src/lib.rs
• contracts/common/src/test_fuzz.rs (new)

Testing

• ✅ All 131 tests pass
  • 58 escrow tests
  • 40 milestones tests
  • 23 maintenance-pool tests
  • 10 property-based fuzz tests
• Fixed MockPanicToken in test files to support balance tracking
• All existing functionality preserved

Closes

• Closes Audit accounting correctness against non-standard (fee-on-transfer / rebasing / reentrant) SEP-41 tokens #3
• Closes cancel_milestone does not block release_issue for already-allocated, not-yet-released issues — budget/accounting invariant violation #5
• Closes Build a property-based fuzz harness for compute_split covering adversarial recipient vectors #17
• Closes fee_bps is immutable after initialize — design and implement a secure, bounded update mechanism #20

[vercel]

Someone is attempting to deploy a commit to the chonilius' projects Team on Vercel.

A member of the Team first needs to authorize it.

[drips-wave]

@gideononiru Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits