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

Computing `bountyEntitlement` with `snapshotTotalStaked + snapshotTotalBonus` silently fail to capture the full balance of the pool as the attacker bounty

Author Revealed upon completion

Root + Impact

Computing bountyEntitlement with snapshotTotalStaked + snapshotTotalBonus silently failed to capture the full balance of the pool, as the attacker's bounty, leading to the attacker missing out on some of their bounty to the recovery address

Description

The expected behavior is that if the agreement is corrupted in goodFaith then the moderator passes in the boolean goodFaith_ as true and passes in the address of the whitehat attacker that corrupted the agreement, making the attacker eligible for the total balance of the pool as their bounty from the resolution moment till corruptedClaimDeadlineas specified by the DESIGN.md

## 12. CORRUPTED bounty mechanics
​
For good-faith CORRUPTED, `bountyEntitlement` and `corruptedReserve` are both set to
`snapshotTotalStaked + snapshotTotalBonus` — i.e. the **entire pool** is the named attacker's
bounty (there is no separate "surplus" alongside an unclaimed bounty in the normal case).

Unfortunately, because the protocol developers used snapshotTotalStaked + snapshotTotalBonus to compute bountyEntitlement, which is assumed to be the total balance of the pool, which is to be awarded to the whitehat attacker, they silently failed to capture the full balance of the pool, because it is very likely that a user may donate bonus to the pool by sending tokens directly to the pool instead of calling the contributeBonus function. That excess bonus that was sent directly to the pool without calling the contributeBonus function would be swept to the recoveryAddress later, which breaks the core design of the protocol, which states that there is no separate "surplus" alongside an unclaimed bounty in the normal case

Let's walk through your possible argument and my response to it:

Your argument: This is a design choice, it is intended by the protocol

My response: This does not make any sense, and it was not intended by the protocol because I carefully read through the DESIGN.mdand it was never mentioned for once, which makes it clear the protocol dev never thought of it or even saw it coming. Instead, they give plenty of evidence, making it clear that the whole pool balance and not (snapshotTotalStaked + snapshotTotalBonus), which they foresee as the pool's whole balance, which excludes some of the balance of the pool, is supposed to go to the whitehat attacker.

I am not talking from my feelings, but strictly on the design of the protocol. Now, look at the below said in ##12 of DESIGN.md


A donation landing while the bounty is unclaimed is held until the attacker claims (then swept immediately) or the window elapses

The above makes it clear that only the bonus that lands after the total balance of the pool at the time of resolution has been awarded to the whitehat attacker is to be swept to the recoveryAddress and not any other bonus before the pool resolution time



// Root cause in the codebase with @> marks to highlight the relevant section
​
/// @inheritdoc IConfidencePool
function flagOutcome(
PoolStates.Outcome newOutcome,
bool goodFaith_,
address attacker_
) external onlyModerator {
...
// compute the corrupted reserve
corruptedReserve = newOutcome == PoolStates.Outcome.CORRUPTED
? snapshotTotalStaked + snapshotTotalBonus
: 0;
// compute bountyEntitlement
@>bountyEntitlement = willBeGoodFaithCorrupted
? snapshotTotalStaked + snapshotTotalBonus
: 0;
...
}

Since bountyEntitlement and corruptedReserve hold the same value, this vulnerability affects them both, and the mitigation recommended below should be applied to both.


Risk

Likelihood:

  • This will happen when anyone contributes a bonus to the pool without directly calling the contributeBonus function, and a whiteHat corrupts the agreement the pool is backing.


Impact:

  • The whitehat would lose out on some of their bounty to the reserveAddress

Proof of Concept

Put the code below in test/unit/ConfidencePool.k2Bonus.t.sol and run the test with forge test --mt testBountyDoesNotCaptureDirectDonationsSilentlyCuttingDownAttackerBounty

function testBountyDoesNotCaptureDirectDonationsSilentlyCuttingDownAttackerBounty() external {
// 1. Stake and contribute bonus normally
_stake(alice, 100 * ONE);
_contributeBonus(carol, 50 * ONE);
// 2. Direct donation of tokens to the pool (not through contributeBonus)
token.mint(address(pool), 10 * ONE);
// 3. Move agreement to under attack and seal the risk window
_passThroughUnderAttack();
// 4. Agreement gets corrupted, moderator flags the outcome as CORRUPTED (good faith)
attackRegistry.setAgreementState(
IAttackRegistry.ContractState.CORRUPTED
);
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
// 5. Check bountyEntitlement and corruptedReserve
// Expected pool balance at resolution = 100 (stake) + 50 (bonus) + 10 (donation) = 160 * ONE
// But bountyEntitlement is calculated as snapshotTotalStaked (100) + snapshotTotalBonus (50) = 150 * ONE
uint256 expectedBalance = 160 * ONE;
uint256 entitlement = pool.bountyEntitlement();
uint256 reserve = pool.corruptedReserve();
assertEq(token.balanceOf(address(pool)), expectedBalance);
assertEq(entitlement, 150 * ONE);
assertEq(reserve, 150 * ONE);
assertTrue(
entitlement < expectedBalance,
"Bounty entitlement is less than the total pool balance"
);
// 6. Attacker claims bounty
uint256 attackerBefore = token.balanceOf(attacker);
vm.prank(attacker);
pool.claimAttackerBounty();
// Attacker only gets 150 * ONE, NOT the whole pool balance (160 * ONE)
assertEq(token.balanceOf(attacker) - attackerBefore, 150 * ONE);
assertEq(token.balanceOf(address(pool)), 10 * ONE); // 10 * ONE remains trapped in the pool
}

Recommended Mitigation

corruptedReserve only goes down, and bountyEntitlement stays as it is to be compared against bountyClaimed to be able to determine if the attacker has successfully claimed all their bounty, so these changes have no side effects

- remove this code
corruptedReserve = newOutcome == PoolStates.Outcome.CORRUPTED
? snapshotTotalStaked + snapshotTotalBonus
: 0;
​
bountyEntitlement = willBeGoodFaithCorrupted
? snapshotTotalStaked + snapshotTotalBonus
: 0;
+ add this code
corruptedReserve = newOutcome == PoolStates.Outcome.CORRUPTED
? snapshotTotalStaked + (stakeToken.balanceOf(address.this) - snapshotTotalStaked)
: 0;
bountyEntitlement = willBeGoodFaithCorrupted
? snapshotTotalStaked + (stakeToken.balanceOf(address.this) - snapshotTotalStaked)
: 0;
​

Support

FAQs

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

Give us feedback!