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

Zero check for duplicate user entries

incogknito

Root + Impact

Users can call joinEvent() multiple times, causing their address to be added to usersAddress array repeatedly. This leads to inflated totalWinnerShares calculation and incorrect reward distribution where winners receive less than their fair share.

Description

Each user should only be able to join the event once, and their address should appear exactly once in the usersAddress array. When calculating winner shares, each winner's shares should be counted only once in totalWinnerShares but the joinEvent() function lacks a check to prevent users from calling it multiple times. Each call appends msg.sender to the usersAddress array without verifying if the address already exists. This creates duplicate entries that are later iterated over in _getWinnerShares(), causing the same user's shares to be counted multiple times in totalWinnerShares.

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); // No duplicate check - pushes every time
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
function _getWinnerShares () internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i){ // Iterates over duplicates
address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // Counts same user multiple times
}
return totalWinnerShares;
}

Risk

Likelihood:

  • Users can accidentally call joinEvent() multiple times through UI interactions or transaction retries before eventStartDate, as there is no check that is preventing repeated calls.

  • Also, malicious users can intentionally exploit this by calling joinEvent() multiple times with the same or different countryId values to manipulate the totalWinnerShares calculation.

Impact:

  • Winners receive significantly less rewards than deserved because totalWinnerShares is artificially inflated with duplicate share counts, reducing the numerator in the withdrawal calculation: assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares).

  • The numberOfParticipants counter becomes inaccurate, showing more participants than actually exist, which can break any logic or external systems relying on this metric

Proof of Concept

If a user deposits lets say 1000 tokens and then proceeds to call join event 3 times as i've shown below, the usersAddress array will store duplicates of that user 3 times even though its the same person because there is no check that handles that. The only check now in the joinEvent() is just to ensure the user has deposit. This will affect the winner calculation as the shares will be divided by 3(mind you the 3 addresses are the same person) instead of 1

user.deposit(1000, user);
​
// Exploit: User calls joinEvent multiple times
user.joinEvent(5); // usersAddress = [user]
user.joinEvent(5); // usersAddress = [user, user]
user.joinEvent(5); // usersAddress = [user, user, user]
​
// Result after setWinner(5):
// _getWinnerShares() loops through usersAddress 3 times
// totalWinnerShares = userShares + userShares + userShares = 3x actual shares
​
// When user withdraws:
// assetToWithdraw = (userShares * vaultAsset) / (3 * userShares)
// User receives only 1/3 of what they should get
​

Recommended Mitigation

Add a mapping to track whether a user has already joined the event and revert if they attempt to join again:

// Add state variable
mapping(address => bool) public hasJoined;
​
// Add custom error
error alreadyJoined();
​
// Modify joinEvent function
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
if (hasJoined[msg.sender]) {
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;
​
hasJoined[msg.sender] = true; // Mark as joined
usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}

Also, you can reset by doing hasJoined[msg.sender] = false in the cancelParticipation() function to allow users to rejoin after canceling.

Updates

Appeal created

bube Lead Judge 21 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Duplicate registration through `joinEvent`

Support

FAQs

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

Give us feedback!