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

CRITICAL-05: cancelParticipation Doesn't Clean Up joinEvent State

mostafapahlevani93

Root + Impact

When users call cancelParticipation() after joining an event, the function refunds their deposit but fails to clean up their joinEvent state, leaving stale data that corrupts winner calculations.

Description

Normal behavior expects that when a user cancels participation, all their event-related state should be reset including their team selection, participant count, and share tracking. The user should be completely removed from the event.

The current implementation only clears stakedAsset and burns shares, but leaves all joinEvent state intact including userToCountry, userSharesToCountry, presence in usersAddress[], and numberOfParticipants count.

function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
// @> Only clears stakedAsset
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
​
// @> Missing cleanup:
// @> - userToCountry[msg.sender] not deleted
// @> - userSharesToCountry[msg.sender][countryId] not deleted
// @> - usersAddress[] still contains user
// @> - numberOfParticipants not decremented
// @> - totalParticipantShares not decremented
}

Risk

Likelihood:

  • Users who want to withdraw their funds before event starts will naturally use this function

  • No warnings or documentation about the state corruption

  • Common user flow to change mind about participation

Impact:

  • User remains in usersAddress[] array, causing _getWinnerShares() to iterate over them

  • If user's team wins, totalWinnerShares incorrectly includes their phantom shares (now zero)

  • numberOfParticipants inflated, misrepresenting actual participant count

  • userToCountry[user] remains set, indicating they're still in event

  • totalParticipantShares not adjusted, creating accounting mismatch

  • Winner calculations become incorrect, affecting all winner payouts

  • Gas wasted iterating over cancelled users in setWinner()

  • Could cause division errors if all winners cancel but system thinks they exist

Proof of Concept

// User deposits and joins event
user.deposit(1000e18, user);
user.joinEvent(0); // Choose team 0
​
// State:
// stakedAsset[user] = 990
// userToCountry[user] = "TeamA"
// balanceOf(user) = 990 shares
// usersAddress[] = [user]
// numberOfParticipants = 1
// totalParticipantShares = 990
​
// User cancels
user.cancelParticipation();
​
// State after cancel:
// stakedAsset[user] = 0 ✓ Cleared
// userToCountry[user] = "TeamA" ✗ STILL SET
// balanceOf(user) = 0 ✓ Burned
// usersAddress[] = [user] ✗ STILL PRESENT
// numberOfParticipants = 1 ✗ NOT DECREMENTED
// totalParticipantShares = 990 ✗ NOT ADJUSTED
​
// Later when setting winner:
// _getWinnerShares() iterates over usersAddress
// Adds userSharesToCountry[user][0] = 990 to totalWinnerShares
// But user has 0 actual shares now!
// Calculation corrupted

Recommended Mitigation

function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
+
+ // Check if user actually has a deposit
+ if (refundAmount == 0) {
+ revert noDeposit();
+ }
​
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
+ // Clean up joinEvent state if user had joined
+ if (bytes(userToCountry[msg.sender]).length != 0) {
+ // Find and get the countryId (need to iterate or track separately)
+ for (uint256 i = 0; i < teams.length; i++) {
+ if (userSharesToCountry[msg.sender][i] > 0) {
+ userSharesToCountry[msg.sender][i] = 0;
+ break;
+ }
+ }
+
+ delete userToCountry[msg.sender];
+ numberOfParticipants--;
+ totalParticipantShares -= shares;
+
+ // Remove from usersAddress array
+ // Note: This is gas-intensive, consider using mapping instead
+ for (uint256 i = 0; i < usersAddress.length; i++) {
+ if (usersAddress[i] == msg.sender) {
+ usersAddress[i] = usersAddress[usersAddress.length - 1];
+ usersAddress.pop();
+ break;
+ }
+ }
+ }
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+
+ emit ParticipationCancelled(msg.sender, refundAmount);
}

Alternative Better Design:

// Use mapping instead of array for O(1) removal
mapping(address => bool) public hasJoined;
​
function joinEvent(uint256 countryId) public {
// ... existing checks ...
require(!hasJoined[msg.sender], "Already joined");
hasJoined[msg.sender] = true;
// ... rest of logic ...
}
​
function cancelParticipation() public {
// ... existing logic ...
if (hasJoined[msg.sender]) {
hasJoined[msg.sender] = false;
delete userToCountry[msg.sender];
numberOfParticipants--;
// Don't need usersAddress array
}
}
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!