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

Denial of Service (DoS) in setWinner function

kanyoro

Root + Impact

The _getWinnerShares function iterates over an unbound array of all participants. As the vault gains users, the gas cost of this loop will exceed the block gas limit making the setWinner function not callable and permanently locking all funds in the contract.

Description

  • Normal Behavior: When the owner calls setWinner, the contract is supposed to calculate the total number of shares held by all winners which is held by the totalWinnerShares variable. This value is critical for the final payout calculation.

  • The Problem: The _getWinnerShares function performs this calculation by looping over the usersAddress array, which grows by one every time a new user calls joinEvent. Once this array becomes large enough like thousands of users, the gas cost of the for loop will be higher than the block's gas limit, causing the transaction to revert. This will permanently break the setWinner function.

// Root cause in src/briVault.sol
function _getWinnerShares () internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i){
@> address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
​
// The array grows unbounded in joinEvent:
function joinEvent(uint256 countryId) public {
...
@> usersAddress.push(msg.sender);
...
}

Risk

Likelihood: High

  • Reason 1: This is a guaranteed failure of the contract as it becomes more popular.

  • Reason 2: The vulnerability is triggered by the intended, normal use of the protocol which is users joining the event.

Impact: High

  • Impact 1: Severe disruption of protocol functionality. The setWinner function will become uncallable.

  • Impact 2: Total and permanent loss of all user funds. The withdraw function requires the winnerSet modifier, which relies on setWinner successfully executing. If setWinner fails, no one can ever withdraw their funds.

Proof of Concept

This test can be added to briVault.t.sol. It simulates thousands of users depositing and joining. The logic is self-evident, but a test case would be structured as follows:

function test_FAIL_setWinner() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
// Simulate a large number of users depositing and joining using a loop
uint256 numUsers = 5000;
for (uint256 i = 0; i < numUsers; ++i) {
address user = address(uint160(i + 1000)); // Create unique addresses
mockToken.mint(user, 1 ether);
​
vm.startPrank(user);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user);
briVault.joinEvent(uint256(i % 48)); // Bet on a team
vm.stopPrank();
}
​
// Warp time to after the event
vm.warp(eventEndDate + 1);
​
// Attempt to setWinner will fail due to out-of-gas
vm.startPrank(owner);
vm.expectRevert(); // Expects a revert due to out-of-gas
briVault.setWinner(10); // Set team 10 as winner
vm.stopPrank();
}

Recommended Mitigation

Do not iterate over an unbounded array but instead, track the total shares for each team in a new mapping and update it in joinEvent.

// src/briVault.sol
contract BriVault is ERC4626, Ownable {
...
uint256 public totalWinnerShares;
uint256 public totalParticipantShares;
+ mapping(uint256 => uint256) public totalSharesPerTeam;
bool public _setWinner;
...
function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
...
_setWinner = true;
​
@> - _getWinnerShares();
@> + totalWinnerShares = totalSharesPerTeam[winnerCountryId];
​
_setFinallizedVaultBalance();
emit WinnerSet (winner);
...
/**
@notice get winnerShares
*/
@> - 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 {
...
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
+ totalSharesPerTeam[countryId] += participantShares;
​
@> - usersAddress.push(msg.sender);
​
numberOfParticipants++;
...
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!