BattleChain Confidence Pools
Cyfrin
FoundrySolidityLayer 2
7.25 ETH
View all submissions
Previous Next
Submission Details
Severity: low
Valid

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.
Updates

Lead Judging Commences

inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Validated
Assigned finding tags:

stake() ignores the riskWindowStart latch withdraw() enforces, letting a staker deposit into an already risk-sealed pool that still displays pre-attack state, with no informed consent

Impact - Low The depositor loses the exit the moment they enter, and if the agreement later settles CORRUPTED their principal is swept along with everyone else's. What they read beforehand said the opposite: DESIGN.md #3 tells stakers to check the live registry state, and after a rewind that state reports pre-attack while the latch behind it is already sealed. They still earn an ordinary late-entrant bonus, so what they lose is the exit and the exposure that comes with it. Likelihood - Low Reaching the split needs the attack-registry pointer migrated while a pool is mid-risk, in the window before the fresh registry has re-registered the agreement. Nothing else is privileged, and once the live state reads pre-attack any ordinary depositor walks in through public calls. Cutting the other way, riskWindowStart is a public variable, so anyone who checks the pool instead of the registry is safe.

Support

FAQs

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

Give us feedback!