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

Unbounded Loop in _getWinnerShares() Can Cause Denial of Service, Permanently Locking All Funds

3mr_obito4

Root + Impact

Description

  • The setWinner function, which is essential for enabling withdrawals, must calculate the total shares of all winners by looping through an array of every participant. As more users join the event, this array grows, and the cost of the loop increases. If the event becomes popular enough, the gas cost will exceed the blockchain's block limit, causing the setWinner transaction to fail every time. This makes it impossible to ever declare a winner, permanently freezing all funds in the contract.

function joinEvent(uint256 countryId) public {
// ...
@> usersAddress.push(msg.sender); // <-- This array can grow to an unbounded size.
// ...
}
​
​
function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
// ...
@> _getWinnerShares(); // <-- This call triggers the expensive loop.
// ...
}
​
​
function _getWinnerShares() internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i) { // <-- Gas cost is proportional to array length.
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • This vulnerability is triggered only when the number of entries in the usersAddress array becomes sufficiently large (potentially hundreds or thousands, depending on block gas limits).

  • The contract becomes a victim of its own success; the more popular the event, the higher the likelihood of triggering the DoS condition.

Impact:

  • IPermanent DoS of Core Functionality: The setWinner() function becomes permanently uncallable, preventing the contract from ever moving into the "payout" phase.

  • Permanent Freezing of All Funds: Since setWinner() can never succeed, the winnerSet modifier will always fail. This means the claimWinnings() (or withdraw()) function can never be called by any user, locking all deposited assets forever.

Proof of Concept

This test exploits the related "Duplicate Joins" vulnerability to simulate a high number of participants cheaply. A single user joins the event many times, bloating the usersAddress array until the gas cost of the loop in setWinner exceeds the transaction limit, causing it to revert.

function test_POC_SetWinnerFailsDueToGasLimit() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
uint256 depositAmount = 1 ether;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
​
uint256 participantCount = 800;
for (uint256 i = 0; i < participantCount; i++) {
briVault.joinEvent(4);
}
vm.stopPrank();
​
vm.warp(eventEndDate + 1 days);
vm.startPrank(owner);
​
vm.expectRevert();
briVault.setWinner(4); // Attempt to call the vulnerable function.
vm.stopPrank();
​
}

Recommended Mitigation

The root cause is iterating over an unbounded array. The solution is to remove the loop and instead track the total shares per team as users join. This provides a gas-efficient O(1) lookup when the winner is set.

​
+ mapping(uint256 => uint256) public totalSharesPerCountry;
​
​
// ... constructor and other functions ...
​
​
function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
require(countryIndex < teams.length, "Invalid country index");
​
if (_setWinner) {
revert WinnerAlreadySet();
}
​
winnerCountryId = countryIndex;
winner = teams[countryIndex];
​
_setWinner = true;
​
- _getWinnerShares();
+ totalWinnerShares = totalSharesPerCountry[winnerCountryId];
​
_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;
- }
​
// ...
​
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// ... other checks ...
​
userToCountry[msg.sender] = teams[countryId];
​
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
+ totalSharesPerCountry[countryId] += participantShares;
​
usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
// ...
}
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.

Support

FAQs

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

Give us feedback!