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

cancelParticipation() does not clear participation mappings, causing incorrect accounting and misallocated winner rewards

shashankwcw

The cancelParticipation() function only resets stakedAsset[msg.sender] and burns the user’s ERC4626 shares. It does not clear other state variables modified during joinEvent(), such as userToCountry, userSharesToCountry, usersAddress numberOfParticipants and totalParticipantShares counters.

Description

  • When a user cancels participation, the contract should fully remove the user from all participation-related state (staked amount, assigned country, per-country shares, and their presence in the participant list) so they are excluded from any later calculations (e.g., winner-share aggregation).

  • cancelParticipation() only clears stakedAsset and burns the caller’s shares, but it does not clear participation mappings (userToCountry, userSharesToCountry, usersAddress, numberOfParticipants, totalParticipantShares), so cancelled participants remain counted in later winner-share calculations — resulting in incorrect totals and misallocated payouts.


// Root cause in the codebase with @> marks to highlight the relevant section
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
// @> Only stakedAsset is cleared
stakedAsset[msg.sender] = 0;
​
/*
@note :
The following mappings and variables are set during joinEvent():
- userToCountry[msg.sender] = teams[countryId];
- userSharesToCountry[msg.sender][countryId] = participantShares;
- usersAddress.push(msg.sender);
- numberOfParticipants++;
- totalParticipantShares += participantShares;
*/
​
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
​
// @> Although the refund is processed correctly,
// stale participation data causes incorrect totalWinnerShares calculations later
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood:

  • cancelParticipation() is a normal user operation and the cleanup omission is easy to trigger.

  • Any user who calls cancelParticipation() will create the problematic state; realistic in normal usage or tests.

Impact:

  • Incorrect accounting in winner-share aggregation.

Cancelled participants remain counted, inflating totalWinnerShares for the winning country.

  • Can cause incorrect reward calculations (over- or under-allocation), wrong payouts, or withdrawal logic breakage.

Proof of Concept

  • As the test shows after the cancellation by user2 the winner amount is 3940000000000000000

  • After cancellation by winner amount should be 1970000000000000000

// Test
function test_cancelParticipation_math_error() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(2 ether, user1);
briVault.joinEvent(2);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(2 ether, user2);
briVault.joinEvent(5);
vm.stopPrank();
​
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(2 ether, user3);
briVault.joinEvent(1);
vm.stopPrank();
​
// cancel competetion
vm.startPrank(user2);
briVault.cancelParticipation();
vm.stopPrank();
​
vm.startPrank(user4);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(2 ether, user4);
briVault.joinEvent(5);
vm.stopPrank();
​
vm.warp(block.timestamp + 35 days) ;
vm.startPrank(owner);
briVault.setWinner(5);
vm.stopPrank();
​
uint winneramount = briVault.totalWinnerShares();
console.log("Winner amount is : ", winneramount);
}
​
// Output is
Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_cancelParticipation_math_error() (gas: 952136)
Logs:
Winner amount is : 3940000000000000000 : 3940000000000000000
​
Traces:
[991936] BriVaultTest::test_cancelParticipation_math_error()
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
​
│ └─ ← [Return] 3940000000000000000 [3.94e18]
├─ [0] console::log("Amount is : ", 3940000000000000000 [3.94e18]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.80ms (773.20µs CPU time)
​
// The correct winner's amount should be 1970000000000000000
​

Recommended Mitigation

  • Just clear all the mappings and rduce the variables

​
+ uint256 refundAmount = stakedAsset[msg.sender];
+ stakedAsset[msg.sender] = 0;
+ delete userToCountry[msg.sender]; uint participantShares = balanceOf(msg.sender);
+ totalParticipantShares -= participantShares;
+ delete userSharesToCountry[msg.sender][countryId];
+ numberOfParticipants--;
+ //remove the address from the usersAddress array
+ // @note : this is an inefficient way to remove an element from an array
+ usersAddress.pull(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!