BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

CRITICAL-08: _getWinnerShares Accumulates Without Reset

mostafapahlevani93

Root + Impact

The _getWinnerShares() function accumulates shares into totalWinnerShares without resetting it first. If the function is called multiple times, it doubles (or multiplies) the share count, completely breaking winner payout calculations.

Description

Normal behavior expects that totalWinnerShares should be calculated exactly once and represent the true sum of all winning participants' shares.

The current implementation uses += to accumulate shares without first setting totalWinnerShares = 0. While setWinner() has guards against being called twice, any refactoring or edge case that allows multiple calls will corrupt this critical value.

function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
require(countryIndex < teams.length, "Invalid country index");
​
if (_setWinner) {
revert WinnerAlreadySet();
}
​
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
​
// @> Calls _getWinnerShares
_getWinnerShares();
​
_setFinallizedVaultBalance();
emit WinnerSet(winner);
return winner;
}
​
function _getWinnerShares() internal returns (uint256) {
// @> No reset of totalWinnerShares before accumulation
// @> Adds to existing value instead of replacing it
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
// @> Uses += on potentially non-zero value
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • Currently protected by _setWinner flag in setWinner()

  • However, if contract is ever upgraded or _getWinnerShares() is called from another function, bug triggers

  • Code refactoring could accidentally remove or bypass the _setWinner check

  • Internal functions should be defensive and not rely on external guards

Impact:

  • If called twice, totalWinnerShares doubles, reducing all payouts by 50%

  • If called N times, payouts are divided by N

  • Winners receive drastically less than deserved

  • Excess funds remain locked in contract forever

  • Creates unfair distribution contradicting the tournament rules

  • Contract owner could accidentally or maliciously harm users

  • No way to fix after the corruption occurs

Proof of Concept

// Assume setWinner protection is bypassed (through upgrade, bug, or refactoring)
​
// First call to _getWinnerShares
vault._getWinnerShares(); // Assume made public for testing
// totalWinnerShares = 10000 (sum of all winner shares)
​
// Second call to _getWinnerShares
vault._getWinnerShares();
// totalWinnerShares = 20000 (doubled!)
​
// Winner withdrawals:
// Each winner's payout = (theirShares * vaultAsset) / totalWinnerShares
// = (1000 * 100000) / 20000
// = 5000 (should be 10000)
​
// Winners receive 50% of what they should
// Other 50% locked forever in contract
​
// ----
​
// More realistic scenario: Contract upgrade removes _setWinner check
function setWinnerV2(uint256 countryIndex) public onlyOwner {
// Updated version without proper guard
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_getWinnerShares(); // Called again
// totalWinnerShares now inflated
}

Recommended Mitigation

function _getWinnerShares() internal returns (uint256) {
+ // Reset before calculation to ensure idempotency
+ totalWinnerShares = 0;
+
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
​
+// Alternative: Add explicit guard in the function
+function _getWinnerShares() internal returns (uint256) {
+ require(totalWinnerShares == 0, "Winner shares already calculated");
+
+ for (uint256 i = 0; i < usersAddress.length; ++i) {
+ address user = usersAddress[i];
+ totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+ }
+ return totalWinnerShares;
+}

Additional Protection:

function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
require(countryIndex < teams.length, "Invalid country index");
​
if (_setWinner) {
revert WinnerAlreadySet();
}
​
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
​
_getWinnerShares();
​
+ // Ensure calculation was successful
+ require(totalWinnerShares > 0, "No winners found");
​
_setFinallizedVaultBalance();
emit WinnerSet(winner);
return 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.

mostafapahlevani93 Submitter
18 days ago
bube Lead Judge
15 days ago
bube Lead Judge 14 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!