BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

Winner Eligibility Check Inconsistent with Share Allocation

osamaebaid

Root + Impact

Description

  • Users call joinEvent to select a country, allocating their shares to that country via userSharesToCountry[msg.sender][countryId] and updating userToCountry[msg.sender] to the chosen team.

  • After the winner is set, withdraw allows users who picked the winner to redeem shares for assets, checking userToCountry[msg.sender] against the winner and using balanceOf(msg.sender) for payout calculation.

  • The issue is that the eligibility check uses userToCountry[msg.sender] (only the last joined country), while share allocation in _getWinnerShares and payouts use cumulative userSharesToCountry across all joins, leading to mismatches if users join multiple times or change countries.

  • This causes users to be incorrectly denied withdrawals (check fails but shares exist) or receive incorrect payouts (old allocations counted but check passes/fails wrongly).

// In joinEvent - overwrites userToCountry but accumulates shares
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]; // @> Overwrites last country, used in withdraw check
​
uint256 participantShares = balanceOf(msg.sender);
@> userSharesToCountry[msg.sender][countryId] = participantShares; // @> Accumulates without clearing old, used in payouts
​
usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
// In withdraw - checks only last country
function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
@> if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) { // @> Checks only last, inconsistent with cumulative shares
revert didNotWin();
}
uint256 shares = balanceOf(msg.sender);
​
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
​
emit Withdraw(msg.sender, assetToWithdraw);
}

Risk

Likelihood:High

  • Users call joinEvent multiple times with different countries before the event starts.

  • No clearing of previous allocations in userSharesToCountry.

Impact:High

  • Users denied legitimate withdrawals if last country mismatches winner, despite shares in winning pool.

  • Incorrect payouts where old allocations are counted, potentially overpaying or underpaying winners.

Proof of Concept

  • Add testInconsistentEligibility function to briVaultTest.t.sol

  • Run forge test --mt testInconsistentEligibility

function testInconsistentEligibility() public {
// Owner sets countries
vm.prank(owner);
briVault.setCountry(countries);
​
// User deposits 2 ether
vm.prank(user1);
mockToken.approve(address(briVault), 2 ether);
vm.prank(user1);
briVault.deposit(2 ether, user1);
​
// User joins winning country 0 → shares allocated
vm.prank(user1);
briVault.joinEvent(0);
​
// User switches to losing country 1 → previous shares OVERWRITTEN
vm.prank(user1);
briVault.joinEvent(1);
​
// Advance time and set country 0 as winner
vm.warp(eventEndDate + 1 days);
vm.prank(owner);
briVault.setWinner(0);
​
// User tries to withdraw → REVERTS with didNotWin()
vm.expectRevert("didNotWin()");
vm.prank(user1);
briVault.withdraw();
​
// BUT: User's shares were allocated to winning country 0
uint256 sharesInWinnerCountry = briVault.userSharesToCountry(user1, 0);
assertGt(sharesInWinnerCountry, 0, "User had shares in winning country");
​
emit log_string("VULNERABILITY PROVEN: Shares overwritten, not accumulated");
emit log_named_uint("Winning shares lost", sharesInWinnerCountry);
}

Recommended Mitigation

  • Prevent overwrites: accumulate shares per country

  • Optional: Enforce single country choice

  • Or allow multiple but track properly

​
- userSharesToCountry[msg.sender][countryId] = participantShares;
+ userSharesToCountry[msg.sender][countryId] += participantShares;
​
​
+ mapping(address => bool) public hasJoined;
+ if (hasJoined[msg.sender]) revert("Already joined");
+ hasJoined[msg.sender] = true;
​
​
+ userCountryId[msg.sender] = countryId; // Only if single choice allowed
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!