Root + Impact
Description
-
Normal behavior: The vault allows users to deposit ERC20 tokens and receive ERC4626 shares. Users join an event by selecting a team, and after the event ends the owner sets the winning team. Winners withdraw their share of the pooled assets.
-
Issue: withdraw() uses balanceOf(msg.sender) (the user’s live ERC4626 share balance) to compute the payout while totalWinnerShares is computed once at setWinner(). Because shares are standard ERC20 tokens and transferable, losing shares can be moved to a winning address after the snapshot. This inflates the winner’s balance at withdrawal and causes the vault to overpay (or attempt to transfer more than its balance).
function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
...
_getWinnerShares();
...
}
function _getWinnerShares() internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
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);
...
Risk
Likelihood:
-
Occurs after the owner calls setWinner() and before a winning user withdraws.
-
Requires transferable ERC4626/ERC20 shares (true here because vault mints ERC20 shares).
-
Can be executed by a colluding loser or any holder who transfers shares to the winner after snapshot.
Impact:
-
Winners can receive a larger payout than their fair share; the vault may attempt to transfer more tokens than it owns (ERC20InsufficientBalance) resulting in revert/DoS or partial/full drain of the vault.
-
Honest winners may receive reduced rewards or be blocked from withdrawing if the vault balance is exhausted.
-
Financial loss and integrity failure of reward distribution.
Proof of Concept
function test_exploitPostSnapshotInjection() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
uint256 user1Shares = briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
uint256 balanceBeforeUser1 = mockToken.balanceOf(user1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
uint256 user2Shares = briVault.deposit(5 ether, user2);
briVault.joinEvent(20);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
vm.stopPrank();
vm.startPrank(user2);
briVault.transfer(user1, user2Shares);
vm.stopPrank();
vm.startPrank(user1);
vm.expectRevert();
briVault.withdraw();
vm.stopPrank();
}
Recommended Mitigation
- 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);
+ uint256 snapshotShares = userSharesToCountry[msg.sender][winnerCountryId];
+ if (snapshotShares == 0) revert didNotWin();
+
+ uint256 vaultAsset = finalizedVaultAsset;
+ uint256 assetToWithdraw = Math.mulDiv(snapshotShares, vaultAsset, totalWinnerShares);
+
+ // prevent double-claim
+ userSharesToCountry[msg.sender][winnerCountryId] = 0;
+
+ IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
