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

DoS on setWinner via Unlimited joinEvent Spamming

osamaebaid

Root + Impact

Description

  • Users deposit assets and call joinEvent to select a country, appending their address to usersAddress for participant tracking.

  • The owner calls setWinner after the event to finalize the winner, calculating totalWinnerShares by looping over usersAddress in _getWinnerShares.

  • The issue is that joinEvent allows unrestricted repeated calls by the same user, bloating usersAddress with duplicates.

  • This bloat makes the loop in _getWinnerShares exceed transaction gas limits, causing setWinner to revert due to out-of-gas, preventing event finalization.

// In joinEvent - unrestricted push
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
​
if (countryId >= teams.length) {
revert invalidCountry();
}
​
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
​
userToCountry[msg.sender] = teams[countryId];
​
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
@> usersAddress.push(msg.sender); // @> Unrestricted push allows spamming/duplicates
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
// In _getWinnerShares - loop over bloated array
function _getWinnerShares () internal returns (uint256) {
​
@> for (uint256 i = 0; i < usersAddress.length; ++i){ // @> Loop iterates over every entry, becoming too expensive with spam
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood: High

  • A user with a deposit repeatedly calls joinEvent before the event starts.

  • No checks prevent duplicates in usersAddress.

Impact: High

  • setWinner reverts due to out-of-gas, blocking winner declaration.

  • All vault assets remain locked indefinitely, preventing withdrawals or event resolution.

Proof of Concept

  • Add testDoSOnSetWinner to briVault.t.sol

  • To verify the cause is out-of-gas ,run forge test --mt testDoSOnSetWinner -vvvv

function testDoSOnSetWinner() public {
// User deposits
vm.prank(user1);
mockToken.approve(address(briVault), 5 ether);
vm.prank(user1);
briVault.deposit(5 ether, user1);
​
// Spam joinEvent 25,000 times
for (uint i = 0; i < 25000; i++) {
vm.prank(user1);
briVault.joinEvent(0); // Pick country 0 repeatedly
}
​
// Advance time past event end
vm.warp(eventEndDate + 1);
​
vm.expectRevert();
​
vm.prank(owner);
​
briVault.setWinner{gas: 30000000}(0);
}
}
​

Recommended Mitigation

The mitigation introduces a hasJoined mapping to ensure each user can call joinEvent only once, preventing duplicates in usersAddress. If already joined, it reverts. After a successful join, it sets hasJoined[msg.sender] to true. This keeps the array bounded by unique participants, avoiding gas bloat in _getWinnerShares and enabling setWinner to succeed. For further optimization, consider removing the array if possible or using an enumerable set for uniqueness.

// Add a mapping to track joined users and prevent duplicates
+ mapping(address => bool) public hasJoined;
​
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
​
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
​
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
​
+ if (hasJoined[msg.sender]) {
+ revert("Already joined");
+ }
​
userToCountry[msg.sender] = teams[countryId];
​
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender);
​
+ hasJoined[msg.sender] = true;
​
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!