Description
The protocol is designed so that each user can join only one team. Users who are on the winning team should share the assets in the vault proportionally to their shares.
However, once a user deposits assets into the vault, they can call joinEvent multiple times with different countryId values. The countryId from the last call will determine the team the user is officially assigned to, but each call creates a new entry in userSharesToCountry and appends the user’s address to userAddresses. As a result, _getWinnerShares will compute an inflated totalWinnerShares, since it counts duplicate entries for the same user.
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);
}
function _getWinnerShares () internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
Risk
Likelihood:
There is currently no restriction preventing a user from calling joinEvent multiple times after depositing. This enables a malicious user to manipulate the vault’s accounting with only the minimum deposit amount. Even regular users switching teams before the event starts can unintentionally create the same issue.
Impact:
This vulnerability can cause incorrect reward distribution and permanent asset locking. The totalWinnerShares will be artificially inflated, reducing the payout for legitimate winners and leaving a significant portion of the vault’s assets unclaimable.
Proof of Concept
Add this test to the test suite in test/briVault.t.sol.
function testJoiningMultipleTeamsWillDiluteSharesOfWinnersAndLockTokensInTheVault() public {
vm.prank(owner);
briVault.setCountry(countries);
uint256 depositAmount = 10e18;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
briVault.joinEvent(0);
briVault.joinEvent(1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user2);
briVault.joinEvent(0);
vm.stopPrank();
uint256 user2AmountAfterDeposit = mockToken.balanceOf(user2);
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
uint256 totalAmountInVaultBeforeWithdraw = mockToken.balanceOf(address(briVault));
console.log(briVault.userToCountry(user1));
vm.prank(user1);
vm.expectRevert(BriVault.didNotWin.selector);
briVault.withdraw();
vm.prank(user2);
briVault.withdraw();
uint256 user2WithdrawAmount = mockToken.balanceOf(user2) - user2AmountAfterDeposit;
assertEq(user2WithdrawAmount, totalAmountInVaultBeforeWithdraw / 3);
assertApproxEqAbs(mockToken.balanceOf(address(briVault)), 2 * (totalAmountInVaultBeforeWithdraw / 3), 2);
}
This test shows that a user who joins multiple teams inflates the vault’s accounting. Even though user2 is the only legitimate participant on the winning team, they only receive one-third of the vault’s assets, while two-thirds remain locked due to duplicated entries from user1.
Recommended Mitigation
Modify joinEvent to check whether the user has already joined a team. If they have, update their existing team instead of creating a new entry and pushing their address again. This ensures proper accounting and prevents duplicates in userAddresses.
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
+ uint256 participantShares = balanceOf(msg.sender);
+ string currentCountry = userToCountry[msg.sender];
+ if (bytes(currentCountry).length != 0) {
+ // need to remove shares from previous country
+ delete userSharesToCountry[msg.sender][getCountryIndex(currentCountry)];
+ userSharesToCountry[msg.sender][countryId] = participantShares;
+ userToCountry[msg.sender] = teams[countryId];
+ // dont need to push again to usersAddress or change values of numberOfParticipants and totalParticipantShares
+ } else {
+ userToCountry[msg.sender] = teams[countryId];
+ userSharesToCountry[msg.sender][countryId] = participantShares;
+ usersAddress.push(msg.sender);
+ numberOfParticipants++;
+ totalParticipantShares += participantShares;
+ }
- 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);
}
