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

Duplicate joins and unreset totalWinnerShares cause inflated totals and potential division-by-zero

wyd_derick

Root + Impact

Description

  • The joinEvent and _getWinnerShares functions allow users to join multiple times and accumulate duplicate entries in usersAddress, while totalWinnerShares is never reset before aggregation.

  • This leads to double counting of participants, inflated total shares, and a possible division-by-zero when calculating payouts (e.g., in withdrawal or distribution logic).

function joinEvent(uint256 countryId) public {
...
@> usersAddress.push(msg.sender); // duplicates allowed
@> totalParticipantShares += participantShares; // adds repeatedly
}
​
function _getWinnerShares() internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // not reset
}
}
​

Risk

Likelihood:

  • Every time a user calls joinEvent more than once, their address is pushed again, and their shares are added multiple times.

  • When _getWinnerShares runs, it sums over this inflated list.

Impact:

  • Winner calculations become incorrect (inflated or unfair payouts).

  • In cases with no winners, totalWinnerShares == 0, causing division-by-zero and possible contract reverts or denial of withdrawals.

Proof of Concept

function testDoubleJoinInflation() public {
vault.deposit(100e18, user1);
vault.joinEvent(0);
vault.joinEvent(0); // duplicate join
​
vault.setWinner(0);
uint256 totalShares = vault.getWinnerShares();
​
// Expect 1x shares, but result is 2x inflated
assertEq(totalShares, 2 * vault.balanceOf(user1)); // ❌ incorrect
}

Explanation:
This test shows that calling joinEvent() twice causes the same user’s shares to be counted twice. When _getWinnerShares() runs, it sums over both entries, producing inflated totals and corrupting the reward logic.

Recommended Mitigation

Explanation:
This fix introduces a hasJoined mapping to prevent duplicate event joins, ensuring each participant is counted only once.
It also resets totalWinnerShares before summation, preventing accumulated values from previous rounds.
Both changes ensure correct total share calculations and eliminate division-by-zero risk.

function joinEvent(uint256 countryId) public {
require(stakedAsset[msg.sender] > 0, "No deposit");
+ require(!hasJoined[msg.sender], "Already joined");
require(countryId < teams.length, "Invalid team");
require(block.timestamp <= eventStartDate, "Event started");
​
userToCountry[msg.sender] = teams[countryId];
userSharesToCountry[msg.sender][countryId] = balanceOf(msg.sender);
usersAddress.push(msg.sender);
+ hasJoined[msg.sender] = true; // track unique joins
​
numberOfParticipants++;
totalParticipantShares += balanceOf(msg.sender);
​
emit joinedEvent(msg.sender, countryId);
}
​
function _getWinnerShares() internal returns (uint256) {
+ totalWinnerShares = 0; // reset before counting
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
​
Updates

Appeal created

bube 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.

Duplicate registration through `joinEvent`

Support

FAQs

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

Give us feedback!