FoundrySolidityLayer 2
7.25 ETH
Submission Details
Impact: high
Likelihood: low

Deposit gate lacks the one-way `riskWindowStart` latch that protects `withdraw`, letting a benign registry rewind re-open the deposit path and freeze the depositor's principal

Author Revealed upon completion

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

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

// Root cause in the codebase with @> marks to highlight the relevant section
**Affected functions:** `stake`, `contributeBonus`, `_assertDepositsAllowed`, `withdraw`, `_clampUserSums`, `_bonusShare`, `claimSurvived`, `claimCorrupted`, `sweepUnclaimedBonus`
**Affected actors:** Any user who deposits after a benign registry rewind
### Description
`stake()` and `contributeBonus()` gate deposits only on the live `_observePoolState()` reading. They do not check the one-way `riskWindowStart != 0` latch that `withdraw()` uses. When the trusted registry benignly rewinds from `UNDER_ATTACK` back to a pre-risk state, the deposit path re-opens. `docs/DESIGN.md §11` frames such rewinds as harmless on the basis that the latch protects the user-facing surface, but the latch only guards `withdraw`. A depositor entering after the rewind receives no bonus (their entry time is later floored to `riskWindowStart`, collapsing the k=2 score to zero) and cannot exit. Principal is trapped.
```solidity
// stake() -- src/ConfidencePool.sol:222-227
function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount < minStake) revert BelowMinStake();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert StakingClosed();
_assertDepositsAllowed(_observePoolState()); // @> live state only, latch not read
```
```solidity
// withdraw() -- src/ConfidencePool.sol:288-299
if (
riskWindowStart != 0 // @> the latch
|| (state != IAttackRegistry.ContractState.NOT_DEPLOYED
&& state != IAttackRegistry.ContractState.NEW_DEPLOYMENT
&& state != IAttackRegistry.ContractState.ATTACK_REQUESTED)
) {
revert WithdrawsDisabled();
}
```
`withdraw` combines the latch with the live-state check. `stake` and `contributeBonus` use only the live-state check. If the registry moves from `UNDER_ATTACK` back to `NEW_DEPLOYMENT` (a rewind), `_assertDepositsAllowed` accepts new deposits, but `withdraw` still reverts with `WithdrawsDisabled` because the latch is set. The post-rewind depositor's entry time is later floored to `riskWindowStart` via `_clampUserSums` (`:677`), so `(T − entry)² = 0` and `_bonusShare` (`:696`) returns zero. No bonus offsets the trapped principal.
### Invariant violated
`docs/DESIGN.md §11` states the invariant: once risk has been observed, the pool's user-facing entry and exit surface is one-way locked to resolution. The `riskWindowStart != 0` latch is what enforces this. The deposit gate violates the invariant by re-opening `stake` and `contributeBonus` after a rewind while `withdraw` correctly remains latched shut.
### Attack path
1. A user stakes into a pool while the registry is in `NOT_DEPLOYED` or `NEW_DEPLOYMENT`.
2. The registry advances to `UNDER_ATTACK`. Anyone calls `pokeRiskWindow`, sealing `riskWindowStart != 0`.
3. The SafeHarborRegistry owner (DAO) migrates to a fresh `AttackRegistry` reporting a pre-risk state for this agreement. The migration is an in-scope operation (`BattleChainSafeHarborRegistry.sol:123`; `docs/DESIGN.md §9 / §11`).
4. `stake()` and `contributeBonus()` re-open because `_assertDepositsAllowed(_observePoolState())` sees the rewound state. `withdraw()` remains latched shut because `riskWindowStart != 0`.
5. A new depositor stakes. `_clampUserSums` floors their entry time to `riskWindowStart`, so `_bonusShare` short-circuits to zero.
6. The registry later escapes to `CORRUPTED`. The moderator flags `CORRUPTED, goodFaith=false, attacker=0`, or auto-resolution takes the same route via `claimExpired`. The post-rewind depositor's principal is included in `snapshotTotalStaked` and swept to `recoveryAddress` (`ConfidencePool.sol:361`, `:456-471`). In the good-faith case it is instead diverted to the attacker.
No malicious registry, moderator compromise, or nonstandard token behavior is required. Only a legitimate DAO-triggered registry migration.
### Why this is not an accepted DESIGN.md behavior
`docs/DESIGN.md §11` frames benign registry rewinds as harmless because the `riskWindowStart != 0` latch protects the user-facing surface. §11 further states that in the worst case, principal is returned to stakers rather than swept. Both claims are violated. The deposit surface re-opens (contradicting the "protected" claim) and the post-rewind depositor's principal is swept, not returned (contradicting the worst-case guarantee). At deposit time the live registry state reads `NEW_DEPLOYMENT` or `NOT_DEPLOYED`, indistinguishable from a fresh pool.

Risk

Likelihood:

### Likelihood: Low
Requires the SafeHarborRegistry owner (DAO) to migrate the underlying `AttackRegistry` while a pool is mid-risk. The migration itself is a documented in-scope operation but not a routine one.
No other privileged action is required. After the rewind, any ordinary depositor can enter and be trapped through public calls.
The rewind window persists indefinitely. Every deposit made after the rewind and before the registry re-advances to a terminal state is affected.

Impact:

Principal loss, not bonus redistribution. The post-rewind depositor's principal is included in `snapshotTotalStaked` and swept to `recoveryAddress` on bad-faith `CORRUPTED`, or diverted into the attacker's bounty on good-faith `CORRUPTED`. `docs/DESIGN.md §7` permits only "redistributes bonus among stakers" as an accepted residual, so principal loss falls outside every accepted-residual clause.
No bonus offset. `_clampUserSums` floors the depositor's entry time to `riskWindowStart`, so `_bonusShare` returns zero. The depositor pays the full loss with no compensation.
No exit. `withdraw` remains latched shut post-rewind (correctly), and no other staker-side recovery path exists. Official pool clones are non-upgradeable.

Proof of Concept

Executable tests, run against unmodified `src/ConfidencePool.sol`:
| Scenario | Test | Command |
| --- | --- | --- |
| Post-rewind `stake` traps principal on CORRUPTED | `test/audit/AuditF1_DepositGateRewind.t.sol::test_F1_stakePostRewindLosesPrincipal` | `forge test --match-test test_F1_stakePostRewindLosesPrincipal -vvvv` |
| Post-rewind `contributeBonus` also re-opens | `test/audit/AuditF1_DepositGateRewind.t.sol::test_F1_contributeBonusReopensAfterRewind` | `forge test --match-test test_F1_contributeBonusReopensAfterRewind -vvvv` |
| Negative control: no rewind, deposits stay closed | `test/audit/AuditF1_DepositGateRewind.t.sol::test_F1_negativeControl_noRewindMeansGateStaysClosed` | `forge test --match-test test_F1_negativeControl_noRewindMeansGateStaysClosed -vvvv` |
The primary test seals the risk window under `UNDER_ATTACK`, performs a documented in-scope registry migration to a fresh `AttackRegistry` reporting `NOT_DEPLOYED`, verifies `riskWindowStart != 0` still holds, then shows `stake()` succeeds while `withdraw()` reverts with `WithdrawsDisabled`. On subsequent `CORRUPTED` resolution the post-rewind depositor's principal is swept to `recoveryAddress`.
- create a new file under test/audit and name it `AuditF1_DepositGateRewind.t.sol`
- Add the test
```solidty
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
import {MockAttackRegistry} from "test/mocks/MockAttackRegistry.sol";
// Deposit gate reads live registry state only. After a benign registry rewind, stake() and
// contributeBonus() re-open even though withdraw() correctly stays latched by riskWindowStart != 0.
contract AuditF1_DepositGateRewindTest is BaseConfidencePoolTest {
function test_F1_stakePostRewindLosesPrincipal() external {
_stake(alice, 100 * ONE);
_passThroughUnderAttack();
uint32 sealedStart = pool.riskWindowStart();
assertGt(sealedStart, 0, "risk window sealed");
// DAO swaps AttackRegistry to a fresh one reporting NOT_DEPLOYED.
MockAttackRegistry rewound = new MockAttackRegistry();
rewound.setAgreementState(IAttackRegistry.ContractState.NOT_DEPLOYED);
safeHarborRegistry.setAttackRegistry(address(rewound));
assertEq(pool.riskWindowStart(), sealedStart, "latch is one-way; rewind cannot reset it");
_stake(bob, 100 * ONE);
assertEq(pool.eligibleStake(bob), 100 * ONE, "deposit gate re-opened after rewind");
vm.prank(bob);
vm.expectRevert(IConfidencePool.WithdrawsDisabled.selector);
pool.withdraw();
rewound.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
uint256 recoveryBefore = token.balanceOf(recovery);
pool.claimCorrupted();
assertEq(
token.balanceOf(recovery) - recoveryBefore,
200 * ONE,
"post-rewind principal swept to recovery"
);
assertEq(token.balanceOf(bob), 0, "bob recovers nothing");
}
function test_F1_negativeControl_noRewindMeansGateStaysClosed() external {
_stake(alice, 100 * ONE);
_passThroughUnderAttack();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.PROMOTION_REQUESTED);
token.mint(bob, 100 * ONE);
vm.startPrank(bob);
token.approve(address(pool), 100 * ONE);
vm.expectRevert(IConfidencePool.StakingClosed.selector);
pool.stake(100 * ONE);
vm.stopPrank();
}
function test_F1_contributeBonusReopensAfterRewind() external {
_stake(alice, 100 * ONE);
_passThroughUnderAttack();
MockAttackRegistry rewound = new MockAttackRegistry();
rewound.setAgreementState(IAttackRegistry.ContractState.NOT_DEPLOYED);
safeHarborRegistry.setAttackRegistry(address(rewound));
_contributeBonus(carol, 25 * ONE);
assertEq(pool.totalBonus(), 25 * ONE, "contributeBonus re-opened via the same gate");
}
}
```
```bash
forge test --match-path test/audit/AuditF1_DepositGateRewind.t.sol -vvvv
```
### Observed result
```text
[PASS] test_F1_contributeBonusReopensAfterRewind()
[PASS] test_F1_negativeControl_noRewindMeansGateStaysClosed()
[PASS] test_F1_stakePostRewindLosesPrincipal()
Suite result: ok. 3 passed; 0 failed; 0 skipped
```

Recommended Mitigation

## Recommended Mitigation
Add the `riskWindowStart != 0` latch to both deposit paths, mirroring `withdraw`.
```diff
function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount < minStake) revert BelowMinStake();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert StakingClosed();
+ if (riskWindowStart != 0) revert StakingClosed();
_assertDepositsAllowed(_observePoolState());
}
function contributeBonus(uint256 amount) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert BonusClosed();
+ if (riskWindowStart != 0) revert BonusClosed();
_assertDepositsAllowed(_observePoolState());
}
```
This restores the DESIGN.md #11 invariant. Fixing only one side of the gate leaves the sibling path exploitable.
### Regression properties
Pre-risk deposits (registry never left `NOT_DEPLOYED`, `NEW_DEPLOYMENT`, or `ATTACK_REQUESTED`, `riskWindowStart == 0`) continue to succeed.
Post-rewind deposits revert atomically before any transfer or state change.
`withdraw` behavior is unchanged.
`sweepUnclaimedBonus` sees no change in the `riskWindowStart == 0` branch; the latch is additive on the deposit side only.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!