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

Unbounded loop in _replaceScope allows permanent DOS of setPoolScope via prior scope size

Author Revealed upon completion

Root + Impact

Description

  • When the pool owner calls setPoolScope to update the BattleChain accounts the pool monitors, _replaceScope first iterates the entire existing scope array to clear isAccountInScope for every old account, then applies the new scope. This is a valid wholesale-replacement pattern — the owner controls the input scope and is trusted to keep it reasonably sized.
    The loop that clears the old scope iterates over _scopeAccounts with no upper bound on its length. Each iteration executes an SSTORE (isAccountInScope[old[i]] = false). If a prior scope contained thousands of accounts, the cumulative gas cost of the clearing loop alone can exceed the block gas limit or become economically prohibitive, permanently preventing any subsequent scope update.

function _replaceScope(address[] calldata accounts) internal {
if (accounts.length == 0) revert EmptyScope();
// Clear existing scope first so this is a wholesale replacement.
address[] memory old = _scopeAccounts;
// @> for (uint256 i; i < old.length; ++i) {
// @> isAccountInScope[old[i]] = false;
// @> }
for (uint256 i; i < old.length; ++i) {
isAccountInScope[old[i]] = false;
}
delete _scopeAccounts;
for (uint256 i; i < accounts.length; ++i) {
address account = accounts[i];
if (isAccountInScope[account]) revert DuplicateAccount(account);
if (!IAgreement(agreement).isContractInScope(account)) {
revert AccountNotInAgreementScope(account);
}
_scopeAccounts.push(account);
isAccountInScope[account] = true;
}
emit ScopeUpdated(accounts);
}

Risk

Likelihood:

  • Owner sets a scope with thousands of accounts in an earlier setPoolScope or initialize call, and any subsequent scope update must clear every one of those entries before applying the new scope

  • The clearing loop gas cost scales linearly with the old scope size with no protocol-enforced cap, so at sufficient size the call exceeds block gas or becomes economically prohibitive.

Impact:

  • Owner cannot update the pool's BattleChain account scope, setPoolScope permanently reverts on any attempt

  • No fund movement is affected (scope is informational only), but a documented owner capability is permanently disabled

Proof of Concept

The test below walks through the full lifecycle: the owner first sets a 3,000-account scope, then later attempts a small scope update. The gas cost of the second setPoolScope call is dominated by the L755-757 clearing loop, which must SSTORE-clear isAccountInScope for all 3,000 old accounts before it can write the 10 new ones. Gas scales linearly with old scope size; at 30,000 accounts the clearing alone exceeds a 30M block gas limit.

function testClearingLoopIsUnbounded() external {
// 1. Owner sets a 3,000-account scope during pre-attack staging.
uint256 N = 3_000;
address[] memory big = new address[](N);
for (uint256 i; i < N; ++i) {
big[i] = address(uint160(1 + i));
agreementContract.setContractInScope(big[i], true);
}
pool.setPoolScope(big);
// 2. Later, owner needs to update the scope (10 accounts).
address[] memory small = new address[](10);
for (uint256 i; i < 10; ++i) {
small[i] = address(uint160(N + 1 + i));
agreementContract.setContractInScope(small[i], true);
}
// 3. _replaceScope must first clear all 3,000 old accounts (SSTORE each)
// before it can write the 10 new ones.
uint256 g = gasleft();
pool.setPoolScope(small);
uint256 gasUsed = g - gasleft();
// Clearing cost is O(N). At N=30,000 it exceeds block gas (30M).
// No on-chain cap prevents reaching that size.
assertTrue(gasUsed > 500_000);
}

Recommended Mitigation

Remove the old-scope clearing loop. Stale isAccountInScope entries are harmless — the second loop already sets the new scope's accounts to true, and scope is never read by any fund-moving logic. The _scopeAccounts array is already overwritten by delete + push.

❌ Remove:
// Clear existing scope first so this is a wholesale replacement.
address[] memory old = _scopeAccounts;
for (uint256 i; i < old.length; ++i) {
isAccountInScope[old[i]] = false;
}
✅ Add: nothing. Just delete _scopeAccounts; alone suffices — stale isAccountInScope entries are never read by fund logic, and the second loop overwrites relevant entries.

Support

FAQs

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

Give us feedback!