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

Non-Winner Shares Are Included in Total Supply, Diluting Winner Payouts

jufel

Description:

When calculating withdrawal amounts for winners, the contract uses:

uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);

However, totalWinnerShares only includes shares of users who bet on the winning team. The problem is that vaultAsset (the total balance) includes deposits from ALL participants (winners and losers), but the calculation divides by only the winner shares.

This seems correct at first glance - winners should split the entire pool. However, there's a critical issue: non-winners still hold shares in the vault (they were minted shares but cannot withdraw). These shares remain in totalSupply() and if non-winners haven't burned their shares, it creates accounting inconsistencies.

More critically, if the vault inherits standard ERC4626 functions like totalAssets() or other view functions, they may report incorrect values because they consider all shares, not just winner shares.

Impact:

  • Non-winner shares remain in circulation

  • ERC4626 view functions may return incorrect values

  • If any integration relies on share price or total assets calculations, they will be wrong

  • After winners withdraw, remaining dust amounts in vault with orphaned non-winner shares

  • Vault state becomes inconsistent with actual claimable amounts

Proof of Concept:

function testNonWinnerSharesDilution() public {
// Two users bet on different teams
vm.startPrank(attacker);
asset.approve(address(vault), 10000 * 10**18);
vault.deposit(10000 * 10**18, attacker);
vault.joinEvent(0); // Team 0
vm.stopPrank();
​
vm.startPrank(victim);
asset.approve(address(vault), 10000 * 10**18);
vault.deposit(10000 * 10**18, victim);
vault.joinEvent(1); // Team 1
vm.stopPrank();
​
uint256 totalSupplyBeforeWinner = vault.totalSupply();
​
// Event ends, team 0 wins
vm.warp(block.timestamp + 31 days);
vault.setWinner(0);
​
// Winner withdraws
vm.prank(attacker);
vault.withdraw();
​
// Non-winner still has shares
uint256 victimShares = vault.balanceOf(victim);
assertTrue(victimShares > 0, "Victim still has shares");
​
// Total supply decreased only by winner's shares
uint256 totalSupplyAfter = vault.totalSupply();
assertEq(totalSupplyAfter, victimShares, "Non-winner shares remain");
​
// But vault balance should be ~0 (minus rounding)
uint256 vaultBalance = asset.balanceOf(address(vault));
console.log("Vault balance after winner withdrawal:", vaultBalance);
console.log("Non-winner shares still exist:", victimShares);
​
// This creates an inconsistent state where shares exist but no assets back them
}

Mitigation:

Option 1: Burn non-winner shares when winner is set:

function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
// ...
​
_getWinnerShares();
_setFinallizedVaultBalance();
​
// Burn all non-winner shares
+ _burnNonWinnerShares();
​
emit WinnerSet(winner);
return winner;
}

Add function:

function _burnNonWinnerShares() internal {
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
​
// Check if user is not a winner
if (keccak256(abi.encodePacked(userToCountry[user])) !=
keccak256(abi.encodePacked(winner))) {
​
uint256 shares = balanceOf(user);
if (shares > 0) {
_burn(user, shares);
}
}
}
}

Option 2: Add a claim function for non-winners to explicitly forfeit their shares:

function forfeitShares() external {
require(_setWinner, "Winner not set yet");
require(
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner)),
"Winners cannot forfeit"
);
​
uint256 shares = balanceOf(msg.sender);
if (shares > 0) {
_burn(msg.sender, shares);
}
}
Updates

Appeal created

bube Lead Judge 21 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!