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

Users Who Cancel Participation Remain in usersAddress Array, Corrupting totalWinnerShares Calculation

adrianheldesai

Root + Impact

Description

  • Normal Behavior: When users cancel their participation, all their state should be cleared, including their entry in the usersAddress array and their team selection. They should not be counted in winner calculations.

  • The cancelParticipation() function burns shares and refunds the staked amount, but does not remove the user from the usersAddress array or clear their userToCountry and userSharesToCountry mappings. If a user joined an event and then canceled, they remain in usersAddress. When _getWinnerShares() is called, it iterates through usersAddress and tries to add userSharesToCountry[user][winnerCountryId] to totalWinnerShares. Even though the user's shares were burned, the mapping still contains the old value, causing totalWinnerShares to be inflated with phantom shares.

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);
// @> Does not remove user from usersAddress array
// @> Does not clear userToCountry[msg.sender]
// @> Does not clear userSharesToCountry[msg.sender][countryId]
}
​
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
// @> Includes canceled users with stale share data
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • Users who cancel after joining will trigger this issue

  • The stale data remains in mappings

  • Occurs whenever a user joins then cancels

Impact:

  • totalWinnerShares becomes inflated with phantom shares from canceled users

  • All winners receive less than their fair share

  • Funds become locked in the contract as the math doesn't add up

  • Protocol becomes insolvent

Proof of Concept

function test_cancelAfterJoinPhantomShares() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
// User1 deposits and joins
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
// userSharesToCountry[user1][10] = 4.925 ether shares
// usersAddress contains user1
vm.stopPrank();
​
// User2 deposits and joins
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
​
// User1 cancels
vm.startPrank(user1);
briVault.cancelParticipation();
// Shares burned, but userSharesToCountry[user1][10] still = 4.925 ether
// usersAddress still contains user1
vm.stopPrank();
​
// Set winner
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
// totalWinnerShares = user1's phantom shares + user2's real shares
// totalWinnerShares is inflated!
vm.stopPrank();
​
// User2 receives less than 100% of the vault
}

Recommended Mitigation

+ mapping(address => uint256) public userCountryId;
​
function joinEvent(uint256 countryId) public {
// ... existing checks ...
userToCountry[msg.sender] = teams[countryId];
+ userCountryId[msg.sender] = countryId;
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender);
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);
+ // Clear all user state
+ uint256 countryId = userCountryId[msg.sender];
+ userSharesToCountry[msg.sender][countryId] = 0;
+ delete userToCountry[msg.sender];
+ delete userCountryId[msg.sender];
+
+ // Note: Removing from usersAddress array is gas-intensive
+ // Better approach: track active users separately or check balanceOf in _getWinnerShares
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
​
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
+ // Skip users who canceled (have no shares)
+ if (balanceOf(user) == 0) continue;
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
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!