BattleChain Confidence Pools

Cyfrin

FoundrySolidityLayer 2

7.25 ETH

View all submissions

Previous Next

Submission Details

Impact: medium

Likelihood: medium

contributeBonus() does not set expiryLocked, allowing owner to change expiry after a bonus commitment.

Author Revealed upon completion

Root + Impact

contributeBonus() accepts a real financial contribution to the pool, just like stake() does. But stake() sets expiryLocked = true on first use, which permanently blocks any future change to expiry. contributeBonus() never sets that flag, so a bonus contribution does not get the same protection.

Description

Describe the normal behavior in one or more sentences
When a user calls stake(), the contract sets expiryLocked to true on the first stake, which permanently prevents the pool owner from calling setExpiry() afterward. This protects anyone who has already put money into the pool from having the deadline changed on them later.
Explain the specific issue or problem in one or more sentences
contributeBonus() is also a real financial contribution to the pool, but it never sets expiryLocked. This means the owner can accept a bonus contribution and then still call setExpiry() to change the deadline, even though a bonus contributor has money committed to the pool just like a staker does.

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());

// @> missing: no expiryLocked check/set here, unlike stake()

uint256 balanceBefore = stakeToken.balanceOf(address(this));

stakeToken.safeTransferFrom(msg.sender, address(this), amount);

uint256 balanceAfter = stakeToken.balanceOf(address(this));

uint256 received = balanceAfter > balanceBefore ? balanceAfter - balanceBefore : 0;

if (received == 0) revert NoTokensReceived();

totalBonus += received;

emit BonusContributed(msg.sender, received);

}

Risk

Likelihood:

Reason 1
This happens any time a bonus contribution is made before the first stake -- a normal, expected sequence of events, not an edge case.
Reason 2
The pool owner does not need any special access beyond what they already have (setExpiry is onlyOwner) -- no extra exploit chain is required.

Impact:

Impact 1
A bonus contributor's funds are locked into a pool timeline they never agreed to, with no way to withdraw the contribution afterward.
Impact 2
Changing expiry shifts the timing anchor used in the bonus payout formula, which can change how the bonus pool is ultimately split among stakers.

Proof of Concept

The test below deploys everything through the real ConfidencePoolFactory (the actual deployment path, not a workaround). Most of the file is boilerplate mock contracts required to satisfy external interface types so the pool compiles standalone -- the actual bug is demonstrated in the last test function, test_ContributeBonus_DoesNotLockExpiry_AllowingLaterExpiryChange(): a contributor sends a bonus, then the sponsor successfully changes expiry afterward, even though expiryLocked should have prevented that once money was committed.

pragma solidity 0.8.26;

import "forge-std/Test.sol";

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

import {ConfidencePool} from "../src/ConfidencePool.sol";

import {ConfidencePoolFactory} from "../src/ConfidencePoolFactory.sol";

import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";

import {IBattleChainSafeHarborRegistry} from "@battlechain/interface/IBattleChainSafeHarborRegistry.sol";

import {IAgreement} from "@battlechain/interface/IAgreement.sol";

import {AgreementDetails, BountyTerms, Contact, Chain, Account} from "@battlechain/types/AgreementTypes.sol";

contract MockToken is ERC20 {

constructor() ERC20("Stake", "STK") {}

function mint(address to, uint256 amount) public { _mint(to, amount); }

}

// Minimal stubs -- only exist to satisfy interface requirements so the pool can be deployed

// through the real factory. Registry always reports NEW_DEPLOYMENT (irrelevant to this bug).

contract StubAttackRegistry is IAttackRegistry {

function getAgreementState(address) external pure returns (ContractState) { return ContractState.NEW_DEPLOYMENT; }

function registerDeployment(address, address) external {}

function authorizeAgreementOwner(address, address) external {}

function transferAttackModerator(address, address) external {}

function requestUnderAttack(address) external {}

function requestUnderAttackForUnverifiedContracts(address) external {}

function requestUnderAttackByNonAuthorized(address) external {}

function goToProduction(address) external {}

function promote(address) external {}

function cancelPromotion(address) external {}

function markCorrupted(address) external {}

function approveAttack(address) external {}

function rejectAttackRequest(address, bool) external {}

function instantPromote(address) external {}

function instantCorrupt(address) external {}

function changeRegistryModerator(address) external {}

function setSafeHarborRegistry(address) external {}

function registerContractForExistingAgreement(address) external {}

function unregisterContractForExistingAgreement(address) external {}

function syncNewContracts(address) external {}

function isTopLevelContractUnderAttack(address) external pure returns (bool) { return false; }

function getAttackModerator(address) external pure returns (address) { return address(0); }

function getAgreementForContract(address) external pure returns (address) { return address(0); }

function getAgreementInfo(address) external pure returns (AgreementInfo memory info) { return info; }

function getRegistryModerator() external pure returns (address) { return address(0); }

function getSafeHarborRegistry() external pure returns (address) { return address(0); }

function getAuthorizedOwner(address) external pure returns (address) { return address(0); }

}

contract StubSafeHarborRegistry is IBattleChainSafeHarborRegistry {

address public attackRegistryAddr;

constructor(address _attackRegistry) { attackRegistryAddr = _attackRegistry; }

function getAttackRegistry() external view returns (address) { return attackRegistryAddr; }

function isAgreementValid(address) external pure returns (bool) { return true; }

function setAgreementFactory(address) external {}

function adoptSafeHarbor(address) external {}

function getAgreement(address) external pure returns (address) { return address(0); }

function isChainValid(string calldata) external pure returns (bool) { return true; }

function getAgreementFactory() external pure returns (address) { return address(0); }

}

contract StubAgreement is IAgreement {

address public immutable ownerAddr;

constructor(address _owner) { ownerAddr = _owner; }

function owner() external view returns (address) { return ownerAddr; }

function isContractInScope(address) external pure returns (bool) { return true; }

function extendCommitmentWindow(uint256) external {}

function setProtocolName(string memory) external {}

function setContactDetails(Contact[] memory) external {}

function addOrSetChains(Chain[] memory) external {}

function removeChains(string[] memory) external {}

function addAccounts(string memory, Account[] memory) external {}

function removeAccounts(string memory, string[] memory) external {}

function setBountyTerms(BountyTerms memory) external {}

function setAgreementURI(string memory) external {}

function getCantChangeUntil() external pure returns (uint256) { return 0; }

function getDetails() external pure returns (AgreementDetails memory d) { return d; }

function getProtocolName() external pure returns (string memory) { return ""; }

function getBountyTerms() external pure returns (BountyTerms memory b) { return b; }

function getAgreementURI() external pure returns (string memory) { return ""; }

function getRegistry() external pure returns (address) { return address(0); }

function getChainIds() external pure returns (string[] memory arr) { return arr; }

function getBattleChainCaip2ChainId() external pure returns (string memory) { return ""; }

function getBattleChainScopeAddresses() external pure returns (address[] memory arr) { return arr; }

function getBattleChainScopeCount() external pure returns (uint256) { return 0; }

}

contract SetExpiryLockGapTest is Test {

ConfidencePool pool;

MockToken token;

address sponsor = makeAddr("sponsor");

address bonusContributor = makeAddr("bonusContributor");

function setUp() public {

ConfidencePool poolImpl = new ConfidencePool();

token = new MockToken();

StubAttackRegistry attackRegistry = new StubAttackRegistry();

StubSafeHarborRegistry safeHarborRegistry = new StubSafeHarborRegistry(address(attackRegistry));

StubAgreement agreement = new StubAgreement(sponsor);

// Deployed through the real factory -- the actual protocol path, not a bypass.

ConfidencePoolFactory factoryImpl = new ConfidencePoolFactory();

bytes memory factoryInit = abi.encodeCall(

ConfidencePoolFactory.initialize, (address(safeHarborRegistry), address(poolImpl), makeAddr("moderator"))

);

ConfidencePoolFactory factory = ConfidencePoolFactory(address(new ERC1967Proxy(address(factoryImpl), factoryInit)));

vm.prank(factory.owner());

factory.setStakeTokenAllowed(address(token), true);

address[] memory accounts = new address[](1);

accounts[0] = address(0x1234);

uint256 originalExpiry = block.timestamp + 60 days;

vm.prank(sponsor);

pool = ConfidencePool(factory.createPool(address(agreement), address(token), originalExpiry, 1 ether, sponsor, accounts));

assertEq(pool.expiry(), originalExpiry, "sanity: expiry set at creation");

assertFalse(pool.expiryLocked(), "sanity: not locked before any stake/bonus");

}

/// @notice contributeBonus() never sets expiryLocked, even though it's a real financial

/// commitment just like stake() (which does set the flag). The owner can accept a bonus

/// and then move expiry -- changing the terms on money already committed, exactly what

/// expiryLocked is supposed to prevent.

function test_ContributeBonus_DoesNotLockExpiry_AllowingLaterExpiryChange() public {

uint256 bonusAmount = 500 ether;

token.mint(bonusContributor, bonusAmount);

vm.startPrank(bonusContributor);

token.approve(address(pool), bonusAmount);

pool.contributeBonus(bonusAmount);

vm.stopPrank();

assertEq(token.balanceOf(address(pool)), bonusAmount, "bonus received");

// Bug: the bonus was just deposited, but the deadline is still changeable.

assertFalse(pool.expiryLocked(), "BUG: expiryLocked still false after contributeBonus");

uint256 newExpiry = block.timestamp + 400 days;

vm.prank(sponsor);

pool.setExpiry(newExpiry); // should have reverted with ExpiryLocked()

assertEq(pool.expiry(), newExpiry, "sponsor successfully moved expiry post-bonus-commitment");

}

Recommended Mitigation

Add the same expiryLocked check that stake() already has, at the start of contributeBonus() -- right after the deposit-allowed check and before the token transfer. This makes a bonus contribution lock the deadline exactly the way a stake does, closing the gap.

_assertDepositsAllowed(_observePoolState());

+ if (!expiryLocked) {

+ expiryLocked = true;

+ }

uint256 balanceBefore = stakeToken.balanceOf(address(this));

Prize pool breakdown

Total prize

7.25 ETH

nSLOC

589

0 ETH / LOC

High Medium

Low

1.25

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

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

Give us feedback!