BattleChain Confidence Pools
Cyfrin
FoundrySolidityLayer 2
7.25 ETH
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: low

Deposit gate ignores the one-way `riskWindowStart` latch that guards `withdraw`, so a benign registry rewind admits new principal that can never be withdrawn and is swept on CORRUPTED

Author Revealed upon completion

Root + Impact

Confidence: High
​
Affected functions: `stake`, `contributeBonus`, `_assertDepositsAllowed`, `_observePoolState`,
`withdraw`, `_clampUserSums`, `_bonusShare`, `claimCorrupted`, `claimExpired`
​
Affected actors: Any user who deposits after a benign upstream registry rewind; their principal
is redirected to the sponsor-selected `recoveryAddress` (bad-faith CORRUPTED) or to the named
attacker (good-faith CORRUPTED)

Description

#### Normal behavior
​
Once a pool observes the registry in an active-risk state, `riskWindowStart` is sealed to a nonzero
value and never reset — it is the pool's one-way, on-chain record that "risk has materialized."
`withdraw()` gates on that latch: it exits the caller's principal only while
`riskWindowStart == 0` **and** the live registry is still in a pre-attack state
(`NOT_DEPLOYED`, `NEW_DEPLOYMENT`, or `ATTACK_REQUESTED`). Combining the latch with the live-state
read is deliberate. `docs/DESIGN.md §9` and `§11` state the intent explicitly: *"an upstream
registry rewind cannot re-open"* withdrawals, because the exit is gated on the latch, "not solely
on live state." `§11` further promises that under a benign rewind, "the worst case is a recoverable
liveness delay or principal *returned* to stakers rather than swept."
​
The deposit surface is supposed to inherit that same protection. `docs/DESIGN.md §3` justifies
allowing deposits during active risk by telling stakers they self-protect: *"A staker who does not
want this can read the live registry state before staking."* `§9` promises symmetric fairness:
*"stakers can freely exit until risk materializes."*
​
#### Specific issue
​
`withdraw()` and the deposit paths use **different** definitions of "risk has materialized," and
they disagree after a rewind:
​
- `withdraw()` reads the one-way latch: `riskWindowStart != 0` ⇒ risk materialized ⇒ exit blocked.
- `stake()` and `contributeBonus()` read only the **live** registry state via
`_assertDepositsAllowed(_observePoolState())`. They never consult `riskWindowStart`.
​
When the trusted registry benignly rewinds — e.g. the protocol DAO migrates the attack-registry
pointer to a fresh `AttackRegistry` that reports the agreement in a pre-risk state during the
migration window — the two surfaces split:
​
- `withdraw()` correctly stays latched shut (`riskWindowStart != 0`).
- `stake()` / `contributeBonus()` re-open, because the live state now reads `NOT_DEPLOYED` /
`NEW_DEPLOYMENT` and `_assertDepositsAllowed` only blocks `PROMOTION_REQUESTED` / `PRODUCTION` /
`CORRUPTED`.
​
A depositor entering in that window reads exactly the live state that `§3`/`§9` told them
guarantees an exit hatch — but the hidden latch silently denies it. Worse, `_clampUserSums` floors
their entry time to `riskWindowStart`, collapsing their k=2 score to zero, so they earn **no bonus**
to offset the risk they unknowingly took on. When the registry escapes back to `CORRUPTED`, their
principal is swept to `recoveryAddress` (bad-faith) or the attacker (good-faith) — the exact "swept,
not returned" outcome `§11` promises a benign rewind cannot cause.// Root cause in the codebase with @> marks to highlight the relevant section
​
### Invariant violated
​
Two DESIGN promises are broken by the same asymmetry:
​
1. **`§11` benign-rewind guarantee** — under a benign upstream rewind, "the worst case is … principal
*returned* to stakers rather than swept." A post-rewind depositor's principal is *swept*, not
returned.
2. **`§9` exit guarantee** — "stakers can freely exit until risk materializes." A post-rewind
depositor cannot exit at all, even though every observable live-registry signal (`NOT_DEPLOYED` /
`NEW_DEPLOYMENT`) says risk has **not** materialized.
​
The underlying invariant: the deposit surface and the withdraw surface must agree on whether risk has
materialized. `withdraw` uses the durable latch; the deposit gate must too. It does not.

Risk

The deposit gate reads live state only, while the withdraw gate additionally reads the one-way latch.
​
```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; the latch is never read
...
}
​
// contributeBonus() — src/ConfidencePool.sol:266-271
function contributeBonus(uint256 amount) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert StakingClosed();
_assertDepositsAllowed(_observePoolState()); // @> same: live state only, no latch
...
}
​
// withdraw() — src/ConfidencePool.sol:288-300
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
// `riskWindowStart` is the pool's one-way record that risk has materialised;
// gate on it so an upstream registry rewind cannot re-open withdrawals.
if (
riskWindowStart != 0 // @> the durable latch
|| (state != IAttackRegistry.ContractState.NOT_DEPLOYED
&& state != IAttackRegistry.ContractState.NEW_DEPLOYMENT
&& state != IAttackRegistry.ContractState.ATTACK_REQUESTED)
) {
revert WithdrawsDisabled();
}
...
}
```
​
`_assertDepositsAllowed` blocks only the three states below — never a rewound pre-risk state, and
never on the latch:
​
```solidity
// _assertDepositsAllowed() — src/ConfidencePool.sol:727-734
function _assertDepositsAllowed(IAttackRegistry.ContractState state) private pure {
if (
state == IAttackRegistry.ContractState.PROMOTION_REQUESTED
|| state == IAttackRegistry.ContractState.PRODUCTION
|| state == IAttackRegistry.ContractState.CORRUPTED
) {
revert StakingClosed();
}
}
```
​
The live read resolves the attack-registry pointer every call, so a repointed/rewound registry is
observed immediately:
​
```solidity
// _getAgreementState() — src/ConfidencePool.sol:740-744
function _getAgreementState() internal view returns (IAttackRegistry.ContractState) {
address attackRegistry = safeHarborRegistry.getAttackRegistry(); // @> live pointer, never cached
if (attackRegistry == address(0)) revert InvalidAgreement();
return IAttackRegistry(attackRegistry).getAgreementState(agreement);
}
```
​
The post-rewind deposit's entry time is then floored to `riskWindowStart`, zeroing its bonus:
​
```solidity
// _clampUserSums() — src/ConfidencePool.sol:677-685
function _clampUserSums(address u) internal {
uint256 start = riskWindowStart;
uint256 stake_ = eligibleStake[u];
if (start == 0 || stake_ == 0) return;
if (userSumStakeTime[u] < stake_ * start) {
userSumStakeTime[u] = stake_ * start; // @> (T − entry)² collapses to 0
userSumStakeTimeSq[u] = stake_ * start * start;
}
}
```
​
The fix belongs on the deposit side: add the `riskWindowStart != 0` latch to `stake` and
`contributeBonus`, mirroring `withdraw`.
### Attack path
​
1. A user stakes into a pool while the registry reports `NOT_DEPLOYED` / `NEW_DEPLOYMENT`.
2. The registry advances to `UNDER_ATTACK`. Anyone calls `pokeRiskWindow()` (permissionless),
sealing `riskWindowStart != 0`. `withdraw` is now permanently latched shut.
3. The protocol DAO migrates the `SafeHarborRegistry` attack-registry pointer to a fresh
`AttackRegistry`. During the migration window the new registry reports this agreement in a
pre-risk state (`NOT_DEPLOYED`) before it is re-registered — a benign rewind, the exact scenario
`docs/DESIGN.md §11` contemplates.
4. `stake()` and `contributeBonus()` re-open: `_assertDepositsAllowed(_observePoolState())` sees the
rewound live state and admits deposits. `withdraw()` still reverts `WithdrawsDisabled` because
`riskWindowStart != 0`.
5. A new depositor — reading the live registry exactly as `§3` instructs — stakes. `_clampUserSums`
floors their entry to `riskWindowStart`, so `_bonusShare` yields zero bonus, and they cannot
withdraw.
6. The agreement's lifecycle resumes on the new registry and reaches `CORRUPTED`. The moderator
flags `flagOutcome(CORRUPTED, goodFaith=false, attacker=0)`, or — after
`expiry + MODERATOR_CORRUPTED_GRACE` — anyone triggers the same via `claimExpired` (the latch is
still nonzero, satisfying the auto-CORRUPTED gate at `src/ConfidencePool.sol:532`). The
post-rewind depositor's principal is included in `snapshotTotalStaked`
(`src/ConfidencePool.sol:357`) and swept to `recoveryAddress` by `claimCorrupted`
(`src/ConfidencePool.sol:408-426`). In the good-faith case it is instead paid to the named
attacker via `claimAttackerBounty`.
​
No malicious registry, moderator compromise, or nonstandard token behavior is required — only a
legitimate DAO-triggered registry migration and an ordinary depositor acting on the live registry
state that DESIGN tells them to trust.
​
### Why this is not an accepted DESIGN.md behavior
​
This is **not** the accepted "deposits during `UNDER_ATTACK`" behavior of `§3`. There, the depositor
reads a live `UNDER_ATTACK` state, *knows* risk has materialized, and knowingly accepts a
near-zero-reward, self-locking position — "voluntary, visible risk." Here the depositor reads a live
`NOT_DEPLOYED` / `NEW_DEPLOYMENT` state, which `§3` and `§9` present as the safe pre-risk window with
a guaranteed exit hatch. The registry lies to the depositor about whether risk has materialized, and
the deposit gate — unlike `withdraw` — believes the lie. `§3`'s stated self-protection ("read the
live registry state before staking") is precisely the mechanism that fails.
​
It is **not** the accepted `§7` bonus-timing residual either. That residual only "redistributes the
fixed bonus pot" among stakers. This finding moves **principal** — swept to `recoveryAddress` or the
attacker — which falls outside every accepted-residual clause.
​
And it is **not** merely the trusted-singleton assumption of `§11`. The finding does not require the
registry to be malicious or to report *false* state. It uses the *benign* rewind that `§11` itself
names and claims to have neutralized ("a benign upstream state rewind cannot re-open withdraw"),
then shows the neutralization is one-sided: the latch protects `withdraw` but the deposit gate never
reads it, so the `§11` "principal returned, not swept" guarantee is broken through the deposit door
that `§11` left open.

Likelihood:

- Requires the `SafeHarborRegistry` DAO to migrate the underlying `AttackRegistry` while a pool is
mid-risk, with a window in which the fresh registry reports the agreement pre-risk before
re-registration. This is a documented, in-scope operation (`§9`/`§11`) but not a routine one, and
the pre-risk window is narrow — this is what holds the finding at Low, not High.
- No other privileged action is required. After the rewind, any ordinary depositor can enter and be
trapped through permissionless public calls.
- Partial mitigant a judge will raise: `riskWindowStart` is a `public` variable, so an exceptionally
careful depositor could read it and abstain. But DESIGN's own guidance (`§3`, `§9`) frames the
exit/risk decision in terms of **live registry state**, not this latch, so a depositor following
the documented self-protection is still caught. The code inconsistency — two surfaces disagreeing
on whether risk materialized — is the root defect regardless.
​

Impact:

- **Principal loss, not bonus redistribution.** The post-rewind depositor's principal is swept to
`recoveryAddress` on bad-faith `CORRUPTED`, or diverted to the attacker's bounty on good-faith
`CORRUPTED`.
- **No bonus offset.** `_clampUserSums` floors the depositor's entry to `riskWindowStart`, so
`_bonusShare` returns zero. They bear the full downside with zero compensation.
- **No exit.** `withdraw` remains latched shut post-rewind (correctly), and no other staker-side
recovery path exists. Official pool clones are non-upgradeable — the loss is final.

Proof of Concept

### Preconditions and actor permissions
​
| Actor | Permission used |
| --- | --- |
| Early staker | Ordinary staker who deposits pre-rewind and seals the risk window via `pokeRiskWindow` |
| Post-rewind depositor | Ordinary staker; victim who deposits after the benign rewind |
| Registry DAO | Trusted; performs only a documented `setAttackRegistry` migration to a fresh registry |
| Outcome moderator | Flags the eventual `CORRUPTED` outcome (or auto-resolution via `claimExpired`) |
| Sponsor | Selected `recoveryAddress`; receives the swept principal under bad-faith CORRUPTED |
| Arbitrary callers | `pokeRiskWindow`, `claimExpired`, `claimCorrupted` are permissionless |
​
function test_depositGateRewindTrapsPostRewindPrincipal() external {
// 1. Early staker deposits pre-risk.
_stake(alice, 100 * ONE);
​
// 2. Registry -> UNDER_ATTACK; seal the one-way latch.
_enterUnderAttack(); // NOT_DEPLOYED -> ATTACK_REQUESTED -> UNDER_ATTACK
pool.pokeRiskWindow();
assertTrue(pool.riskWindowStart() != 0);
​
// 3. Benign DAO migration: repoint safeHarborRegistry to a fresh AttackRegistry that reports
// this agreement NOT_DEPLOYED during the migration window.
AttackRegistry fresh = _deployFreshAttackRegistry(); // reports NOT_DEPLOYED for `agreement`
vm.prank(registryDao);
safeHarborRegistry.setAttackRegistry(address(fresh)); // documented §9/§11 operation
assertEq(
uint256(fresh.getAgreementState(address(agreement))),
uint256(IAttackRegistry.ContractState.NOT_DEPLOYED)
);
assertTrue(pool.riskWindowStart() != 0); // latch survives the rewind
​
// 4. Deposit path re-opens, withdraw stays shut.
assertTrue(_tryStake(bob, 100 * ONE)); // stake() succeeds post-rewind
vm.prank(bob);
vm.expectRevert(WithdrawsDisabled.selector);
pool.withdraw(); // withdraw() still reverts
​
// 5. Agreement resumes on the fresh registry and reaches CORRUPTED; resolve + sweep.
_reachCorruptedOn(fresh);
vm.prank(outcomeModerator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
uint256 recoveryBefore = token.balanceOf(recovery);
pool.claimCorrupted();
​
// 6. Bob's principal was swept to recovery; bonus was zero; withdraw was never available.
assertEq(token.balanceOf(recovery) - recoveryBefore, 200 * ONE);
assertEq(token.balanceOf(address(pool)), 0);
assertEq(pool.eligibleStake(bob), 0);
}
```
​
A negative control (no rewind ⇒ `stake` reverts `StakingClosed` once the live state is terminal, and
the pool resolves via the normal path) should accompany it, plus a `contributeBonus` variant showing
the sibling deposit path re-opens identically.
​
### Expected result (from code analysis, pending an executed run)
​
| Observation | Expected |
| --- | --- |
| `riskWindowStart` after the rewind | nonzero (one-way latch, never reset) |
| `withdraw()` post-rewind | reverts `WithdrawsDisabled` |
| `stake()` / `contributeBonus()` post-rewind | succeed |
| Post-rewind depositor bonus | `0` (entry floored to `riskWindowStart`) |
| Final outcome | `CORRUPTED` |
| Post-rewind principal | swept to `recoveryAddress` (bad-faith) or attacker (good-faith) |
| Post-rewind depositor recovered principal | `0` |
​

Recommended Mitigation

Add the `riskWindowStart != 0` latch to both deposit paths, mirroring `withdraw`. Fixing only one
side leaves the sibling path exploitable.
​
```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();
+ // Mirror withdraw's one-way latch: once risk has materialised, an upstream registry
+ // rewind must not re-open deposits any more than it re-opens withdrawals.
+ 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 StakingClosed();
+ if (riskWindowStart != 0) revert StakingClosed();
_assertDepositsAllowed(_observePoolState());
...
}
```
​
This restores the `§9`/`§11` invariant that the deposit and withdraw surfaces agree on whether risk
has materialized, and it makes the benign-rewind guarantee ("principal returned, not swept") hold on
both sides of the gate.

Support

FAQs

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

Give us feedback!