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

Unbounded Loop in _getWinnerShares() Allows Gas-Limit DoS — Finalization/Withdrawals Can Be Permanently Blocked

ishaanbansal2312

Root + Impact

Description

NORMAL BEHAVIOUR:

  • When finalizing the event, the contract should compute the total shares for the winning country in a gas-bounded, reliable manner (constant time or bounded iterations), or use an aggregation model that avoids looping across an unbounded user list inside a single transaction.

PROBLEM :

  • _getWinnerShares() iterates over usersAddress (which grows with each joinEvent) and increments a storage variable totalWinnerShares inside the loop. Because usersAddress can be arbitrarily large and duplicates are possible, this function is unbounded and can exceed block gas limits. If setWinner() calls _getWinnerShares() and gas is exceeded, setWinner() will revert — the contract owner cannot finalize the winner and finalizedVaultAsset is not set, meaning withdraw() cannot succeed. An attacker can spam joinEvent() (or join multiple times if duplicates allowed) to permanently prevent finalization.

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: any large real user base creates many entries; duplicates amplify the issue.

  • High: an attacker with small deposits can spam joinEvent or exploit lack of hasJoined to create many entries cheaply.

Impact:

  • Owner cannot call setWinner() as it will revert due to gas exhaustion → finalizedVaultAsset never set → withdraw() cannot be executed by winners → funds locked.

  • if _getWinnerShares() partially succeeds or is callable multiple times without reset, totalWinnerShares may be incorrectly inflated, leading to wrong payouts.

Proof of Concept

Contract expects to iterate userAddress length N.

  • If N is large enough such that gasPerIteration *N > block gas limit (~30M), the loop will run out of gas and revert.

  • Attacker can, from a single EOAs or many low-value deposits, call joinEvent() repeatedly (if duplicate protection missing) to increase userAddress length to N causing setWinner() to revert.


for (i = 0; i < 6000; i++) {
await vault.connect(attacker).deposit(assets, attacker.address);
await vault.connect(attacker).joinEvent(countryId); // since duplicate allowed, repeat same
}
// Now call setWinner() - > should revert due to gas.
​

Recommended Mitigation

Primary strategy (best): Maintain per-country aggregate share counts during join — O(1) updates — eliminate need to iterate at finalization. Also prevent duplicate joins.

+mapping(uint256 => uint256) public countryShares; // cumulative shares per country
+mapping(address => bool) public hasJoined; // prevent duplicates
​
​
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);
-
- numberOfParticipants++;
- totalParticipantShares += participantShares;
-
- emit joinedEvent(msg.sender, countryId);
+ if (stakedAsset[msg.sender] == 0) revert noDeposit();
+ if (countryId >= teams.length) revert invalidCountry();
+ if (block.timestamp > eventStartDate) revert eventStarted();
+ if (hasJoined[msg.sender]) revert("AlreadyJoined");
+
+ hasJoined[msg.sender] = true;
+ userToCountry[msg.sender] = teams[countryId];
+
+ uint256 participantShares = balanceOf(msg.sender);
+ userSharesToCountry[msg.sender][countryId] = participantShares;
+
+ // maintain aggregates (constant time)
+ countryShares[countryId] += participantShares;
+
+ // optionally keep usersAddress but now unique and bounded
+ usersAddress.push(msg.sender);
+
+ numberOfParticipants++;
+ totalParticipantShares += participantShares;
+
+ emit joinedEvent(msg.sender, countryId);
}
​
​
Updates

Appeal created

bryanconquer 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!