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

`BriVault::cancelParticipation` does not clear user state, leaving stale data used in winner calculations

jfornells

BriVault::cancelParticipation does not clear user state, leaving stale data used in winner calculations

Description

  • When a user cancels their participation before the tournament starts, all associated data should be deleted or updated to reflect that they are no longer participating. This prevents them from being considered in the calculation of totalWinnerShares or other metrics.

  • The cancelParticipation() function only burns the shares and returns the funds, but does not clean up the user's related state. The address remains in usersAddress. When _getWinnerShares() is called, these “ghost” entries are still counted, inflating totalWinnerShares and reducing the actual amount received by winners.

@> 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);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood: High

  • This always occurs since the function does not remove their data from mappings or the usersAddress array.

  • Their old records continue to be used in _getWinnerShares(), inevitably affecting the denominator calculation when the owner sets the winner.

Impact: High

  • totalWinnerShares is inflated by participants who no longer have any active deposit, so winners receive less than they should.

  • Surplus funds remain locked in the contract, reducing actual rewards and affecting the vault’s economic integrity.

Proof of Concept

user1 deposits, joins the winner, and cancels before the event starts; the contract does not clean up usersAddress or userSharesToCountry.
Later, when the winner is set, totalWinnerShares includes s1 (from user1) even though they cancelled.
withdraw() for user2 pays less than it should because the denominator is inflated; part of the funds remain locked.

function test_CancelLeavesStaleState_InflatesTotalWinnerShares_AndDilutesPayout() public {
​
// user1 deposits, joins the winner (10), and CANCELS before the event starts
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user1);
uint256 s1 = briVault.balanceOf(user1);
briVault.joinEvent(10);
briVault.cancelParticipation(); // <-- Does NOT clean userToCountry / userSharesToCountry / usersAddress
vm.stopPrank();
​
// user2 deposits and joins the same winner (10)
vm.startPrank(user2);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user2);
uint256 s2 = briVault.balanceOf(user2);
briVault.joinEvent(10);
vm.stopPrank();
​
// --- event ends and winner is set ---
vm.warp(briVault.eventEndDate() + 1);
vm.prank(owner);
briVault.setWinner(10);
​
// 1) Inflated denominator: user1 is counted even though they cancelled
assertEq(briVault.totalWinnerShares(), s1 + s2, "stale user1 shares still counted");
​
// 2) Dilution of user2’s payout (should receive ALL vault funds)
uint256 vaultAsset = briVault.finalizedVaultAsset(); // only user2’s deposit remains
uint256 balBefore = mockToken.balanceOf(user2);
vm.prank(user2);
briVault.withdraw();
uint256 received = mockToken.balanceOf(user2) - balBefore;
​
// with clean state, user2 would receive 'vaultAsset'; with inflation, receives less
assertLt(received, vaultAsset, "payout diluted by inflated totalWinnerShares");
}

Recommended Mitigation

Remove the user's address from the usersAddress array when they cancel participation, to ensure they are not counted in _getWinnerShares() after funds are refunded.
It is also recommended to clean up associated data (userToCountry and userSharesToCountry) for consistency, although the main issue is presence in usersAddress.

function cancelParticipation() public {
(... )
​
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
​
+ for (uint256 i; i < usersAddress.length; i++) {
+ if (usersAddress[i] == msg.sender) {
+ uint256 lastIdx = usersAddress.length - 1;
+ if (i != lastIdx) {
+ usersAddress[i] = usersAddress[lastIdx];
+ }
+ usersAddress.pop();
+ break;
+ }
+ }
​
+ // (Optional) Clean up mappings for consistency
+ delete userToCountry[msg.sender];
​
(... )
}
​
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!