Shares miscalculation on cancel
Description
-
Normal behavior: Cancelling participation before the event starts should fully revert the participant’s state (address removed from participant lists, country selection cleared, recorded shares cleared, and counters/totals updated) so future winner calculations exclude the cancelled user.
-
Issue: cancelParticipation() burns the user’s current shares and refunds their staked assets, but it does not clean up the participant accounting set during joinEvent. As a result, the cancelled user remains counted in usersAddress, numberOfParticipants, totalParticipantShares, and userSharesToCountry, causing inflated totalWinnerShares and underpayment to legitimate winners.
function joinEvent(uint256 countryId) public {
...
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
...
}
function cancelParticipation () public {
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
}
function _getWinnerShares () internal returns (uint256) {
totalWinnerShares = 0;
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
Risk
Likelihood: High
-
Happens whenever a joined user cancels before start; the stale state persists into winner selection.
-
Common user action, no safeguards to clean state make this likely in practice.
Impact: High
Proof of Concept
Description:
-
user1 deposits and joins a country
-
user1 cancels participation before start.
-
Contract still counts user1 in all participant metrics and userSharesToCountry.
-
user2 enters and joins a country
-
After event ends and winner is set, user2 (legitimate winner) withdraws less than expected; tokens remain in the vault.
-
user2 should have all winner shares and get the full prize pool
-
user2 actually only has a portion of the total win shares counted, and only gets back a portion
-
the rest of the funds remain locked in the vault forever
function testSharesMiscalculationOnCancel() public {
vm.prank(owner);
briVault.setCountry(countries);
vm.startPrank(user1);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user1);
briVault.joinEvent(7);
vm.stopPrank();
vm.startPrank(user1);
briVault.cancelParticipation();
vm.stopPrank();
vm.assertEq(briVault.numberOfParticipants(), 1);
vm.assertEq(briVault.totalParticipantShares(), 985000000000000000);
vm.assertEq(briVault.usersAddress(0), user1);
vm.assertEq(briVault.userSharesToCountry(user1, 7), 985000000000000000);
vm.startPrank(user2);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user2);
briVault.joinEvent(7);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(7);
vm.stopPrank();
vm.startPrank(user2);
console.log("total winner shares", briVault.totalWinnerShares());
console.log("total user2 shares", briVault.balanceOf(user2));
briVault.withdraw();
vm.stopPrank();
console.log("balance user 2", mockToken.balanceOf(user2));
console.log("tokens left in the vault", mockToken.balanceOf(address(briVault)));
}
Recommended Mitigation
-
Clean up all participant accounting on cancel if the user had joined:
-
Clear the user’s recorded winning shares for their selected country.
-
Decrement numberOfParticipants, subtract from totalParticipantShares, remove the address from usersAddress.
-
Clear userToCountry and a new userCountryId used to track the selection efficiently.
-
Reset totalWinnerShares to 0 at the start of _getWinnerShares() before summing (defensive).
-
Add userCountryId and userIndexInArray mappings to support efficient cleanup.
+ mapping(address => uint256) public userCountryId;
+ mapping(address => uint256) private userIndexInArray;
function joinEvent(uint256 countryId) public {
...
// track chosen country and index for O(1) removal
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
+ userCountryId[msg.sender] = countryId;
+ userIndexInArray[msg.sender] = usersAddress.length - 1;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
...
}
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
+ // remove accounting if user had joined
+ uint256 countryId = userCountryId[msg.sender];
+ if (bytes(userToCountry[msg.sender]).length != 0) {
+ uint256 recordedShares = userSharesToCountry[msg.sender][countryId];
+ if (recordedShares != 0) {
+ totalParticipantShares -= recordedShares;
+ userSharesToCountry[msg.sender][countryId] = 0;
+ }
+ numberOfParticipants--;
+ // swap & pop from usersAddress
+ uint256 idx = userIndexInArray[msg.sender];
+ uint256 lastIdx = usersAddress.length - 1;
+ if (idx != lastIdx) {
+ address lastUser = usersAddress[lastIdx];
+ usersAddress[idx] = lastUser;
+ userIndexInArray[lastUser] = idx;
+ }
+ usersAddress.pop();
+ delete userIndexInArray[msg.sender];
+ delete userToCountry[msg.sender];
+ delete userCountryId[msg.sender];
+ }
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
function _getWinnerShares () internal returns (uint256) {
+ totalWinnerShares = 0;
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
Optionally, prevent duplicate joins per address (this would simplify cleanup and accounting a bit):
function joinEvent(uint256 countryId) public {
+ require(bytes(userToCountry[msg.sender]).length == 0, "already joined");
...
}
