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

cancelParticipation() Does Not Clean Up Event Participation State

jufel

Description:

The cancelParticipation() function refunds the user's deposit and burns their shares, but it does not:

  1. Remove the user from the usersAddress array

  2. Clear the userToCountry mapping

  3. Clear the userSharesToCountry mapping

  4. Decrement numberOfParticipants

  5. Update totalParticipantShares

This causes several issues:

  • The user remains in usersAddress and will be counted in _getWinnerShares()

  • If the user later deposits again and joins, they appear twice in usersAddress

  • numberOfParticipants becomes inaccurate

  • Winner share calculations become incorrect

Impact:

  • Incorrect winner share calculations leading to wrong payout amounts

  • Users may receive more or less than they should

  • The usersAddress array grows unboundedly with duplicates

  • Gas costs increase for winner calculation

  • Accounting becomes inconsistent

Proof of Concept:

function testCancelParticipationDoesNotCleanup() public {
uint256 depositAmount = 10000 * 10**18;
​
// Attacker deposits and joins
vm.startPrank(attacker);
asset.approve(address(vault), depositAmount);
vault.deposit(depositAmount, attacker);
vault.joinEvent(0);
vm.stopPrank();
​
uint256 participantsBefore = vault.numberOfParticipants();
uint256 arrayLengthBefore = vault.usersAddress.length;
​
// Attacker cancels
vm.prank(attacker);
vault.cancelParticipation();
​
// State not cleaned up
assertEq(vault.numberOfParticipants(), participantsBefore, "Participants not decremented");
​
// User still in array
assertEq(vault.usersAddress(0), attacker, "User still in usersAddress array");
​
// If attacker deposits again
vm.startPrank(attacker);
asset.approve(address(vault), depositAmount);
vault.deposit(depositAmount, attacker);
vault.joinEvent(1);
vm.stopPrank();
​
// Now appears twice
assertTrue(vault.usersAddress.length > arrayLengthBefore, "Duplicate entries possible");
}

Mitigation:

Clean up all related state in cancelParticipation():

function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
require(refundAmount > 0, "No deposit to cancel");
​
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
​
// Clear participation state
delete userToCountry[msg.sender];
​
// Clear shares for all countries
for(uint256 i = 0; i < teams.length; i++) {
if(userSharesToCountry[msg.sender][i] > 0) {
totalParticipantShares -= userSharesToCountry[msg.sender][i];
userSharesToCountry[msg.sender][i] = 0;
}
}
​
// Remove from usersAddress array (swap with last element and pop)
for(uint256 i = 0; i < usersAddress.length; i++) {
if(usersAddress[i] == msg.sender) {
usersAddress[i] = usersAddress[usersAddress.length - 1];
usersAddress.pop();
break;
}
}
​
numberOfParticipants--;
​
_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!