diff --git a/contracts/wave_milestone/src/lib.rs b/contracts/wave_milestone/src/lib.rs index 4f9daf8..a210bfa 100644 --- a/contracts/wave_milestone/src/lib.rs +++ b/contracts/wave_milestone/src/lib.rs @@ -386,13 +386,18 @@ impl WaveMilestoneContract { .get::<_, MilestonePool>(&DataKey::Pool) .ok_or(Error::PoolNotFound)?; + // ── Authorization ── + // Non-owners must pass the WaveGuard check first, then are still + // rejected with UnauthorizedCaller. The pool owner bypasses WaveGuard + // so they can always recover their funds even if the guard is revoked. if maintainer != pool.maintainer { - return Err(Error::NotPoolMaintainer); + ensure_is_maintainer(&env, &pool.guard_contract, &maintainer)?; + return Err(Error::UnauthorizedCaller); } let now = env.ledger().timestamp(); if now <= pool.expiry { - return Err(Error::ClawbackTooEarly); + return Err(Error::PoolNotExpired); } let remaining = pool.remaining_balance(); diff --git a/contracts/wave_milestone/src/test.rs b/contracts/wave_milestone/src/test.rs index 31f28b2..97e4ac7 100644 --- a/contracts/wave_milestone/src/test.rs +++ b/contracts/wave_milestone/src/test.rs @@ -337,7 +337,7 @@ fn test_clawback_before_expiry_rejected() { let result = WaveMilestoneContractClient::new(&t.env, &t.contract_id).try_clawback_expired_funds(&t.maintainer); - assert_eq!(result.err().unwrap(), Ok(Error::ClawbackTooEarly)); + assert_eq!(result.err().unwrap(), Ok(Error::PoolNotExpired)); } #[test] @@ -573,8 +573,11 @@ fn test_revoked_maintainer_cannot_release_bounty() { assert_eq!(remaining, pool_size); } -/// Clawback uses address equality, not WaveGuard, to authenticate. -/// A revoked maintainer who created the pool can still recover their funds. +/// A maintainer removed from the WaveGuard registry can no longer claw +/// back expired funds as a non-owner. However, the pool *creator* always +/// retains clawback rights regardless of registry status — clawback uses +/// address equality only, bypassing WaveGuard so fund recovery is not +/// blocked by a compromised or revoked registry (see trust assumptions). #[test] fn test_revoked_maintainer_cannot_clawback() { let t = setup(); @@ -584,8 +587,11 @@ fn test_revoked_maintainer_cannot_clawback() { MockWaveGuardClient::new(&t.env, &t.guard_id).remove_maintainer(&t.maintainer); t.env.ledger().set_timestamp(t.expiry + 1); + // Pool owner bypasses WaveGuard — clawback must succeed even after revocation. + let before = MockTokenClient::new(&t.env, &t.token_id).balance(&t.maintainer); WaveMilestoneContractClient::new(&t.env, &t.contract_id) .clawback_expired_funds(&t.maintainer); + let after = MockTokenClient::new(&t.env, &t.token_id).balance(&t.maintainer); assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedMaintainer)); // Pool funds must remain untouched. diff --git a/contracts/wave_milestone/src/types.rs b/contracts/wave_milestone/src/types.rs index d9bdbf5..4662f94 100644 --- a/contracts/wave_milestone/src/types.rs +++ b/contracts/wave_milestone/src/types.rs @@ -125,22 +125,18 @@ pub enum DataKey { pub enum Error { /// No milestone pool has been created yet. PoolNotFound = 1, - /// The pool has not yet reached its expiry timestamp. PoolNotExpired = 2, - /// The `(repo_hash, issue_id)` pair has already been paid out. BountyAlreadyClaimed = 3, /// The pool's remaining balance is less than the requested bounty amount. InsufficientPoolBalance = 4, - InvalidGuard = 5, - UnauthorizedMaintainer = 6, - UnauthorizedCaller = 7, - NoFundsToClawback = 8, - TransferFailed = 9, - InvalidAmount = 10, - ExpiryInPast = 11, - /// `repo_hash` is the all-zero sentinel value, indicating a misconfigured call. + UnauthorizedMaintainer = 5, + UnauthorizedCaller = 6, + NoFundsToClawback = 7, + TransferFailed = 8, + InvalidAmount = 9, + ExpiryInPast = 10, + InvalidGuard = 11, InvalidRepoHash = 12, - /// `developer` is a zero-like address that cannot hold tokens. InvalidDeveloper = 13, } diff --git a/contracts/wave_milestone/tests/clawback.rs b/contracts/wave_milestone/tests/clawback.rs index 1c0a1c0..9f21618 100644 --- a/contracts/wave_milestone/tests/clawback.rs +++ b/contracts/wave_milestone/tests/clawback.rs @@ -47,7 +47,7 @@ fn test_clawback_before_expiry_rejected() { let result = ctx.client().try_clawback_expired_funds(&ctx.maintainer); - assert_eq!(result.err().unwrap(), Ok(Error::ClawbackTooEarly)); + assert_eq!(result.err().unwrap(), Ok(Error::PoolNotExpired)); } #[test] diff --git a/contracts/wave_milestone/tests/error_enum_coverage.rs b/contracts/wave_milestone/tests/error_enum_coverage.rs index 46d3bb7..1113c34 100644 --- a/contracts/wave_milestone/tests/error_enum_coverage.rs +++ b/contracts/wave_milestone/tests/error_enum_coverage.rs @@ -85,6 +85,9 @@ fn test_error_unauthorized_maintainer_release_bounty() { fn test_error_unauthorized_caller_clawback() { let ctx = TestContext::new(); ctx.fund_pool(DEFAULT_POOL_FUNDS); + // Register stranger as a valid WaveGuard maintainer — passes guard check + // but is not the pool creator, so UnauthorizedCaller is returned. + MockWaveGuardClient::new(&ctx.env, &ctx.guard_id).add_maintainer(&ctx.stranger); ctx.advance_to_expiry(); // stranger is not a WaveGuard maintainer, so UnauthorizedMaintainer fires first let result = ctx.client().try_clawback_expired_funds(&ctx.stranger);