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

DoS Attack via Unbounded Loop in `_getWinnerShares` Function

ishwar

Root

The _getWinnerShares() function iterates through the entire usersAddress array without any gas limit protection. Since users can be added to this array multiple times (as shown in the previous finding), and there's no limit on the number of participants, a malicious actor could create thousands of deposits and join events to bloat the array. This would cause the setWinner() function to run out of gas and revert, effectively locking all funds in the contract permanently as no one could withdraw without the winner being set.

Impact

Complete denial of service preventing winner declaration and permanent fund lock.
All participants' funds become irretrievable as the withdraw function requires the winner to be set first.
The owner cannot set the winner due to gas limits, creating a deadlock situation.

Scenario

// Attacker contract
contract GriefingAttack {
BriVault vault;
IERC20 token;
function attack() external {
// Create 10,000 small deposits
for(uint i = 0; i < 10000; i++) {
address user = address(uint160(i));
token.transfer(user, minimumAmount + fee);
// From each address, deposit and join
vault.deposit(minimumAmount + fee, user);
vault.joinEvent(0);
}
// Now usersAddress.length = 10,000
// When owner calls setWinner(), it will iterate 10,000 times
// Gas cost: ~10,000 * 5,000 gas = 50,000,000 gas (exceeds block limit)
}
}

Affected Code

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

Proposed Fix

Implement a pull-based winner share calculation pattern or use pagination:

mapping(uint256 => uint256) public countryTotalShares;
mapping(address => bool) public hasJoined;
​
function joinEvent(uint256 countryId) public {
require(!hasJoined[msg.sender], "Already joined");
// ... other checks ...
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
countryTotalShares[countryId] += participantShares; // Track total per country
hasJoined[msg.sender] = true;
// Don't push to array
}
​
function setWinner(uint256 countryIndex) public onlyOwner {
// ... checks ...
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
totalWinnerShares = countryTotalShares[countryIndex]; // O(1) operation
_setFinallizedVaultBalance();
emit WinnerSet(winner);
}
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!