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

Withdraw Verification Logic - Users Cannot Withdraw Winnings After Joining Multiple Countries

ke5haav

Root + Impact

Description

  • The withdraw() function verifies if a user won by checking if userToCountry[msg.sender] == winner on lines 299-303. Under normal behavior, users who joined the winning country should be able to withdraw their share of the vault, regardless of whether they joined other countries.

  • However, the userToCountry mapping is overwritten each time a user calls joinEvent(), storing only the last country they joined. If a user joins the winning country first, then joins a different country, userToCountry[msg.sender] will show the non-winning country, causing the withdraw function to revert with didNotWin() even though the user has valid shares for the winning country stored in userSharesToCountry[msg.sender][winnerCountryId]. This locks user funds, as they cannot withdraw their legitimate winnings.

// In joinEvent() - line 257
@>userToCountry[msg.sender] = teams[countryId]; // Overwrites previous country
​
// In withdraw() - lines 299-303
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) != // Only checks LAST country
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
// But userSharesToCountry[msg.sender][winnerCountryId] may have valid shares!

Risk

Likelihood

  • This vulnerability occurs when users join multiple countries, as the userToCountry mapping is overwritten on each join, storing only the last country joined

  • The bug manifests during withdrawal attempts when users who joined the winning country first but then joined another country cannot withdraw, even though they have valid shares for the winning country in userSharesToCountry

Impact

  • Users who joined the winning country but then joined another country cannot withdraw their legitimate winnings, causing their funds to be permanently locked in the vault

  • The verification logic incorrectly denies withdrawals to legitimate winners who have valid shares for the winning country, breaking the core functionality of the vault

Proof of Concept

function testWithdrawVerificationLogic() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
// Join winning country first (country 10 - Japan)
briVault.joinEvent(10);
assertGt(briVault.userSharesToCountry(user1, 10), 0, "Has shares for winner");
// Join losing country second (country 20 - Spain)
briVault.joinEvent(20);
// userToCountry now shows last country (Spain, not winner)
string memory lastCountry = briVault.userToCountry(user1);
assertTrue(keccak256(abi.encodePacked(lastCountry)) != keccak256(abi.encodePacked("Japan")));
vm.stopPrank();
// Set winner to country 10
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
vm.stopPrank();
// Try to withdraw - reverts even though user has valid shares
vm.startPrank(user1);
vm.expectRevert(BriVault.didNotWin.selector);
briVault.withdraw(); // Reverts even though user has valid shares for winner
vm.stopPrank();
// Verify shares are locked
assertGt(briVault.balanceOf(user1), 0, "User still has shares but cannot withdraw");
}

Explanation of PoC:

This proof of concept demonstrates how users who joined the winning country but then joined another country cannot withdraw their winnings. The test shows that even though the user has valid shares for the winning country, the withdraw function reverts because userToCountry only stores the last country joined.

Test Results:

  • ✅ User joins winning country first

  • ✅ User joins losing country second (overwrites userToCountry)

  • ✅ User has valid shares for winning country

  • ✅ Withdraw reverts even though user should be able to withdraw

Recommended Mitigation

Explanation:

The recommended mitigation checks if the user has shares for the winning country using userSharesToCountry[owner][winnerCountryId] instead of relying on userToCountry, which only stores the last country joined. This ensures users can withdraw if they have valid shares for the winning country, regardless of which country they joined last.

function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
- if (
- keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
- keccak256(abi.encodePacked(winner))
- ) {
- revert didNotWin();
- }
- uint256 shares = balanceOf(msg.sender);
+ // Check if user has shares for winning country
+ uint256 shares = userSharesToCountry[msg.sender][winnerCountryId];
+ if (shares == 0) {
+ revert didNotWin();
+ }
​
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);
}
Updates

Appeal created

bube Lead Judge 20 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!