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

Winner Share Calculation Bug - Duplicate Users Cause Inflated totalWinnerShares Leading to Incorrect Payouts

ke5haav

Root + Impact

Description

  • The _getWinnerShares() function calculates the total shares held by all winners by iterating through the usersAddress[] array and summing up shares for users who bet on the winning country. Under normal behavior, each user should appear only once in the array, and their shares should be counted exactly once, resulting in an accurate totalWinnerShares value that is used to calculate proportional payouts for winners.

  • However, due to the Multiple Join Event Exploit, the usersAddress[] array can contain duplicate entries for the same user. When _getWinnerShares() iterates through this array on lines 193-195, it counts the same user's shares multiple times without any deduplication logic. This inflates the totalWinnerShares value, which is then used as the denominator in the payout calculation formula: assetToWithdraw = (shares * vaultAsset) / totalWinnerShares. Since the denominator is inflated, winners receive less than their fair share of the prize pool.

function _getWinnerShares () internal returns (uint256) {
​
@>for (uint256 i = 0; i < usersAddress.length; ++i){
@> address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • This vulnerability occurs when setWinner() is called and the usersAddress[] array contains duplicate entries from users who exploited the Multiple Join Event vulnerability, as there is no deduplication logic in _getWinnerShares() to prevent counting the same user's shares multiple times

  • The bug manifests during the winner share calculation phase when _getWinnerShares() iterates through all entries in usersAddress[] without checking whether a user has already been processed, causing duplicate entries to inflate totalWinnerShares

Impact:

  • Winners receive less than their fair share of the prize pool because totalWinnerShares is inflated, making the denominator larger in the payout calculation formula

  • The financial impact scales with the number of duplicate entries, causing more significant underpayment to winners as more duplicates exist

Proof of Concept

function testWinnerShareCalculationBug() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10); // Join with country 10 (Japan)
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10); // Join with country 10 (Japan)
vm.stopPrank();
​
// User1 exploits Multiple Join bug by joining again
vm.startPrank(user1);
briVault.joinEvent(10); // Join again - creates duplicate entry
vm.stopPrank();
​
// Move to after event end and set winner
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10); // Country 10 wins
vm.stopPrank();
​
// Calculate expected vs actual totalWinnerShares
uint256 user1Shares = briVault.balanceOf(user1);
uint256 user2Shares = briVault.balanceOf(user2);
// Expected: user1Shares + user2Shares (each counted once)
uint256 expectedTotalWinnerShares = user1Shares + user2Shares;
// Actual: user1Shares counted twice + user2Shares (due to duplicate)
uint256 actualTotalWinnerShares = briVault.totalWinnerShares();
console.log("Expected totalWinnerShares:", expectedTotalWinnerShares);
console.log("Actual totalWinnerShares:", actualTotalWinnerShares);
console.log("Inflation:", actualTotalWinnerShares - expectedTotalWinnerShares);
// Verify inflation occurred
assertGt(actualTotalWinnerShares, expectedTotalWinnerShares,
"totalWinnerShares should be inflated due to duplicate counting");
// Calculate expected vs actual payouts
uint256 vaultBalance = briVault.finalizedVaultAsset();
// Expected payout for user1 (if counted correctly)
uint256 expectedPayoutUser1 = (user1Shares * vaultBalance) / expectedTotalWinnerShares;
// Actual payout for user1 (with inflated denominator)
uint256 actualPayoutUser1 = (user1Shares * vaultBalance) / actualTotalWinnerShares;
console.log("Expected payout for user1:", expectedPayoutUser1);
console.log("Actual payout for user1:", actualPayoutUser1);
console.log("Loss for user1:", expectedPayoutUser1 - actualPayoutUser1);
// Verify user1 receives less than they should
assertLt(actualPayoutUser1, expectedPayoutUser1,
"User1 should receive less due to inflated totalWinnerShares");
// Test actual withdrawal
uint256 user1BalanceBefore = mockToken.balanceOf(user1);
vm.startPrank(user1);
briVault.withdraw();
vm.stopPrank();
uint256 user1BalanceAfter = mockToken.balanceOf(user1);
uint256 actualReceived = user1BalanceAfter - user1BalanceBefore;
console.log("User1 actually received:", actualReceived);
assertEq(actualReceived, actualPayoutUser1, "Withdrawal matches calculated payout");
assertLt(actualReceived, expectedPayoutUser1, "User1 received less than fair share");
}

Explanation of PoC:

This proof of concept demonstrates the vulnerability by showing how duplicate entries in the usersAddress[] array cause totalWinnerShares to be inflated, resulting in incorrect payout calculations. The test performs the following steps:

  1. Setup: Two users deposit funds and join the event with the same winning country

  2. Exploit: One user exploits the Multiple Join bug by joining multiple times

  3. Winner Set: Owner sets the winner, triggering _getWinnerShares() calculation

  4. Verification: The test verifies that totalWinnerShares is inflated due to duplicate counting

  5. Payout Calculation: Shows that winners receive less than they should due to the inflated denominator

The test calculates the expected payout (if shares were counted correctly) versus the actual payout (with inflated totalWinnerShares) to demonstrate the financial impact.

Recommended Mitigation

Explanation:

The recommended mitigation fixes the _getWinnerShares() function to prevent duplicate counting by using a mapping to track which users have already been processed. This ensures each user's shares are counted exactly once, regardless of how many times they appear in the usersAddress[] array.

function _getWinnerShares () internal returns (uint256) {
+ totalWinnerShares = 0; // Reset to ensure clean calculation
+ address[] memory processedUsers; // Track processed users to avoid duplicates
​
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+
+ // Check if user has already been processed
+ bool alreadyProcessed = false;
+ for (uint256 j = 0; j < processedUsers.length; j++) {
+ if (processedUsers[j] == user) {
+ alreadyProcessed = true;
+ break;
+ }
+ }
+
+ // Only count shares if user hasn't been processed yet
+ if (!alreadyProcessed) {
+ totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+ processedUsers.push(user);
+ }
}
return totalWinnerShares;
Updates

Appeal created

bube Lead Judge 19 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!