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

cancelParticipation() doesn’t unwind event state

s3mvl4d

Description

  • When a user cancels before eventStartDate, the vault should fully undo their participation: remove/deduct their shares from any per‑country totals, clear their userToCountry / userSharesToCountry snapshot(s), deduplicate the participants list, and adjust aggregate counters (e.g., numberOfParticipants, totalParticipantShares) so finalization and payouts remain correct.

  • cancelParticipation() only burns the caller’s shares and refunds stakedAsset[msg.sender]. It does not (a) remove the caller from usersAddress, (b) decrement numberOfParticipants, (c) reduce totalParticipantShares, or (d) clear their userSharesToCountry entry / userToCountry mapping. It also does not adjust any running totals (e.g., countryShares if you added that mitigation). These stale entries are later consumed by _getWinnerShares() and other logic, producing overcounted denominators, skewed payouts, and possible DoS due to bloated arrays.

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);
​
// @> Missing: remove user from usersAddress / decrement numberOfParticipants
// @> Missing: reduce totalParticipantShares
// @> Missing: clear userSharesToCountry and userToCountry
// @> Missing: reduce any per-country aggregates (e.g., countryShares[cid])
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood: High

  • Users commonly test/join and later cancel before the event starts; UIs may attempt multiple joins/cancels (retries).

  • A griefer can join many times (inflating usersAddress) and then cancel, leaving stale entries that still get counted during finalization.

Impact: High

  • Incorrect winner denominator / under‑payments: Stale userSharesToCountry snapshots (and duplicate usersAddress entries) inflate totalWinnerShares, causing winners to receive less and leaving residual assets stuck.

  • Finalize DoS risk: setWinner() loops over usersAddress in the current design; stale duplicates from canceled users bloat the loop, increasing gas and risking out‑of‑gas at finalize.

  • Misleading analytics/state: numberOfParticipants and totalParticipantShares become inaccurate, undermining integrity of the event.

Proof of Concept

  • The test shows that after canceling, the user remains represented in state, causing inflated accounting at finalize.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "./MockErc20.t.sol";
​
contract CancelDoesNotUnwindStateTest is Test {
BriVault vault;
MockERC20 token;
​
address owner = makeAddr("owner");
address u1 = makeAddr("u1");
address u2 = makeAddr("u2");
address fee = makeAddr("fee");
​
uint256 start;
uint256 end;
string[48] countries;
​
function setUp() public {
start = block.timestamp + 2 days;
end = start + 30 days;
​
token = new MockERC20("Mock", "M");
token.mint(u1, 10 ether);
token.mint(u2, 10 ether);
​
vm.startPrank(owner);
vault = new BriVault(
IERC20(address(token)),
150, // 1.5%
start,
fee,
0.0002 ether,
end
);
countries[10] = "Japan";
countries[20] = "Spain";
vault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(u1); token.approve(address(vault), type(uint256).max); vm.stopPrank();
vm.startPrank(u2); token.approve(address(vault), type(uint256).max); vm.stopPrank();
}
​
function test_CancelLeavesStaleParticipation() public {
// u1 participates then cancels
vm.startPrank(u1);
vault.deposit(5 ether, u1);
vault.joinEvent(10);
vault.cancelParticipation(); // burns shares & refunds but does not unwind participant state
vm.stopPrank();
​
// u2 is a valid participant
vm.startPrank(u2);
vault.deposit(5 ether, u2);
vault.joinEvent(10);
vm.stopPrank();
​
// Finalize
vm.warp(end + 1);
vm.startPrank(owner);
vault.setWinner(10);
vm.stopPrank();
​
// Withdraw for u2; payout is reduced due to stale denominator from u1's snapshot still counted
uint256 bBefore = token.balanceOf(u2);
vm.prank(u2);
vault.withdraw();
uint256 bAfter = token.balanceOf(u2);
​
// We can't assert exact numbers without inspecting internal totals,
// but the presence of stale entries reduces the payout compared to a clean state.
assertGt(bAfter - bBefore, 0, "u2 received something");
// A stronger check would compare with expected split after manually removing u1 contributions,
// showing that u2's payout is less than the correct amount.
}
}

Recommended Mitigation

  • Implement full unwind on cancel and guard participation to avoid duplicates.

​
@@
+ error alreadyJoined();
+ mapping(address => bool) internal hasJoined;
​
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) { revert noDeposit(); }
if (countryId >= teams.length) { revert invalidCountry(); }
if (block.timestamp > eventStartDate) { revert eventStarted(); }
+ if (hasJoined[msg.sender]) { revert alreadyJoined(); }
​
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender);
+ hasJoined[msg.sender] = true;
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
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);
​
+ // Unwind snapshots: clear the user's recorded country & shares
+ for (uint256 cid; cid < teams.length; ++cid) {
+ if (userSharesToCountry[msg.sender][cid] != 0) {
+ userSharesToCountry[msg.sender][cid] = 0;
+ break;
+ }
+ }
+ userToCountry[msg.sender] = "";
​
+ // Deduplicate/remove user from participants list (compact in-place)
+ if (hasJoined[msg.sender]) {
+ hasJoined[msg.sender] = false;
+ // remove one occurrence of msg.sender; if duplicates exist from earlier bugs, remove all
+ for (uint256 i = 0; i < usersAddress.length; ++i) {
+ if (usersAddress[i] == msg.sender) {
+ usersAddress[i] = usersAddress[usersAddress.length - 1];
+ usersAddress.pop();
+ i--; // continue removing if multiple entries exist
+ }
+ }
+ // Update aggregates
+ if (numberOfParticipants > 0) { numberOfParticipants -= 1; }
+ if (totalParticipantShares >= shares) { totalParticipantShares -= shares; }
+ }
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
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!