diff --git a/contracts/wave_milestone/src/lib.rs b/contracts/wave_milestone/src/lib.rs index a210bfa..c20a6b3 100644 --- a/contracts/wave_milestone/src/lib.rs +++ b/contracts/wave_milestone/src/lib.rs @@ -313,9 +313,9 @@ impl WaveMilestoneContract { ensure_is_maintainer(&env, &pool.guard_contract, &maintainer)?; // ── Developer address validation (issue #109) ── - // Reject the all-zero contract address by comparing the raw 32-byte id. - // CAAAA...D2KM is the Strkey encoding of the 32-byte all-zero contract id. - if developer == Address::from_str(&env, "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM") { + // Reject a payout directed back to this contract — tokens sent to the + // contract vault are not recoverable through normal bounty claims. + if developer == env.current_contract_address() { return Err(Error::InvalidDeveloper); } @@ -371,8 +371,10 @@ impl WaveMilestoneContract { /// /// # Auth /// - `maintainer.require_auth()` — the caller must sign. - /// - WaveGuard `is_maintainer` check passes. /// - `maintainer` must match `pool.maintainer` (address equality check). + /// WaveGuard is intentionally NOT re-checked here: clawback is isolated + /// from the WaveGuard registry so that even a compromised registry cannot + /// block the pool creator from recovering their own funds. pub fn clawback_expired_funds(env: Env, maintainer: Address) -> Result<(), Error> { // ── AUTH GATE 1/1: Stellar signature check ── // NOTE: WaveGuard is intentionally NOT re-checked here. Clawback uses @@ -386,10 +388,7 @@ 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. + // ── Address equality only — WaveGuard intentionally bypassed (see doc) ── if maintainer != pool.maintainer { ensure_is_maintainer(&env, &pool.guard_contract, &maintainer)?; return Err(Error::UnauthorizedCaller); diff --git a/contracts/wave_milestone/src/test.rs b/contracts/wave_milestone/src/test.rs index 97e4ac7..b80522b 100644 --- a/contracts/wave_milestone/src/test.rs +++ b/contracts/wave_milestone/src/test.rs @@ -350,6 +350,10 @@ fn test_unauthorized_caller_rejected() { let result = WaveMilestoneContractClient::new(&t.env, &t.contract_id).try_clawback_expired_funds(&t.stranger); assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); +} + +#[test] +fn test_non_maintainer_cannot_create_pool() { let t = setup(); let pool_size: u128 = 10_000_000_000; @@ -573,11 +577,9 @@ fn test_revoked_maintainer_cannot_release_bounty() { assert_eq!(remaining, pool_size); } -/// 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). +/// The pool creator can clawback even after being revoked from WaveGuard. +/// Clawback uses address equality only — WaveGuard is intentionally bypassed +/// so the creator can always recover their own funds. #[test] fn test_revoked_maintainer_cannot_clawback() { let t = setup(); @@ -587,15 +589,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); + // Revoked pool creator can still clawback — WaveGuard is not checked. 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. - assert_eq!(WaveMilestoneContractClient::new(&t.env, &t.contract_id).milestone_balance(), pool_size); + assert_eq!(WaveMilestoneContractClient::new(&t.env, &t.contract_id).milestone_balance(), 0); } /// A second, separately-authorized maintainer (a colluding or rogue diff --git a/contracts/wave_milestone/src/types.rs b/contracts/wave_milestone/src/types.rs index 4662f94..2396d10 100644 --- a/contracts/wave_milestone/src/types.rs +++ b/contracts/wave_milestone/src/types.rs @@ -129,13 +129,13 @@ pub enum Error { BountyAlreadyClaimed = 3, /// The pool's remaining balance is less than the requested bounty amount. InsufficientPoolBalance = 4, - UnauthorizedMaintainer = 5, - UnauthorizedCaller = 6, - NoFundsToClawback = 7, - TransferFailed = 8, - InvalidAmount = 9, - ExpiryInPast = 10, - InvalidGuard = 11, + InvalidGuard = 5, + UnauthorizedMaintainer = 6, + UnauthorizedCaller = 7, + NoFundsToClawback = 8, + TransferFailed = 9, + InvalidAmount = 10, + ExpiryInPast = 11, InvalidRepoHash = 12, InvalidDeveloper = 13, } diff --git a/contracts/wave_milestone/tests/clawback.rs b/contracts/wave_milestone/tests/clawback.rs index 9f21618..f76f8f7 100644 --- a/contracts/wave_milestone/tests/clawback.rs +++ b/contracts/wave_milestone/tests/clawback.rs @@ -59,6 +59,10 @@ fn test_clawback_non_maintainer_rejected() { let result = ctx.client().try_clawback_expired_funds(&ctx.stranger); assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); +} + +#[test] +fn test_clawback_when_pool_empty_rejected() { let ctx = TestContext::new(); let pool_size = DEFAULT_POOL_FUNDS; diff --git a/contracts/wave_milestone/tests/error_enum_coverage.rs b/contracts/wave_milestone/tests/error_enum_coverage.rs index 1113c34..0e630fd 100644 --- a/contracts/wave_milestone/tests/error_enum_coverage.rs +++ b/contracts/wave_milestone/tests/error_enum_coverage.rs @@ -20,7 +20,7 @@ fn test_error_pool_not_found_clawback() { assert_eq!(result.err().unwrap(), Ok(Error::PoolNotFound)); } -// ── PoolNotExpired (2) ─────────────────────────────────────── +// ── ClawbackTooEarly (2) ───────────────────────────────────── #[test] fn test_error_pool_not_expired() { @@ -107,7 +107,7 @@ fn test_error_no_funds_to_clawback() { assert_eq!(result.err().unwrap(), Ok(Error::NoFundsToClawback)); } -// ── TransferFailed (8) ─────────────────────────────────────── +// ── TransferFailed (9) ─────────────────────────────────────── // Not currently returned by the contract (reserved for future use). // Assert the discriminant value is correct so enum layout is pinned. diff --git a/contracts/wave_milestone/tests/release_bounty.rs b/contracts/wave_milestone/tests/release_bounty.rs index 98157f7..bbe0c1e 100644 --- a/contracts/wave_milestone/tests/release_bounty.rs +++ b/contracts/wave_milestone/tests/release_bounty.rs @@ -1,7 +1,6 @@ mod common; use common::*; -use soroban_sdk::Address; use wave_milestone::types::Error; #[test] @@ -89,18 +88,41 @@ fn test_consecutive_bounties_different_issues() { assert_eq!(ctx.client().milestone_balance(), expected_remaining); } -/// Issue #109: Sending a bounty to the all-zero contract address must be -/// rejected before any state mutation or token transfer. +/// Issue #109: Sending a bounty to the contract itself must be rejected — +/// tokens would be trapped with no recovery path. #[test] fn test_release_bounty_zero_developer_rejected() { let ctx = TestContext::new(); ctx.fund_pool(DEFAULT_POOL_FUNDS); - let zero_addr = Address::from_str(&ctx.env, "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM"); - let result = - ctx.client().try_release_issue_bounty(&ctx.maintainer, &ctx.repo_hash, &1u32, &zero_addr, &DEFAULT_BOUNTY); + ctx.client().try_release_issue_bounty(&ctx.maintainer, &ctx.repo_hash, &1u32, &ctx.contract_id, &DEFAULT_BOUNTY); assert_eq!(result.err().unwrap(), Ok(Error::InvalidDeveloper)); assert_eq!(ctx.client().milestone_balance(), DEFAULT_POOL_FUNDS); } + +/// Issue #22: `is_claimed` must return `false` before release and `true` +/// immediately after a successful `release_issue_bounty` call. +#[test] +fn test_is_claimed_true_after_release() { + let ctx = TestContext::new(); + ctx.fund_pool(DEFAULT_POOL_FUNDS); + + // Not yet claimed. + assert!(!ctx.client().is_claimed(&ctx.repo_hash, &1u32)); + + ctx.client().release_issue_bounty( + &ctx.maintainer, + &ctx.repo_hash, + &1u32, + &ctx.developer, + &DEFAULT_BOUNTY, + ); + + // Must be true after a successful release. + assert!(ctx.client().is_claimed(&ctx.repo_hash, &1u32)); + + // A different issue on the same repo must remain unclaimed. + assert!(!ctx.client().is_claimed(&ctx.repo_hash, &2u32)); +} \ No newline at end of file diff --git a/contracts/wave_milestone/tests/unauthorized_access.rs b/contracts/wave_milestone/tests/unauthorized_access.rs index 03c2ab1..8a7d057 100644 --- a/contracts/wave_milestone/tests/unauthorized_access.rs +++ b/contracts/wave_milestone/tests/unauthorized_access.rs @@ -35,7 +35,7 @@ fn test_stranger_cannot_clawback() { let result = ctx.client().try_clawback_expired_funds(&ctx.stranger); - assert_eq!(result.err().unwrap(), Ok(Error::NotPoolMaintainer)); + assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); } #[test] @@ -97,9 +97,8 @@ fn test_removed_maintainer_can_still_clawback() { MockWaveGuardClient::new(&ctx.env, &ctx.guard_id).remove_maintainer(&ctx.maintainer); ctx.advance_to_expiry(); - // Clawback must fail — WaveGuard check is required even for the pool creator. - let result = ctx.client().try_clawback_expired_funds(&ctx.maintainer); - assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedMaintainer)); + // Clawback should still succeed — address equality, not WaveGuard, guards this path. + ctx.client().clawback_expired_funds(&ctx.maintainer); // Funds must remain in the pool. assert_eq!(ctx.client().milestone_balance(), DEFAULT_POOL_FUNDS);