[Joeloo1]

Summary

This PR addresses four separate issues identified in the codebase, fixing critical bugs in the milestones contract refund logic, documentation inconsistencies, a missing test case, and a pause mechanism design flaw.

Closes #527
Closes #528
Closes #531
Closes #530

---

Issue #1: Missing test_extend_deadline_unauthorized (#527)

Problem

The access control audit documentation (docs/access-control-audit.md, line ~181) references a test test_extend_deadline_unauthorized in escrow/src/test.rs to verify that extend_deadline properly rejects unauthorized callers. However, this test did not exist in the codebase, making the documentation claim unverifiable.

Solution

• Added test_extend_deadline_unauthorized test in contracts/escrow/src/test.rs
• Test verifies that a non-sponsor caller receives Error::Unauthorized when attempting to call extend_deadline
• Test confirms the access control behavior documented in the audit

Additional Fix

While investigating this issue, discovered and fixed a critical pre-existing compilation error in contracts/escrow/src/lib.rs:

• The upgrade and migrate functions had incorrect indentation (missing 4 spaces)
• There was orphaned/duplicate code between these functions causing compilation failure
• Fixed indentation and removed duplicate code, allowing the contract to compile successfully

Files changed:

• contracts/escrow/src/test.rs: +27 lines (new test)
• contracts/escrow/src/lib.rs: Indentation fix and cleanup

---

Issue #2: Outdated upgrade documentation (#528)

Problem

The docs/upgrade-storage-migration-design.md document contained outdated information:

1. Stated "The three deployed testnet contracts have no upgrade function" - incorrect, all three contracts now have upgrade(new_wasm_hash)
2. Referenced get_contract_version() function - incorrect, the actual function is get_version()
3. Multiple sections reflected the pre-upgrade implementation state

Solution

Updated docs/upgrade-storage-migration-design.md to accurately reflect current implementation:

Status updates:

• Changed "no upgrade function" → "all three contracts now have an admin-gated upgrade(new_wasm_hash) function"
• Noted that migration step must be called manually (tracked in [Regression on closed #338] upgrade() still writes the pre-upgrade CONTRACT_VERSION and there is still no migrate function #458)
• Updated "Retrofitting Already-Deployed Contracts" section with current deployment readiness
• Updated "Testnet (Current)" section to reflect upgrade support
• Updated "Backward Compatibility" section

Function name corrections:

• Changed all references from get_contract_version() → get_version() (4 occurrences)
• Updated example code to use correct function name
• Updated testing strategy section

Files changed:

• docs/upgrade-storage-migration-design.md: Comprehensive documentation updates

---

Issue #3: Milestone refund calculation bug with late contributors (#531)

Problem

The refund_remaining_budget function in the milestones contract incorrectly calculated refunds using:

share = (remaining * contribution.amount) / total_budget

This was fundamentally flawed because total_budget includes money already spent through release_issue calls. Late contributors who joined after releases would unfairly lose money to earlier contributors.

Example bug scenario:

1. Sponsor A creates milestone with 100 tokens
2. 90 tokens are released to maintainers
3. Sponsor B contributes 100 tokens (remaining = 110, total_budget = 200)
4. On cancel, using the buggy formula:
   • A gets: 110 * 100 / 200 = 55
   • B gets: 110 * 100 / 200 = 55
5. Result: B loses 45 tokens, and A gets back 45 tokens that were already spent on A's behalf

Solution

Implemented proper FIFO (first-in, first-out) contribution tracking:

1. Added remaining field to Contribution struct (types.rs):

   • Tracks unspent amount for each contribution
   • Initialized equal to amount
   • Decreases as releases draw from it via FIFO

2. Modified release_issue to deduct FIFO (lib.rs):

   • When releasing funds, deducts from contributions in order (earliest first)
   • Updates each contribution.remaining value accurately
   • Ensures precise tracking of what's spent from each contribution

3. Simplified refund_remaining_budget (lib.rs):

   • Now directly refunds each contribution.remaining
   • No more complex pro-rata calculation with dust distribution
   • Each sponsor gets back exactly their unspent remainder
   • Eliminated the entire largest-remainder dust distribution logic (no longer needed)

4. Added comprehensive test (test.rs):

   • test_late_contributor_refund_correctness demonstrates the exact scenario from the issue
   • Verifies A gets 10 (correct) not 55 (buggy)
   • Verifies B gets 100 (correct) not 55 (buggy)
   • Includes detailed comments explaining the before/after behavior

Additional Fix

Fixed a pre-existing bug in ContractInitialized event:

• Event tried to call .unwrap_or() on already-unwrapped max_sponsors variable
• Changed to use the variable directly

Files changed:

• contracts/milestones/src/types.rs: +4 lines (add remaining field with docs)
• contracts/milestones/src/lib.rs: Major refactoring of refund logic, FIFO tracking in release_issue, event fix
• contracts/milestones/src/test.rs: +77 lines (comprehensive new test case)

---

Issue #4: deallocate incorrectly blocked during pause (#530)

Problem

The deallocate function started with a pause check that blocked it during contract pause:

if is_paused(&env) {
    return Err(Error::ContractPaused);
}

This violated a core principle from docs/pause-circuit-breaker-design.md: "A paused contract must still allow sponsor-protective refund/reclaim paths."

The issue: During a long pause, sponsors could not recover allocated-but-unreleased funds at all:

• cancel_milestone (allowed during pause) only refunds remaining_budget
• Allocated amounts can only be moved back to remaining_budget through deallocate
• But deallocate was blocked, trapping sponsor funds

Solution

1. Removed pause check from deallocate (lib.rs):

   • deallocate is now allowed during pause
   • It's sponsor-protective: moves funds toward refund, doesn't commit new funds
   • Updated doc comment to explain the intentional design

2. Updated existing test (test.rs):

   • Renamed test_deallocate_blocked_while_paused → test_deallocate_allowed_while_paused_for_sponsor_protection
   • Test now verifies deallocate WORKS during pause
   • Demonstrates the full sponsor-protective path: deallocate → cancel_milestone → refund
   • Shows sponsor can recover their full contribution during pause

3. Updated access control audit (docs/access-control-audit.md):

   • Changed "pause-gating inconsistency tracked separately" → "intentionally not pause-gated"
   • Added reference to issue Add targeted milestones auth regression tests #752 and design principle 1
   • Clarified that deallocate is sponsor-protective, justifying pause exemption

Files changed:

• contracts/milestones/src/lib.rs: Removed pause check, updated documentation
• contracts/milestones/src/test.rs: Updated test to verify correct behavior
• docs/access-control-audit.md: Updated to reflect intentional design

---

Testing

All changes have been verified:

• ✅ Escrow contract compiles successfully (was failing before)
• ✅ Milestones contract compiles successfully
• ✅ New tests added for all behavioral changes
• ✅ Existing tests updated to reflect corrected behavior
• ✅ Documentation now accurately reflects implementation

---

Impact Assessment

Breaking Changes

Issue #3 (Milestone refund bug) introduces a storage format change:

• Contribution struct now includes a remaining field
• Existing deployed contracts with old Contribution format will need migration or redeployment
• This is acceptable for testnet; should be deployed before mainnet launch

Non-Breaking Changes

• Issues Escrow fund() allows issue-id griefing via front-running before the legitimate sponsor funds #1, Maintenance-pool deposit() allows pool-id squatting via TokenMismatch griefing #2, and Formal reentrancy audit of all token::Client cross-contract calls #4 are non-breaking
• Documentation fixes have no runtime impact
• Test additions have no production impact
• Pause behavior change is a bug fix that improves sponsor protection

---

Files Changed

contracts/escrow/src/lib.rs              | Major indentation fix
contracts/escrow/src/test.rs             | +27 lines (new test)
contracts/milestones/src/lib.rs          | Major refund refactoring, pause fix
contracts/milestones/src/test.rs         | +77 lines (new test), updated test
contracts/milestones/src/types.rs        | +4 lines (new field)
docs/access-control-audit.md             | Updated deallocate documentation
docs/upgrade-storage-migration-design.md | Comprehensive documentation updates

Total: 7 files changed, 227 insertions(+), 129 deletions(-)

[vercel]

@Joeloo1 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]

@Joeloo1 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