Root + Impact

Description

• Description
  setRecoveryAddress() is guarded only by onlyOwner and a non-zero-address check:

Risk

Likelihood:

• Likelihood: Low intrinsically, since it requires the onlyOwner key to act maliciously or be compromised — the same trust boundary as _authorizeUpgrade. However, given the trivial cost of execution (one transaction, no timing constraints, no detection mechanism) and that it directly contradicts the contract's own stated design ("mutable per its own gates"), this should be treated as a must-fix rather than an accepted centralization risk.

Impact:

• Impact: A direct, owner-controlled fund-redirection path with no time restriction and no dependency on contract state — it works identically whether the pool is UNRESOLVED, mid-CORRUPTED-resolution, or fully claimed-out. Unlike the other onlyOwner findings in this codebase (registry/implementation/moderator setters), which mostly degrade functionality or create narrow DoS windows, this is a straightforward path to diverting real, already-committed user and bonus-contributor funds — potentially the entire pool (stake + bonus) in a bad-faith CORRUPTED resolution.

Proof of Concept

Recommended Mitigation

Mitigation
Lock recoveryAddress once the outcome path that depends on it becomes live:

Locking on outcome != UNRESOLVED is the tightest and safest option — it matches how expiryLocked/scopeLocked are one-way flags tied to a meaningful lifecycle event, and removes any window where a resolved-but-not-yet-fully-claimed pool has a mutable payout destination

function setRecoveryAddress(address newRecoveryAddress) external onlyOwner {
if (newRecoveryAddress == address(0)) revert InvalidRecoveryAddress();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert RecoveryAddressLocked();

address oldRecoveryAddress = recoveryAddress;
recoveryAddress = newRecoveryAddress;

emit RecoveryAddressUpdated(oldRecoveryAddress, newRecoveryAddress);

}

POC

contract RecoveryAddressPoCTest is Test {
address owner = address(0x0WNER);
address legitRecovery = address(0xC0FFEE);
address ownerControlled = address(0xBAD);

/// @notice Core PoC: after the pool is flagged CORRUPTED and reserved funds
/// are snapshotted, the owner redirects recoveryAddress before anyone calls
/// claimCorrupted(). The sweep then sends the entire reserve to the owner's
/// new address instead of the recovery address stakers understood was set.
function test_Bug_OwnerRedirectsFundsMidCorruptedResolution() public {
    RecoveryAddressHarnessBuggy pool = new RecoveryAddressHarnessBuggy(owner, legitRecovery);
    vm.deal(address(pool), 110 ether);

    // Pool resolves CORRUPTED; reserve is snapshotted for sweep.
    pool.flagOutcome(RecoveryAddressHarnessBuggy.Outcome.CORRUPTED, true, address(0xBEEF));
    assertEq(pool.corruptedReserve(), 110 ether);
    assertEq(pool.recoveryAddress(), legitRecovery, "still the legit address, pre-redirect");

    // Owner redirects BEFORE anyone claims. Nothing blocks this: no lock,
    // no dependency on outcome or claimsStarted.
    vm.prank(owner);
    pool.setRecoveryAddress(ownerControlled);

    assertEq(pool.recoveryAddress(), ownerControlled, "redirect succeeded with zero resistance");

    // Sweep now sends the full reserve to the owner-controlled address,
    // not the address stakers understood as the legitimate destination.
    uint256 balBefore = ownerControlled.balance;
    pool.claimCorrupted();

    assertEq(ownerControlled.balance, balBefore + 110 ether, "funds diverted to owner-controlled address");
    assertEq(legitRecovery.balance, 0, "legitimate recovery address received nothing");
}

/// @notice Fixed contract: the same redirect attempt reverts once the
/// outcome has been resolved, closing the window entirely.
function test_Fix_RecoveryAddressLockedOnceOutcomeResolved() public {
    RecoveryAddressHarnessFixed pool = new RecoveryAddressHarnessFixed(owner, legitRecovery);
    vm.deal(address(pool), 110 ether);

    pool.flagOutcome(RecoveryAddressHarnessFixed.Outcome.CORRUPTED, true, address(0xBEEF));

    vm.prank(owner);
    vm.expectRevert(RecoveryAddressHarnessFixed.RecoveryAddressLocked.selector);
    pool.setRecoveryAddress(ownerControlled);

    assertEq(pool.recoveryAddress(), legitRecovery, "unchanged, redirect blocked");

    uint256 balBefore = legitRecovery.balance;
    pool.claimCorrupted();
    assertEq(legitRecovery.balance, balBefore + 110 ether, "funds correctly reach the legitimate address");
}

/// @notice Control: fixed contract still allows the owner to correct the
/// address freely while the pool is UNRESOLVED, confirming the lock is
/// scoped to post-resolution only, not a blanket freeze.
function test_Control_FixedContractAllowsChangeWhileUnresolved() public {
    RecoveryAddressHarnessFixed pool = new RecoveryAddressHarnessFixed(owner, legitRecovery);

    vm.prank(owner);
    pool.setRecoveryAddress(ownerControlled);

    assertEq(pool.recoveryAddress(), ownerControlled, "change allowed pre-resolution");
}

}