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

Unbounded Loop in _getWinnerShares() May Cause Denial of Service on setWinner()

themilenkov

The loop scales linearly with the number of participants. With sufficient entries, gas costs may exceed the block limit, making it impossible for the owner to finalize the event and distribute rewards. This leads to a permanent denial of service for all users.

Description

  • The function iterates over the entire usersAddress array to calculate totalWinnerShares.
    Since usersAddress grows with every call to joinEvent(), the loop executes once per registered user.
    There is no upper bound or pagination mechanism, meaning that if a large number of users join the event (or spam entries before the event starts), the setWinner() transaction that triggers this loop can exceed the gas limit and revert indefinitely.

    As a result, the contract owner may become unable to finalize the event, leaving user funds locked in the vault.

    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:

  • Attainable under normal use if the event becomes popular or if attackers intentionally flood registrations.

Impact:

  • Prevents the setWinner() function (and therefore all subsequent withdraw() calls) from executing, effectively freezing the entire vault permanently.

Proof of Concept

Paste the following test to briVault.t.sol:

function test_DoS_getWinnerShares_TooManyUsers() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
// Simulate many participants joining
for (uint256 i; i < 5000; ++i) {
address user = address(uint160(i + 1));
mockToken.mint(user, 1 ether);
vm.startPrank(user);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user);
briVault.joinEvent(0); // everyone picks same team
vm.stopPrank();
}
​
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
​
// Expect high gas usage or revert due to OOG
vm.expectRevert();
briVault.setWinner(0);
vm.stopPrank();
}

Recommended Mitigation

Use an iterative or claim-based model instead of a full on-chain aggregation:

  • Maintain running totals incrementally during joinEvent() rather than recomputing in a loop.

function joinEvent(uint256 countryId) public {
...
+ totalSharesPerCountry[countryId] += participantShares;
}
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!