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

Array Duplication in usersAddress - Duplicate Entries Cause Gas Waste and Incorrect Participant Counting

ke5haav

Root + Impact

Description

  • The joinEvent() function maintains a usersAddress[] array to track all participants. Under normal behavior, each user should appear only once in this array, ensuring efficient iteration and accurate participant counting.

  • However, the joinEvent() function on line 263 performs usersAddress.push(msg.sender) without any duplicate check, allowing the same address to be added multiple times when users call joinEvent() multiple times. When _getWinnerShares() iterates through this array on lines 193-195, it processes duplicate entries unnecessarily, wasting gas. Additionally, numberOfParticipants is incremented on line 265 for each join, even if the user already exists, causing the counter to show more participants than actual unique users.

// In joinEvent() - line 263
@>usersAddress.push(msg.sender); // No duplicate check - same address can be added multiple times
​
@>numberOfParticipants++; // Incremented even for duplicate entries
​
// In _getWinnerShares() - lines 193-195
for (uint256 i = 0; i < usersAddress.length; ++i){ // Iterates through duplicates
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}

Risk

Likelihood:

  • This vulnerability occurs when users call joinEvent() multiple times, as the function adds msg.sender to usersAddress[] on every call without checking if the address already exists in the array

  • The bug manifests during every iteration of _getWinnerShares() and whenever numberOfParticipants is accessed, as duplicate entries cause unnecessary processing and incorrect counts

Impact:

  • Gas costs increase unnecessarily during winner share calculation as _getWinnerShares() iterates through duplicate entries, processing the same user multiple times

  • The numberOfParticipants counter becomes inaccurate, showing more participants than actual unique users

  • The array grows unbounded with duplicates, causing increasing gas costs over time

Proof of Concept

function testArrayDuplication() public {
// Setup: 2 users join normally
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(20);
vm.stopPrank();
assertEq(briVault.numberOfParticipants(), 2, "Should be 2 unique participants");
// Exploit: User1 joins multiple times
vm.startPrank(user1);
briVault.joinEvent(10); // Join again
briVault.joinEvent(10); // Join again
vm.stopPrank();
// Verify duplicates
assertEq(briVault.numberOfParticipants(), 4, "Shows 4 but should be 2 unique");
uint256 user1Count = 0;
for (uint256 i = 0; i < briVault.numberOfParticipants(); i++) {
if (briVault.usersAddress(i) == user1) user1Count++;
}
assertEq(user1Count, 3, "User1 appears 3 times in array");
// Measure gas waste during setWinner iteration
uint256 gasBefore = gasleft();
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10); // Triggers _getWinnerShares() iteration
vm.stopPrank();
uint256 gasUsed = gasBefore - gasleft();
// Verify array has duplicates causing gas waste
assertGt(briVault.numberOfParticipants(), 2, "Array has more entries than unique users");
}

Explanation of PoC:

This proof of concept demonstrates how duplicate entries in the usersAddress[] array cause gas waste and incorrect participant counting. The test shows that when a user joins multiple times, they appear multiple times in the array, causing numberOfParticipants to be incorrect and wasting gas during _getWinnerShares() iterations.

Test Results:

  • ✅ Same user appears multiple times in usersAddress[] array

  • ✅ numberOfParticipants shows incorrect count

  • ✅ Gas costs increase with duplicate entries

Recommended Mitigation

Explanation:

The recommended mitigation prevents duplicate entries by checking if a user has already joined before adding them to the array. This ensures each user appears only once, preventing gas waste and incorrect participant counting.

+ mapping(address => bool) public hasJoined;
​
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
​
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
​
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
​
+ // Prevent duplicate entries in usersAddress array
+ bool isFirstJoin = !hasJoined[msg.sender];
+ if (isFirstJoin) {
+ hasJoined[msg.sender] = true;
+ usersAddress.push(msg.sender);
+ numberOfParticipants++;
+ }
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.

Duplicate registration through `joinEvent`

Support

FAQs

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

Give us feedback!