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

Missing `attackApproved` check conflates DAO bypass with unobserved risk

Author Revealed upon completion

Root + Impact

Description

_bonusShare() pays zero bonus when riskWindowStart == 0, on the reasoning that "no observable risk → no bonus" i.e the pool never saw the registry in an active-risk state (UNDER_ATTACK/PROMOTION_REQUESTED), so there is nothing to time-weight against, and the entire bonus pot is instead swept to recoveryAddress via sweepUnclaimedBonus(). This is meant to withhold reward when stakers were never actually exposed to risk.

function _bonusShare(address u, uint256 userEligible) internal view returns (uint256) {
if (snapshotTotalBonus == 0) return 0;
// No observable risk → no bonus.
// @audit-issue This single, pool-local, timing-dependent flag is the ONLY signal used to decide
// "was there ever real risk". It cannot tell a genuine DAO bypass apart from a
// genuine attack window that this pool simply never happened to observe.
@> if (riskWindowStart == 0) return 0;
// ...
}

Specific issue:

riskWindowStart == 0 is used as a proxy for "no risk ever existed," but it only actually means "no transaction on this pool happened to observe an active-risk state."
The pool never queries the registry's own persistent AgreementInfo.attackApproved flag (readable via getAttackRegistry().getAgreementInfo(agreement)), which is set permanently the moment the DAO calls approveAttack() and stays true forever regardless of the agreement's later state.

s_agreementInfo[agreementAddress].attackApproved = true;

So the pool only calls this:

function _getAgreementState() internal view returns (IAttackRegistry.ContractState) {
address attackRegistry = safeHarborRegistry.getAttackRegistry();
if (attackRegistry == address(0)) revert InvalidAgreement();
// @audit-issue Only ever reads the current computed enum. Never calls
// `getAgreementInfo(agreement)`, so `AgreementInfo.attackApproved`.
// A persistent, registry-native record of whether this agreement was
// EVER actually approved into UNDER_ATTACK is never consulted.
@> return IAttackRegistry(attackRegistry).getAgreementState(agreement);
}

As a result, the contract cannot distinguish two entirely different situations, and pays zero bonus in both:

  1. The DAO bypassed battle-testing entirely (instantPromote() called directly from ATTACK_REQUESTED, attackApproved stays false) — no real risk period ever existed for any pool watching this agreement.

  2. The agreement genuinely passed through UNDER_ATTACK (attackApproved == true, real risk existed and stakers genuinely bore it for the full duration) but no stake()/withdraw()/pokeRiskWindow() transaction on this specific pool happened to land while that state was live, so riskWindowStart never sealed on the pool's side even though the risk was real.

Risk

Likelihood:

Occurs on every pool that reaches a terminal state without any stake, withdraw, or pokeRiskWindow transaction happening to land while the registry sits in UNDER_ATTACK/PROMOTION_REQUESTED i.e a condition entirely independent of whether the underlying agreement was actually attacked, since the registry's own persistent history (attackApproved) is never consulted to check.

Impact:

Stakers who backed a pool through a genuine attack window ,real risk, real duration, real exposure, receive the same zero-bonus outcome as stakers whose agreement never had a battle-tested at all, purely because nobody happened to trigger an on-chain observation during the live window.

Proof of Concept

Recommended Mitigation

Preserves the zero-bonus outcome for a genuine DAO bypass (attackApproved == false), while giving stakers in a genuinely-approved-but-unobserved attack window the same amount-weighted fallback already used elsewhere in the contract for the globalScore == 0 case, instead of collapsing both situations into an unconditional zero.

function _getAgreementState() internal view returns (IAttackRegistry.ContractState) {
address attackRegistry = safeHarborRegistry.getAttackRegistry();
if (attackRegistry == address(0)) revert InvalidAgreement();
return IAttackRegistry(attackRegistry).getAgreementState(agreement);
}
​
+ function _agreementWasEverApproved() internal view returns (bool) {
+ address attackRegistry = safeHarborRegistry.getAttackRegistry();
+ if (attackRegistry == address(0)) revert InvalidAgreement();
+ return IAttackRegistry(attackRegistry).getAgreementInfo(agreement).attackApproved;
+ }
​
function _bonusShare(address u, uint256 userEligible) internal view returns (uint256) {
if (snapshotTotalBonus == 0) return 0;
- // No observable risk → no bonus (see contract natspec).
- if (riskWindowStart == 0) return 0;
+ if (riskWindowStart == 0 && !_agreementWasEverApproved()) return 0;
​
//....

Support

FAQs

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

Give us feedback!