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

Multiple Participation Allows Users to Override Country Selection and Lose Shares

ishwar

Root + Impact

The joinEvent() function allows users to call it multiple times, overwriting their previous country selection. When a user calls joinEvent() again with a different countryId, their shares get mapped to the new country but the old mapping is not cleared. Additionally, the user gets added to the usersAddress array multiple times, and totalParticipantShares is incremented each time, leading to incorrect accounting. This can result in users accidentally losing their ability to claim winnings if they change from a winning country to a losing one.

Impact

Users can accidentally lose their winning shares by calling joinEvent multiple times. The usersAddress array becomes bloated with duplicate entries, causing gas issues in _getWinnerShares(). The totalParticipantShares becomes inflated with duplicate counting, breaking the protocol's accounting system.

Scenario

// Step 1: User deposits and gets 100 shares
deposit(1000 ether, alice);
​
// Step 2: User joins with country 5 (which will be the winner)
joinEvent(5);
// userToCountry[alice] = teams[5]
// userSharesToCountry[alice][5] = 100
// usersAddress = [alice]
// totalParticipantShares = 100
​
// Step 3: User changes mind and joins country 10 (loser)
joinEvent(10);
// userToCountry[alice] = teams[10] // OVERWRITTEN!
// userSharesToCountry[alice][10] = 100
// userSharesToCountry[alice][5] = 100 // Still exists!
// usersAddress = [alice, alice] // DUPLICATE!
// totalParticipantShares = 200 // DOUBLE COUNTED!
​
// Step 4: After event, country 5 wins
setWinner(5);
// _getWinnerShares() will count alice's shares twice if she's in winning country
​
// Step 5: Alice cannot withdraw because userToCountry[alice] = teams[10] != winner

Affected code

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);
}

Proposed fix

function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// Add check to prevent multiple joins
if (bytes(userToCountry[msg.sender]).length > 0) {
revert AlreadyJoined();
}
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);
}
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!