diff --git a/contracts/wave_milestone/src/lib.rs b/contracts/wave_milestone/src/lib.rs index c20a6b3..b1038bd 100644 --- a/contracts/wave_milestone/src/lib.rs +++ b/contracts/wave_milestone/src/lib.rs @@ -388,7 +388,10 @@ impl WaveMilestoneContract { .get::<_, MilestonePool>(&DataKey::Pool) .ok_or(Error::PoolNotFound)?; - // ── Address equality only — WaveGuard intentionally bypassed (see doc) ── + // ── Ownership check (no WaveGuard re-check — see trust assumptions in MilestonePool) ── + // Clawback is restricted to the exact pool creator by address equality. + // WaveGuard is intentionally NOT consulted here to prevent a compromised + // or revoked WaveGuard registry from locking the pool creator out of their funds. 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 b80522b..9f3d6f0 100644 --- a/contracts/wave_milestone/src/test.rs +++ b/contracts/wave_milestone/src/test.rs @@ -97,18 +97,19 @@ fn setup() -> TestEnv { let env = Env::default(); env.mock_all_auths(); + // Register contracts first so that Address::generate produces non-zero addresses + // (contract registration consumes address slots starting at 0). + let guard_id = env.register(MockWaveGuard, ()); + let token_id = env.register(MockToken, ()); + let contract_id = env.register(WaveMilestoneContract, ()); + let maintainer = Address::generate(&env); let developer = Address::generate(&env); let stranger = Address::generate(&env); - let guard_id = env.register(MockWaveGuard, ()); MockWaveGuardClient::new(&env, &guard_id).add_maintainer(&maintainer); - - let token_id = env.register(MockToken, ()); MockTokenClient::new(&env, &token_id).init(&maintainer); - let contract_id = env.register(WaveMilestoneContract, ()); - let repo_hash = BytesN::from_array(&env, &[1u8; 32]); let expiry = env.ledger().timestamp() + 2_592_000; @@ -349,6 +350,7 @@ fn test_unauthorized_caller_rejected() { let result = WaveMilestoneContractClient::new(&t.env, &t.contract_id).try_clawback_expired_funds(&t.stranger); + // Clawback uses pool.maintainer address equality (not WaveGuard) — non-owner gets UnauthorizedCaller. assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); } @@ -577,9 +579,9 @@ fn test_revoked_maintainer_cannot_release_bounty() { assert_eq!(remaining, pool_size); } -/// 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. +/// A maintainer removed from WaveGuard can still claw back their own pool. +/// Clawback intentionally bypasses WaveGuard to isolate fund recovery from a +/// potential WaveGuard compromise (pool.maintainer address equality is the guard). #[test] fn test_revoked_maintainer_cannot_clawback() { let t = setup(); @@ -589,9 +591,10 @@ 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); - // Revoked pool creator can still clawback — WaveGuard is not checked. + 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!(WaveMilestoneContractClient::new(&t.env, &t.contract_id).milestone_balance(), 0); } diff --git a/contracts/wave_milestone/tests/clawback.rs b/contracts/wave_milestone/tests/clawback.rs index f76f8f7..78ee38c 100644 --- a/contracts/wave_milestone/tests/clawback.rs +++ b/contracts/wave_milestone/tests/clawback.rs @@ -58,6 +58,7 @@ fn test_clawback_non_maintainer_rejected() { let result = ctx.client().try_clawback_expired_funds(&ctx.stranger); + // Clawback uses pool.maintainer address equality (WaveGuard bypassed) → UnauthorizedCaller. assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); } diff --git a/contracts/wave_milestone/tests/error_enum_coverage.rs b/contracts/wave_milestone/tests/error_enum_coverage.rs index 0e630fd..6e14642 100644 --- a/contracts/wave_milestone/tests/error_enum_coverage.rs +++ b/contracts/wave_milestone/tests/error_enum_coverage.rs @@ -85,8 +85,8 @@ 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. + // Register stranger as a WaveGuard maintainer so the WaveGuard check passes, + // but stranger is not the pool owner — must get UnauthorizedCaller. 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 diff --git a/contracts/wave_milestone/tests/unauthorized_access.rs b/contracts/wave_milestone/tests/unauthorized_access.rs index 8a7d057..a0d37f5 100644 --- a/contracts/wave_milestone/tests/unauthorized_access.rs +++ b/contracts/wave_milestone/tests/unauthorized_access.rs @@ -35,6 +35,7 @@ fn test_stranger_cannot_clawback() { let result = ctx.client().try_clawback_expired_funds(&ctx.stranger); + // Clawback uses pool.maintainer address equality (WaveGuard bypassed) → UnauthorizedCaller. assert_eq!(result.err().unwrap(), Ok(Error::UnauthorizedCaller)); }