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

Cancelled Users Remain in Participant Array, Causing Payout Loss to Tournament Winners

yogiemoji

Root + Impact

Description

  • Normally when a player cancels they are completely removed from the tournament and refunded their deposit (minus participation fee).

  • Currently, cancelled players are refunded but are still counted in the usersAddress array with stale userSharesToCountry mappings. This corrupts winner share calculations and dilutes payouts.

function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0; // Good: Clear their deposit
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);// Good: Burn their shares
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);// Good User refunded
@> // @>MISSING THESE 5 THINGS:
// 1. Remove cancelled user from usersAddress array
// 2. Decrement numberOfParticipants
// 3. Subtract from totalParticipantShares
// 4. Clear userToCountry mapping
// 5. Clear userSharesToCountry mapping
}

Risk

Likelihood:

  • This occurs whenever cancelParticipation() is called. The function refunds assets and burns shares
    but fails to remove users from usersAddress or clear userSharesToCountry mappings. When setWinner()
    calls _getWinnerShares(), it loops through usersAddress and sums
    userSharesToCountry[user][winnerCountryId], including cancelled users' stale values. Every
    cancellation creates ghost participants whose phantom shares inflate the payout denominator. This
    occurs in normal operations without special conditions.

Impact:

  • Ghost shares inflate totalWinnerShares (the denominator in payout calculations), causing proportional
    dilution - one cancellation among two winners yields 50% loss, ten among one hundred yields 10%
    loss. Diluted funds lock permanently. Attackers can cheaply grief by creating multiple accounts,
    joining the likely winner, then cancelling to dilute payouts at only gas cost.

Proof of Concept

Add this test to test/briVault.t.sol

function test_cancelledUser_doesNotAffectWinnerShares() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
// User1 joins Japan
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
​
// User2 joins Japan then cancels
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
briVault.cancelParticipation();
vm.stopPrank();
​
// Set Japan as winner
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(10);
​
// CRITICAL: totalWinnerShares should ONLY include user1
// FAILS: Currently includes user2 in iteration (with 0 shares)
uint256 expectedShares = briVault.balanceOf(user1);
assertEq(
briVault.totalWinnerShares(),
expectedShares,
"Cancelled user should not be in winner calculation"
);
}

Add these helper functions to the briVault.sol file. These helper functions expose the array metadata required for comprehensive test verification of state cleanup operations.

function getUsersAddressLength() external view returns (uint256) {
return usersAddress.length;
//Returns the length of the usersAddress array, enabling external callers (including tests) to verify
//the number of addresses stored without direct array access.
}
​
​
function isUserInArray(address user) external view returns (bool) {
for (uint256 i = 0; i < usersAddress.length; i++) {
if (usersAddress[i] == user) return true;
}
return false;
// Iterates through the usersAddress array to verify if a given address exists, returning true if found
// and false otherwise, used primarily for testing state cleanup after cancellations.
}

Run the test:
forge test --mt test_cancelledUser_doesNotAffectWinnerShares -vv

The test demonstrates that the cancelled participant remains in totalWinnerShares, causing user1 to
receive only 50% of their rightful payout (2.4625 ETH instead of 4.925 ETH). The remaining half is permanently locked in the contract. In this two-participant scenario, one cancellation
results in 50% fund loss; with more participants, the loss is proportional to the cancellation rate.

Recommended Mitigation

Explanation:

When a user cancels their participation, the contract should remove them from the usersAddress
array and clear their userSharesToCountry mapping. Adding the _removeUserFromArray() helper
function, invoking it within the updated cancelParticipation() function, and clearing
participant counters prevents cancelled users from remaining as ghost participants whose phantom
shares inflate totalWinnerShares, and dilute legitimate winners' payouts.

Include this helper function in the briVault.sol file. This function ensures cancelled participants are completely removed from the usersAddress array.

+ function _removeUserFromArray(address user) internal {
+ uint256 length = usersAddress.length;
​
+ for (uint256 i = 0; i < length; i++) {
+ if (usersAddress[i] == user) {
+ // Swap user with last element
+ usersAddress[i] = usersAddress[length - 1];
+ // Remove last element
+ usersAddress.pop();
+ return;
+ }
+ }
+ }

Update the cancelParticipation() function with the following additional lines

function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
+ // 1. Decrement the participant counter
+ if (numberOfParticipants > 0) {
+ numberOfParticipants--;
+ }
​
+ // 2. Subtract cancelled users shares from the total
+ if (totalParticipantShares >= shares) {
+ totalParticipantShares -= shares;
+ }
​
+ // 3. Remove them from the usersAddress array
+ _removeUserFromArray(msg.sender);
​
+ // 4. Clear their team selection
+ delete userToCountry[msg.sender];
​
+ // 5. Clear their share allocation to countries
+ for (uint256 i = 0; i < teams.length; i++) {
+ if (userSharesToCountry[msg.sender][i] > 0) {
+ delete userSharesToCountry[msg.sender][i];
+ }
+ }
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
Updates

Appeal created

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

`cancelParticipation` Leaves Stale Winner Data

CancelParticipation burns shares but leaves the address inside usersAddress and keeps userSharesToCountry populated.

Support

FAQs

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

Give us feedback!