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

Users that `joinEvent` and then `deposit` later will have shares not accounted for in `totalWinningShares`

darkfox

Description

The protocol expects users to first call deposit and then subsequently call joinEvent. This allows users to join a team and have their shares correctly recorded in userSharesToCountry. This mapping is later used to calculate totalWinningShares for the winning team so that vault assets can be distributed proportionally to winners based on their shares.

However, if a user deposits, joins the event, and then continues to deposit without calling joinEvent again, the additional shares from subsequent deposits will not be reflected in userSharesToCountry. As a result, when calculating totalWinningShares, the protocol will underestimate the true number of shares belonging to users on the winning team.

This allows a malicious user to withdraw more assets than their actual share, while other users that redeem later will be unable to claim their portion because the vault has been drained.

The root cause of this issue is the separation of the deposit and joinEvent logic, which leads to inconsistent accounting of participant shares.

Risk

Likelihood:

A malicious user can exploit this by depositing a minimal amount, calling joinEvent, and then making additional deposits. Since there is no restriction on depositing after joining, this situation is likely to occur, especially when users want to increase their stake before the event begins.

Impact:

The accounting for reward distribution will be incorrect, resulting in a low totalWinningShares value. Early withdrawers will receive more than they deserve, while later withdrawers may be blocked from claiming because the vault is either empty or holds too low of a balance.

Proof of Concept

Add this test to your test suite in test/briVault.t.sol.

function testDepositsAfterJoinEventWillIncreaseMailiciousUserPayoutAndLockTokensForOtherWinners() public {
vm.prank(owner);
briVault.setCountry(countries);
​
uint256 depositAmount = 5e18;
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 2 * depositAmount);
briVault.deposit(2 * depositAmount, user1);
briVault.joinEvent(0);
vm.stopPrank();
​
// malicious user
vm.startPrank(user2);
mockToken.approve(address(briVault), 2 * depositAmount);
briVault.deposit(depositAmount, user2);
briVault.joinEvent(0);
briVault.deposit(depositAmount, user2);
vm.stopPrank();
​
// Note: both users have deposited the same amount and joined the same team
​
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
​
uint256 totalInVault = mockToken.balanceOf(address(briVault));
​
// malicious user
vm.prank(user2);
briVault.withdraw();
uint256 user1WithdrawAmount = totalInVault - mockToken.balanceOf(address(briVault));
console.log(user1WithdrawAmount);
​
vm.expectRevert();
vm.prank(user1);
briVault.withdraw();
}

This test shows that the malicious user (user2), who should have received 50% of the vault (10e18), actually receives 66.7% (13.33e18). When the other legitimate winner attempts to withdraw, the transaction reverts because the vault no longer holds enough tokens.

Recommended Mitigation

To prevent this issue, integrate the deposit and joinEvent logic. Doing so ensures that each deposit is tied to a specific team and that all shares are correctly included in team accounting.

- function deposit(uint256 assets, address receiver) public override returns (uint256) {
+ function deposit(uint256 assets, address receiver, uint256 countryId) public override returns (uint256) {
...
+ joinEvent(countryId);
return participantShares;
}
​
- function joinEvent(uint256 countryId) public {
+ function joinEvent(uint256 countryId) internal {
...
}

It will also be be necessary to update the joinEvent logic to account for multiple deposits and attempts to change teams.

Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
darkfox Submitter
19 days ago
bube Lead Judge
15 days ago
bube Lead Judge 15 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Deposits after joining the event are not correctly accounted

Support

FAQs

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

Give us feedback!