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

Missing Array Cleanup on Participation Cancellation Leading to Duplicate Entries and Incorrect Winner Share Calculations

avspedaliere

Missing Array Cleanup on Participation Cancellation Leading to Duplicate Entries and Incorrect Winner Share Calculations

Description

  • When a user calls briVault::joinEvent, their address is added to the briVault::usersAddress array via push(), and they are registered as a participant. When a user calls briVault::cancelParticipation, they should be removed from the participant list and their address should be removed from the briVault::usersAddress array to prevent them from being counted in future calculations.


  • The briVault::cancelParticipation function does not remove the user's address from the briVault::usersAddress array. When a user cancels and then rejoins the event, their address is pushed to the array again, creating duplicate entries. This directly impacts winner share calculations: the briVault::_getWinnerShares function iterates through the entire briVault::usersAddress array and accumulates userSharesToCountry[user][winnerCountryId] for each address into briVault::totalWinnerShares. If a user appears multiple times due to cancel/rejoin cycles, their shares are counted multiple times, artificially inflating briVault::totalWinnerShares. This inflated value is then used as the divisor in the withdrawal calculation (assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares)), causing legitimate winners to receive less than their fair share of the prize pool.

contract BriVault is ERC4626, Ownable {
​
// ...rest of code...
@> // joinEvent function adds a user address to the usersAddress array
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
​
userToCountry[msg.sender] = teams[countryId];
​
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
@> // cancel function does not remove users from the usersAddress array leading to an inflated array length
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);
}
​
// ...rest of code...
​
}

Risk

Likelihood:

  • This occurs whenever a user calls briVault::joinEvent, then calls briVault::cancelParticipation and then calls briVault::joinEvent again before the event starts, as the briVault::usersAddress array is never cleaned up during cancellation.

  • This occurs every time briVault::cancelParticipation is executed, since the function lacks any logic to remove the user's address from the usersAddress array, leaving stale entries that accumulate over time.

Impact:

  • Winners receive less than their fair share of the prize pool because duplicate entries in briVault::usersAddress cause briVault::totalWinnerShares to be artificially inflated. When briVault::_getWinnerShares iterates through the array, users who canceled and rejoined have their shares counted multiple times, increasing the divisor in the withdrawal calculation (assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares)), which reduces each winner's payout proportionally. If gas costs were ignored this process could lead to the winner's payout nearing a sum of 0.

  • The unbounded loop in briVault::_getWinnerShares processes duplicate entries unnecessarily, increasing gas costs for the briVault::setWinner function. As more users cancel and rejoin, the array grows with duplicates, making each winner calculation more expensive and potentially causing a denial of service attack due to prohibitively expensive gas costs/limits.

Proof of Concept

Add the following test and helper function to briVault.t.sol

function test_usersAddress_array_not_updated_on_cancel_participation() public {
// User deposits and joins event
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
​
// User2 deposits and joins event
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
// User1 cancels participation - array should NOT be updated
vm.startPrank(user1);
briVault.cancelParticipation();
vm.stopPrank();
// User1 rejoins event
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(20);
vm.stopPrank();
// User1 cancels again
vm.startPrank(user1);
briVault.cancelParticipation();
vm.stopPrank();
// User1 rejoins again
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(30);
vm.stopPrank();
// Verify all three entries are the same address (in addition to user2)
assertEq(briVault.usersAddress(0), user1, "First entry should be user1");
assertEq(briVault.usersAddress(1), user2, "Second entry should be user2");
assertEq(briVault.usersAddress(2), user1, "Third entry should be user1 (duplicate)");
assertEq(briVault.usersAddress(3), user1, "Fourth entry should be user1 (duplicate)");
// Verify the array length is 3 (same address added three times, proving array is never cleaned up)
uint256 finalLength = getUsersAddressLength();
assertEq(finalLength, 4, "Array should have length 4 (same address added three times plus user2, proving array is never cleaned up)");
}
​
// Helper function to get the length of usersAddress array by looping and counting
function getUsersAddressLength() internal view returns (uint256) {
uint256 length = 0;
// Loop through array indices until we hit an out-of-bounds error
// Use a reasonable upper bound to avoid infinite loops
for (uint256 i = 0; i < 1000; i++) {
try briVault.usersAddress(i) returns (address) {
length++;
} catch {
break;
}
}
return length;
}

Recommended Mitigation

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);
​
// Remove user from usersAddress array
+ for (uint256 i = 0; i < usersAddress.length; i++) {
+ if (usersAddress[i] == msg.sender) {
// Replace with last element and remove last
+ usersAddress[i] = usersAddress[usersAddress.length - 1];
+ usersAddress.pop();
+ break;
+ }
+ }
​
// Decrement counters
+ numberOfParticipants--;
+ totalParticipantShares -= 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!