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

`BriVault::joinEvent` allows multiple joins; incorrect share calculation may prevent user withdrawals or cause the withdrawal amount to be less than they should get.

mafa

BriVault::joinEvent allows multiple joins; incorrect share calculation may prevent user withdrawals or cause the withdrawal amount to be less than they should get.

Description

BriVault::joinEvent allows a user to join multiple times, and calculates participantShares as balanceOf(msg.sender). As a result, when a user joins multiple events, their balance is repeatedly counted. In addition, usersAddress.push(msg.sender) causes the same user to be pushed into usersAddress multiple times, which leads to totalParticipantShares being recalculated each time and including assets from the user's previous joins.

Because userToCountry[msg.sender] is updated every time joinEvent is called, even if a user voted for the winning country, their record can be overwritten, preventing them from being able to withdraw.

function joinEvent(uint256 countryId) public {
​
.....
@> 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);
}

The BriVault::_getWinnerShares function repeatedly counts the same user because the user is pushed into usersAddress multiple times. As a result, totalWinnerShares is calculated multiple times for the same user, causing it to exceed the actual amount.

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:

When a user joins the event multiple times, the system calculates participantShares using the total balance and pushes the user’s address into usersAddress repeatedly, causing _getWinnerShares to iterate over duplicate entries and the shares to be counted multiple times.

Impact:

  • Total winner shares become inflated, causing winning users to receive less on withdrawal than they should, and in some cases even less than their original deposit, leading to a loss of funds.

  • A user may choose the correct winner, but due to incorrect share calculations or overwritten records, they could be prevented from withdrawing their funds.

Proof of Concept

Place the following code in briVault.t.sol.

Proof Of Code
function testWithdrawNotWorking() public{
vm.prank(owner);
briVault.setCountry(countries);
vm.startPrank(user1);
// user 1 deposite and join the event choose Country 10
mockToken.approve(address(briVault), 4 ether);
uint256 user1sharesFirst = briVault.deposit(3 ether, user1);
briVault.joinEvent(10);
// user 2 join event second time choose country 14
uint256 user1sharesSecond = briVault.deposit(1 ether, user1);
briVault.joinEvent(14);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 19 ether);
uint256 user2Shares = briVault.deposit(19 ether, user2);
briVault.joinEvent(10);
uint256 balanceBeforuser2 = mockToken.balanceOf(user2);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
console2.log("finalized vault balance:",briVault.finalizedVaultAsset());
vm.stopPrank();
​
console2.log("total Winner Shares:",briVault.totalWinnerShares());
vm.startPrank(user1);
vm.expectRevert(abi.encodeWithSignature("didNotWin()"));
briVault.withdraw();
vm.stopPrank();
​
vm.startPrank(user2);
briVault.withdraw();
console2.log("user2 balance:",mockToken.balanceOf(user2));
assert(mockToken.balanceOf(user2) < 19 ether);
vm.stopPrank();
​
}

The result:

finalized vault balance: 22655000000000000000
total Winner Shares: 24625000000000000000
user2 balance: 18217800000000000000

From the results, we can see that totalWinnerShares is greater than the finalized vault balance, and user2 wins but withdraws even less than their deposit.

Recommended Mitigation

To allow each user to join only once, use a mapping to track whether a user has already joined and revert if they try again. If multiple joins are allowed, calculate only the newly added shares each time instead of using the msg.sender balance, and avoid pushing the same user multiple times into usersAddress.

+ mapping(address => bool) public hasJoined;
​
function joinEvent(uint256 countryId) public {
+ if(hasJoined[msg.sender]){
+ revert alreadyjoined();}
​
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Duplicate registration through `joinEvent`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!