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

Ghost Share Locks Winner Funds

0xvrka

Root + Impact

Description

  • When a user calls cancelParticipation, they should be fully removed from the event. Their assets should be returned, their shares burned, and their entry in the winner-payout calculation should be erased.

  • The cancelParticipation function only burns the user's shares and returns their assets. It fails to clean up the userSharesToCountry mapping or remove the user from the usersAddress array. This leaves "ghost shares" in the system. When _getWinnerShares is called, it iterates over the usersAddress array and includes these ghost shares in the totalWinnerShares calculation, massively inflating the denominator and diluting the payout for all legitimate winners.

function cancelParticipation () public {
// ...
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares); // <-- Shares are burned...
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
@> // Issue -> No cleanup of userSharesToCountry.
@> // userSharesToCountry[msg.sender][countryId] is still > 0.
}
​
function _getWinnerShares () internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i){ // <-- Loops over stale array
address user = usersAddress[i];
@> // Adds "ghost shares" from users who already canceled
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • This is a guaranteed side effect of normal user behavior. Any user who joins and then cancels will poison the share pool for their chosen country.

Impact:

  • Legitimate winners will receive a diluted payout, as the total pot is divided by an inflated totalWinnerShares. The portion of the payout corresponding to the "ghost shares" becomes permanently locked in the contract, unclaimable by anyone.

Proof of Concept

This PoC (test_cancelerSharesIfWinStillCounted) demonstrates the attack.

  1. user1 deposits, joins country 0, and then calls cancelParticipation. user1 gets their money back, but their "ghost shares" remain in userSharesToCountry[user1][0] and their address remains in usersAddress.

  2. user2 deposits and joins country 0.

  3. The owner sets country 0 as the winner. _getWinnerShares is called.

  4. The function loops usersAddress (containing user1 and user2) and adds both of their shares, even though user1's shares are "ghosts."

  5. 'When user2 withdraws, their payout is calculated with an inflated denominator (shares_user1 + shares_user2), so they only receive a fraction of the funds they are entitled to.

function test_cancelerSharesIfWinStillCounted() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 20 ether);
briVault.deposit(20 ether, user1);
briVault.joinEvent(0);
@> briVault.cancelParticipation(); // User1 leaves "ghost shares"
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 20 ether);
briVault.deposit(20 ether, user2);
briVault.joinEvent(0); // User2 is a legitimate participant
vm.stopPrank();
​
vm.startPrank(owner);
vm.warp(eventEndDate + 1);
@> briVault.setWinner(0); // _getWinnerShares counts user1's ghost shares
vm.stopPrank();
uint256 bal = mockToken.balanceOf(user1);
vm.startPrank(user2);
@> briVault.withdraw(); // User2's payout is diluted
bal = mockToken.balanceOf(user2);
console.log("user 2 balance (ETH):", bal / 1 ether, ".", bal % 1 ether);
}

Output Log:

[PASS] test_cancelerSharesIfWinStillCounted() (gas: 1838813)
Logs:
user 2 balance (ETH): 9 . 850000000000000000

Recommended Mitigation

The only robust fix is to add state to track which countryId a user has joined, so cancelParticipation can fully clear their state.

contract BriVault is ERC4626, Ownable {
// ...
+ mapping(address => uint256) public userCountryId;
// ...
​
function joinEvent(uint256 countryId) public {
// ... (checks) ...
+ userCountryId[msg.sender] = countryId;
userToCountry[msg.sender] = teams[countryId];
userSharesToCountry[msg.sender][countryId] = participantShares;
// ...
}
​
​
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
+ // Clear the "ghost shares"
+ uint256 countryId = userCountryId[msg.sender];
+ userSharesToCountry[msg.sender][countryId] = 0;
+
+ // Clear all other state for the user
+ delete userCountryId[msg.sender];
+ delete userToCountry[msg.sender];
​
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
IERC25(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!