Root + Impact
The withdraw() function uses balanceOf(msg.sender) to determine payout, which reflects the current share balance that can change via transfers. This should use the locked userSharesToCountry value captured at joinEvent time.
Description
Normal behavior expects that a user's payout is determined by their shares at the time they joined the event, not their current balance which can be manipulated through transfers.
The current implementation uses balanceOf(msg.sender) which can be freely transferred after joining the event. Users can game the system by transferring shares after joining, or receiving shares from others.
function joinEvent(uint256 countryId) public {
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
}
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);
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:
-
ERC20 shares are transferable by default with no restrictions
-
Users can transfer shares to other addresses freely
-
Multiple users can coordinate to manipulate their withdrawals
Impact:
-
User joins with 1000 shares for Team A
-
User transfers away 500 shares to another address
-
User withdraws based on remaining 500 shares only
-
The transferred 500 shares can be used by recipient who didn't join
-
Creates double-counting if recipient also joined
-
Allows users to split winnings incorrectly
-
Recipient of transferred shares may not have joined winning team but can still benefit
-
Breaks the fundamental rule that only original participants should win
Proof of Concept
alice.deposit(1000e18, alice);
alice.joinEvent(0);
bob.deposit(1000e18, bob);
bob.joinEvent(1);
vault.setWinner(0);
vm.prank(alice);
vault.transfer(bob, vault.balanceOf(alice));
vm.prank(alice);
vm.expectRevert();
vault.withdraw();
vm.prank(bob);
vm.expectRevert(abi.encodeWithSelector(BriVault.didNotWin.selector));
vault.withdraw();
Recommended Mitigation
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);
+ // Use locked shares from when user joined
+ uint256 shares = userSharesToCountry[msg.sender][winnerCountryId];
+
+ // Prevent double withdrawal
+ require(shares > 0, "Already withdrawn or no shares");
+ userSharesToCountry[msg.sender][winnerCountryId] = 0;
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(
shares,
vaultAsset,
totalWinnerShares
);
- _burn(msg.sender, shares);
+ // Burn shares from whoever holds them now
+ // Only burn if user still has them
+ uint256 currentBalance = balanceOf(msg.sender);
+ if (currentBalance >= shares) {
+ _burn(msg.sender, shares);
+ } else if (currentBalance > 0) {
+ _burn(msg.sender, currentBalance);
+ }
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}
Better approach: Make shares non-transferable after joining
+mapping(address => bool) public hasJoinedEvent;
function joinEvent(uint256 countryId) public {
// ... existing checks ...
+ hasJoinedEvent[msg.sender] = true;
// ... rest of function ...
}
+// Override transfer functions to prevent transfers after joining
+function transfer(address to, uint256 amount) public virtual override returns (bool) {
+ require(!hasJoinedEvent[msg.sender], "Cannot transfer after joining event");
+ return super.transfer(to, amount);
+}
+
+function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
+ require(!hasJoinedEvent[from], "Cannot transfer after joining event");
+ return super.transferFrom(from, to, amount);
+}
