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

M05. Missing Cleanup on Cancel Participation

accessdenied

Root + Impact

Description

  • Normally, cancelParticipation() should remove a participant from the event and adjust all related state variables accordingly.

  • The current implementation burns the participant's shares and refunds their staked assets, but it does not remove the participant from usersAddress, does not decrement totalParticipantShares, and does not clear userSharesToCountry, which can lead to incorrect winner share calculations and inflated totals.

// 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];
​
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
@> // Missing: update of usersAddress, totalParticipantShares, and userSharesToCountry
​

Risk

Likelihood:

  • Users cancel their participation before the event start and then the _getWinnerShares() function is called after the event ends.

  • Users can deposit, join, cancel, and rejoin multiple times, leading to duplicate entries in usersAddress.

Impact:

  • Overestimation of totalWinnerShares or totalParticipantShares.

  • Incorrect payouts to winners due to canceled participants still being counted.

  • Potential manipulation of the system by repeatedly joining/canceling.

Proof of Concept

vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
​
// Cancel participation
briVault.cancelParticipation();
​
// Check that usersAddress still contains user1
assertTrue(briVault.usersAddress(0) == user1); // fails consistency
// Check that totalParticipantShares is not updated
assertEq(briVault.totalParticipantShares(), 5 ether); // still counts canceled shares
// Check that userSharesToCountry is not cleared
assertEq(briVault.userSharesToCountry(user1, 10), 5 ether); // still non-zero
vm.stopPrank();
​

Recommended Mitigation

  • Update usersAddress to remove canceled participants.

  • Adjust totalParticipantShares when a user cancels.

  • Clear userSharesToCountry for that user.

  • Consider using a mapping-based replacement for usersAddress to make removal O(1) and reduce gas cost for large events.

- uint256 refundAmount = stakedAsset[msg.sender];
- stakedAsset[msg.sender] = 0;
-
- uint256 shares = balanceOf(msg.sender);
-
- _burn(msg.sender, shares);
-
- IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+ uint256 refundAmount = stakedAsset[msg.sender];
+ uint256 shares = balanceOf(msg.sender);
+
+ stakedAsset[msg.sender] = 0;
+ _burn(msg.sender, shares);
+ IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+
+ // Remove user from usersAddress
+ for (uint256 i = 0; i < usersAddress.length; i++) {
+ if (usersAddress[i] == msg.sender) {
+ usersAddress[i] = usersAddress[usersAddress.length - 1];
+ usersAddress.pop();
+ break;
+ }
+ }
+
+ // Update participant shares and clear country mapping
+ totalParticipantShares -= shares;
+ for (uint256 c = 0; c < teams.length; c++) {
+ userSharesToCountry[msg.sender][c] = 0;
+ }
​

Note: Using a mapping-based structure such as mapping(address => bool) joined or mapping(address => uint256) userIndex could replace the usersAddress array to allow constant-time removal, improving gas efficiency for large participant sets.

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!