Root + Impact

Description

• Describe the normal behavior in one or more sentences

• Explain the specific issue or problem in one or more sentences

Risk

Likelihood:

• Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

• Reason 2

Impact:

• Impact 1

• Impact 2

Proof of Concept

Recommended Mitigation

Severity
High
Likelihood
Medium
Impact
High
Description
_observePoolState() reads the registry state once and routes it through two mutually exclusive branches.

_isActiveRiskState (UNDER_ATTACK / PROMOTION_REQUESTED) and _isTerminalState (PRODUCTION / CORRUPTED) can never both be true for the same state value in a single call. This works correctly if the registry is ever observed passing through an active-risk state before it reaches a terminal one — some earlier call catches UNDER_ATTACK and sets riskWindowStart, so that by the time a later call sees CORRUPTED, riskWindowStart is already nonzero.
Nothing guarantees that sequencing actually happens on-chain, though. If the very first call to _observePoolState() for a given pool sees the registry already in a terminal state — because the attack and its resolution both occurred between two calls, or because no gated function (stake, withdraw, contributeBonus, pokeRiskWindow) was called while the registry was mid-attack — then:
_isActiveRiskState(CORRUPTED) → false → riskWindowStart.

Impact: High — this directly compounds into the fund-misdirection finding already raised on claimExpired(). A CORRUPTED pool that hits this skip path can never satisfy riskWindowStart != 0, so it is permanently routed to the EXPIRED payout path instead of sweeping funds to recoveryAddress, no matter how many more times any function is called afterward.

Mitigation
Back-fill riskWindowStart in the same call that observes a terminal state, if it hasn't already been set:

This ensures riskWindowStart is always set by the time riskWindowEnd is, regardless of whether the registry was observed mid-attack first or jumped straight to a terminal state — closing the desynchronization entirely.

Bad

function _observePoolState() internal {
IAttackRegistry.ContractState state = _getAgreementState();

if (_isActiveRiskState(state)) {
    if (riskWindowStart == 0) _markRiskWindowStart();
}
if (_isTerminalState(state)) {
    if (riskWindowEnd == 0) _markRiskWindowEnd();
}

}

Fixed

function _observePoolState() internal {
IAttackRegistry.ContractState state = _getAgreementState();

if (riskWindowStart == 0 && _isActiveRiskState(state)) {
    _markRiskWindowStart();
}
if (riskWindowStart == 0 && _isTerminalState(state)) {
    _markRiskWindowStart(); // back-fill using the same capped-at-expiry logic
}
if (riskWindowEnd == 0 && _isTerminalState(state)) {
    _markRiskWindowEnd();
}

}

Test

contract ObservePoolStatePoCTest is Test {
MockAttackRegistry registry;
uint32 expiry;

function setUp() public {
    registry = new MockAttackRegistry();
    expiry = uint32(block.timestamp + 7 days);
}

/// @notice Core PoC: the registry jumps straight to CORRUPTED before
/// _observePoolState() is ever called on this pool (nobody staked,
/// withdrew, or poked during the active-risk window). The very first
/// observation sees a terminal state directly, permanently desyncing
/// riskWindowStart (stays 0) from riskWindowEnd (gets set).
function test_Bug_TerminalStateSkipPermanentlyZeroesRiskWindowStart() public {
    ObservePoolStateHarnessBuggy pool = new ObservePoolStateHarnessBuggy(registry, expiry);

    // Registry silently transitions all the way to CORRUPTED off-chain
    // from this pool's perspective — no intermediate call ever observes
    // UNDER_ATTACK or PROMOTION_REQUESTED.
    registry.setState(IAttackRegistry.ContractState.CORRUPTED);

    assertEq(pool.riskWindowStart(), 0, "never set, no prior call observed active-risk state");
    assertEq(pool.riskWindowEnd(), 0, "not yet observed at all");

    // First-ever observation call sees the terminal state directly.
    pool.observePoolState();

    // BUG CONFIRMED: end got set, start did not — permanently inconsistent pair.
    assertEq(pool.riskWindowStart(), 0, "start permanently stuck at 0, no code path can ever set it now");
    assertTrue(pool.riskWindowEnd() != 0, "end WAS set from the same terminal observation");

    // This is the structural root cause of the claimExpired() bug: any
    // logic gated on `riskWindowStart != 0` to confirm CORRUPTED will
    // never fire for this pool, no matter how many more times
    // observePoolState() or claimExpired() is called later.
    vm.warp(block.timestamp + 1 days);
    pool.observePoolState(); // calling again changes nothing
    assertEq(pool.riskWindowStart(), 0, "still stuck, confirming there is no recovery path");
}

/// @notice Control: when the registry DOES pass through an observed
/// active-risk state first (the "expected sequence"), both flags get set
/// correctly and consistently, at the right respective moments.
function test_Control_ExpectedSequenceSetsBothFlagsConsistently() public {
    ObservePoolStateHarnessBuggy pool = new ObservePoolStateHarnessBuggy(registry, expiry);

    registry.setState(IAttackRegistry.ContractState.UNDER_ATTACK);
    pool.observePoolState();
    uint32 observedStart = pool.riskWindowStart();
    assertTrue(observedStart != 0, "start correctly set while active-risk");
    assertEq(pool.riskWindowEnd(), 0, "end not yet set, still active-risk");

    vm.warp(block.timestamp + 1 hours);
    registry.setState(IAttackRegistry.ContractState.CORRUPTED);
    pool.observePoolState();

    assertEq(pool.riskWindowStart(), observedStart, "start unchanged, already set earlier");
    assertTrue(pool.riskWindowEnd() != 0, "end correctly set now that terminal state is observed");
}

/// @notice Fixed contract: the exact same "jump straight to CORRUPTED,
/// never observed mid-attack" scenario as the core PoC, but the patched
/// _observePoolState() back-fills riskWindowStart in the same call that
/// sets riskWindowEnd, so the pair never desynchronizes.
function test_Fix_TerminalObservationBackFillsStartInSameCall() public {
    ObservePoolStateHarnessFixed pool = new ObservePoolStateHarnessFixed(registry, expiry);

    registry.setState(IAttackRegistry.ContractState.CORRUPTED);

    assertEq(pool.riskWindowStart(), 0);
    assertEq(pool.riskWindowEnd(), 0);

    pool.observePoolState();

    // FIX CONFIRMED: both get set together, from the same terminal observation.
    assertTrue(pool.riskWindowStart() != 0, "back-filled in the same call");
    assertTrue(pool.riskWindowEnd() != 0, "set as before");
    assertEq(pool.riskWindowStart(), pool.riskWindowEnd(), "both stamped at the same observation timestamp");
}

}