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

`ConfidencePool::contributeBonus` function uses `ConfidencePool::_assertDepositsAllowed` internal function to verify bonus acceptance but that cause bonus rejection in an active risk stage (PROMOTION_REQUESTED)

Author Revealed upon completion

Root + Impact

The usage of ConfidencePool::_assertDepositsAllowed internal function inside the ConfidencePool::contributeBonus function leads to the rejection of contributed bonus during an active risk stage (PROMOTION_REQUESTED)

Description

The ConfidencePool::stake function uses the internal function ConfidencePool::_assertDepositAllowed to prevent deposit during PRODUCTIONand CORRUPTED stage, which makes a lot of sense as those are terminal stage and no deposit should allowed anymore, but the ConfidencePool::_assertDepositAllowed internal function also prevents deposit when the agreement the pool is insuring is in the PROMOTION_REQUESTED stage, which does not really make sense at first because it is not a terminal stage but instead it is an active risk stage but after reading ##3 of DESIGN.md I was able to make sense of the situation as the protocol explained the decision for blocking deposit as follows:

**`PROMOTION_REQUESTED`:** the agreement has requested to exit toward `PRODUCTION`, so the risk
window is about to close (`riskWindowEnd` imminent). Blocking deposits here stops a late join
in the final stretch — entering with almost no remaining risk-bearing time before resolution.
The state is still attackable; it is the *closing window*, not assured survival, that justifies
the block.

As we can see clearly, the Protocol itself clearly states that `the state is still attackable; it is the closing window, not assured survival, which means the stakers are equally at risk as they were at risk in the UNDER_ATTACK stage.

Where the vulnerability arises is that the protocol forgetfully and unknowingly (I say forgetfully and unknowingly because I have read the whole DESIGN.md and protocol-readme.md, and not for once did the protocol mention it as a design choice) uses this same ConfidencePool::_assertDepositAllowed internal function inside the ConfidencePool::contributeBonus function to check if deposit should be accepted or rejected. The ConfidencePool::_assertDepositAllowed internal function blocks incoming funds if the agreement that the pool is insuring is in any of these three states: PRODUCTION, CORRUPTED, or PROMOTION_REQUESTED.

The first two states are terminal states, meaning the agreement has reached finality, and there is no further change in state, which makes it very sensible that bonus donation is rejected, but for the last state (PROMOTION_REQUESTED), it is an active risk state, and the state of the agreement the pool is insuring hasn't reached finality; instead, it is preparing to reach finality which means the possibility exists that staker may loose all their stakes plus any associated bonus if the state of the agreement progress from PROMOTION_REQUESTED to CORRUPTED. So it does not make sense that stakers are still at risk of losing their stakes, and we are rejecting the bonus donation, which is the incentive for the risk they are undertaking.


Let's walk through what your arguments might be to invalidate this finding:

Your possible argument: This is a design choice because the protocol does not want any further bonus donations, as it doesn't want any stake in the PROMOTION_REQUESTED state either.

My response: This argument makes no sense because just by merely looking critically at the ConfidencePool::_assertDepositAllowed internal function name, anyone can easily tell that the function was originally created for the ConfidencePool::stake function and was just later slapped on the ConfidencePool::contributeBonus function out of negligence and unawareness of this effect I am pointing out because depositing into the pool is not and was never treated the same as contributing a bonus to the pool


Your possible argument: The PROMOTION_REQUESTED is a state very close to the terminal state because promotion has been requested for the agreement already, and it is about leaving the risk state, so the protocol does not want to take any more bonuses at this state, hence why they blocked bonus deposit, as they have blocked staking deposit as well

My response: This argument makes no sense as well because the protocol explicitly said the following in the DESIGN.md

The BattleChain attack registry's active-risk states — `UNDER_ATTACK` and `PROMOTION_REQUESTED`
— do **NOT** mean "a breach is in progress." They mean the in-scope contracts are *legally
attackable by whitehats*: the agreement is live and exposed, which is its normal operating mode.
They are **not** evidence that funds were lost. Only the terminal `CORRUPTED` state is evidence
of an actual breach.
​
Both active-risk states can still transition to `CORRUPTED` (e.g. `markCorrupted` is reachable
from both `UNDER_ATTACK` and `PROMOTION_REQUESTED`); neither implies survival.

They explicitly called both UNDER_ATTACK AND PROMOTION_REQUESTED as active-risk states and said explicitly that both active-risk states can still transition to CORRUPTED; neither implies survival.


// Root cause in the codebase with @> marks to highlight the relevant section

function contributeBonus(
uint256 amount
) external nonReentrant whenPoolNotPaused {
// revert 0 value contribution
if (amount == 0) revert InvalidAmount();
// if pool left unresolved state revert
if (outcome != PoolStates.Outcome.UNRESOLVED)
revert OutcomeAlreadySet();
// agreement expired, revert
if (block.timestamp >= expiry) revert StakingClosed();
​
// eunsre deposit is allowed
// here the call was made to _assertDepositsAllowed, which rejects the bonus donation
@>_assertDepositsAllowed(_observePoolState());
...
}
​
function _assertDepositsAllowed(
IAttackRegistry.ContractState state
) private pure {
// if agreement is in any of the three states below, revert
if (
// this check here triggers a revert for a bonus deposit while the agreement is in an
// active risk state
@>state == IAttackRegistry.ContractState.PROMOTION_REQUESTED ||
state == IAttackRegistry.ContractState.PRODUCTION ||
state == IAttackRegistry.ContractState.CORRUPTED
) {
revert StakingClosed();
}
}
​
​

Risk

Likelihood:

The likelihood of this vulnerability is high, as it will occur whenever the agreement reaches the PROMOTION_REQUESTED stage, and anyone from anywhere in the world calls the ConfidencePool::contributeBonus function with any amount.

Impact:

This vulnerability blocks bonus deposit, which is the main incentive for stakers who are still actively undertaking risk (risk of losing all their stakes)

Proof of Concept

Put the code below in BaseConfidencePoolTest.sol

function _mintToUser(address user, uint256 amount) internal {
token.mint(user, amount);
}
​
function _approvePool(address user, uint256 amount) internal {
vm.startPrank(user);
token.approve(address(pool), amount);
vm.stopPrank();
}
​
function _contributeBonus2(address user, uint256 amount) internal {
vm.startPrank(user);
pool.contributeBonus(amount);
vm.stopPrank();
}

Put the code below in ConfidencePool.K2Bonus.t.sol, then run the test with forge test --mt testBonusForStakersIsRejectedDuringAnActiveRiskPeriod -vvvv


function _flagPromotionRequested() internal {
attackRegistry.setAgreementState(
IAttackRegistry.ContractState.PROMOTION_REQUESTED
);
}
​
​
​
function testBonusForStakersIsRejectedDuringAnActiveRiskPeriod() external {
// the `stake` function of the `ConfidencePool` uses the `_assertDepositsAllowed` internal function to prevent deposit during `PRODUCTION`, `CORRUPTED` stages
// which makes a lot of sense because those are terminal state but the `_assertDepositsAllowed` internal function also prevent deposit during
// `PROMOTION_REQUESTED` which is an active risk stage, that doesn't make a lot of sense initially but the protocol declared that as design choice and state
// their reason you can check `## 3` in `DESIGN.md`. where the problem arise is that the same `_assertDepositsAllowed` internal function is what is used to
// check if bonus deposit is allowed, hence bonus deposit is denied in `PROMOTION_REQUESTED` stage which is defined as an active risk stage, stakers are still
// undertaking risk which may lead to them eventually lose all their stake and any entitled bonus, hence it makes no sense that you block bonus deposit when
// stakers are still undertaking risk
//
// get the start time of the staking phase
uint256 t0 = vm.getBlockTimestamp();
// alice deposit a stake
_stake(alice, 100 * ONE);
// move time forward by 100 seconds
vm.warp(t0 + 100);
// bob then deposited a stake
_stake(bob, 100 * ONE);
// move time forward by 200 seconds
vm.warp(t0 + 200);
​
// make the agreement enter risk stage
_enterRisk();
// get the riskWindowStart on the pool contract to be sure the risk window has officially began inside the pool
uint256 windowStart = pool.riskWindowStart();
// ensure risk windowStart is set correctly
assertEq(windowStart, t0 + 200);
​
// Globals must reflect both stakers at entry = windowStart.
assertEq(pool.sumStakeTime(), 200 * ONE * windowStart);
assertEq(pool.sumStakeTimeSq(), 200 * ONE * windowStart * windowStart);
​
// Per-user entryTime is still stale until next touch; verify by claiming both and
// checking equal payouts (proves effective entry is clamped equally).
vm.warp(vm.getBlockTimestamp() + 5 days);
// carol came along to deposit bonus to the pool
_contributeBonus(carol, 100 * ONE);
// the pool entered `PROMOTION_REQUESTED` (still an active risk stage as this stage could still lead to CORRUPTED)
_flagPromotionRequested();
​
//Carol thought of adding more bonus for the user because they deserve it, but the protocol rejected the bonus while
// the stakers are still undertaking risk, I won't be using the protocol `_contributeBonus` internal function to
// show this revert, as it does a lot of things inside of it (minting to the bonus contributor, approving the pool, ...)
// before actually depositing the bonus to the pool
// which makes my revert not hit on the next transaction because the next transaction after my vm.expectRevert() was not the bonus
// depositing transaction
​
// mint carol some token to deposit as bonus to the pool
_mintToUser(carol, 100 * ONE);
​
// carol approve the pool to take the token as bonus deposit
_approvePool(carol, 100 * ONE);
​
// carol depositing bonus to the pool reverts which is not supposed to be
vm.expectRevert();
// now carol try to deposit the bonus to the pool
_contributeBonus2(carol, ONE);
}

Recommended Mitigation

You can have two state variables, isStake and isBonus, that are set inside the respective functions ConfidencePool::stake/ConfidencePool::contributeBonus, then refactor your ConfidencePool::_assertDepositsAllowed function to be as below. Based on those flags, you can decide which of the agreement states would determine if you allow or reject both staking and bonus depositing.

//state variables
bool isStake
bool isBonus
​
function _assertDepositsAllowed(
IAttackRegistry.ContractState state
) private pure {
// if agreement is in any of the three states below, revert
if(isStake){
(
state == IAttackRegistry.ContractState.PROMOTION_REQUESTED ||
state == IAttackRegistry.ContractState.PRODUCTION ||
state == IAttackRegistry.ContractState.CORRUPTED
){
revert StakingClosed();
}
} else {
(state == IAttackRegistry.ContractState.PRODUCTION ||
state == IAttackRegistry.ContractState.CORRUPTED) {
revert agreementFinalized();
}
}
}

You may also just create a separate internal function that will be called from inside ConfidencePool::contributeBonus instead of calling ConfidencePool::_assertDepositsAllowed.


function _assertBonusIsAllowed(IAttackRegistry.ContractState state) internal pure {
(state == IAttackRegistry.ContractState.PRODUCTION ||
state == IAttackRegistry.ContractState.CORRUPTED) {
revert agreementFinalized();
}
}

Support

FAQs

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

Give us feedback!