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

cancelParticipation Fails to Fully Revert User State, Leading to Incorrect Payouts and Loss of Funds for Winners

luigi

cancelParticipation Fails to Fully Revert User State, Leading to Incorrect Payouts and Loss of Funds for Winners

Description

  • The cancelParticipation function is intended to allow users to withdraw their stake before the event begins. While the function successfully refunds the user's staked assets and burns their corresponding vault shares, it fails to completely revert all state changes associated with their participation.

    Specifically, when a user joins an event, their address is added to the usersAddress array, and their shares are recorded in the userSharesToCountry mapping. The cancelParticipation function does not remove the user from usersAddress or clear their data from userSharesToCountry.


    As a result, the user becomes a "ghost participant"—their funds have been returned, but the contract still considers them part of the event's share accounting.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Whenever there is a player who cancel participation by calling cancelParticipation

Impact:

  • This incomplete state reversal has a critical financial impact on the final payout for legitimate winners.

    The _getWinnerShares function calculates the total shares of all winning participants by iterating through the usersAddress array. Because this array still contains the addresses of users who have cancelled, their "ghost" shares are incorrectly included in the totalWinnerShares calculation.


    This inflates the totalWinnerShares value, which is used as the denominator in the withdraw function to determine each winner's portion of the prize pool:

    assetToWithdraw = (user_shares * total_assets) / totalWinnerShares


    Because the denominator is artificially high, the assetToWithdraw for every legitimate winner is significantly lower than it should be. This directly results in a loss of funds for the winners, and the remaining assets that should have been distributed are permanently locked in the vault.

Proof of Concept

Copy and paste to test/BriVault.t.sol

function test_cancelParticipation_leadsToIncorrectPayout() public {
// 1. Setup: Three users deposit and bet on the same country (10)
uint256 depositAmount = 10 ether;
uint256 fee = (depositAmount * participationFeeBsp) / 10000;
uint256 stakedAmount = depositAmount - fee;
​
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
briVault.joinEvent(10);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user2);
briVault.joinEvent(10);
vm.stopPrank();
​
vm.startPrank(user3);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user3);
briVault.joinEvent(10);
vm.stopPrank();
​
// 2. Action: User3 cancels their participation
vm.prank(user3);
briVault.cancelParticipation();
​
// 3. Conclude Event: Set the winner
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(10);
​
// 4. Verification: Check user1's payout
// The total prize pool should only be the stakes from user1 and user2.
// Since user3's stake was refunded, the total assets to be distributed are stakedAmount * 2.
// The winners (user1 and user2) should split this pool.
// Due to minor share differences from sequential deposits, we check that user1 gets roughly half.
uint256 correctTotalPool = stakedAmount * 2;
uint256 expectedPayout = correctTotalPool / 2; // Approximately
​
uint256 balanceBeforeWithdraw = mockToken.balanceOf(user1);
vm.prank(user1);
briVault.withdraw();
uint256 balanceAfterWithdraw = mockToken.balanceOf(user1);
​
uint256 actualPayout = balanceAfterWithdraw - balanceBeforeWithdraw;
​
// Because of the bug, user3's shares are counted, so the pool is split 3 ways instead of 2.
// The actual payout will be much less than the expected payout.
assertLt(actualPayout, expectedPayout, "Actual payout is less than expected due to bug");
assertApproxEqAbs(actualPayout, correctTotalPool / 3, 1, "Actual payout is roughly 1/3 of the pool, not 1/2");
}

Recommended Mitigation

  • Exclude players who cancelled participation when calculating the winner's share

+ mapping (address => bool) public hasCancelled;
​
function _getWinnerShares () internal returns (uint256) {
​
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+ if (!hasCancelled[user]) {
+ totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+ }
}
return totalWinnerShares;
}
​
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
​
+ hasCancelled[msg.sender] = true;
​
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
Updates

Appeal created

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