BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Denominator drift: shares can change after they’re snapshotted

s3mvl4d

Description

  • Finalization should divide the finalized vault assets among winners using a stable denominator equal to the aggregate winner shares at the exact moment of finalization. That denominator must reflect the same notion of “shares” that will be used at withdrawal time to avoid residuals or under/over‑payments.

  • The contract snapshots each user’s shares at joinEvent() into userSharesToCountry[user][countryId], but users’ actual share balances can change later (e.g., via cancelParticipation() burning shares, or transfers if shares were transferable). At setWinner(), _getWinnerShares() sums the old snapshots into totalWinnerShares, while withdraw() uses the current balanceOf(msg.sender) in the numerator. This mismatch (stale denominator vs. fresh numerator) causes denominator drift, leading to under‑payments and stranded assets.

// joinEvent(): snapshot shares at join time
function joinEvent(uint256 countryId) public {
...
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares; // @> STALE SNAPSHOT
usersAddress.push(msg.sender);
...
}
​
// setWinner(): sums snapshots into totalWinnerShares
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // @> STALE DENOMINATOR
}
return totalWinnerShares;
}
​
// withdraw(): uses current balance as numerator against stale denominator
function withdraw() external winnerSet {
...
uint256 shares = balanceOf(msg.sender); // @> CURRENT SHARES
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares); // @> MISMATCH
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
...
}

Risk

Likelihood: Medium

  • Participants commonly cancel or adjust positions before eventStartDate. Because joinEvent() can be called early and snapshots shares once, any later burns or changes will desynchronize the denominator.

  • The test suite includes cancelParticipation() which burns all shares and refunds stake; users can call joinEvent() before canceling, creating a realistic drift scenario.

Impact: Medium

  • Under‑payment of winners: The denominator (totalWinnerShares) is larger than the sum of winners’ actual shares at withdrawal time, so each withdrawal returns less than intended.

  • Residual funds stuck: Because payouts are computed with a too‑large denominator, leftover assets remain in the vault with no sweep mechanism, effectively locking funds.

Proof of Concept

  • A user joining, then canceling to burn shares.

  • The stale snapshot still contributes to totalWinnerShares, causing denominator drift and reduced payouts for the remaining winners.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "./MockErc20.t.sol";
​
contract DenominatorDriftTest is Test {
BriVault vault;
MockERC20 token;
​
address owner = makeAddr("owner");
address fee = makeAddr("fee");
address w1 = makeAddr("winner1");
address w2 = makeAddr("winner2");
address loser = makeAddr("loser");
​
uint256 start;
uint256 end;
string[48] countries;
​
function setUp() public {
start = block.timestamp + 2 days;
end = start + 30 days;
​
token = new MockERC20("Mock", "M");
token.mint(w1, 10 ether);
token.mint(w2, 10 ether);
token.mint(loser, 10 ether);
​
vm.startPrank(owner);
vault = new BriVault(
IERC20(address(token)),
150, // 1.5%
start,
fee,
0.0002 ether,
end
);
countries[10] = "Japan";
countries[30] = "Italy";
vault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(w1);
token.approve(address(vault), type(uint256).max);
vm.stopPrank();
vm.startPrank(w2);
token.approve(address(vault), type(uint256).max);
vm.stopPrank();
vm.startPrank(loser);
token.approve(address(vault), type(uint256).max);
vm.stopPrank();
}
​
function test_DenominatorDrift() public {
// Three users deposit and join Japan (winner id=10)
vm.startPrank(w1);
vault.deposit(5 ether, w1);
vault.joinEvent(10); // snapshot shares
vm.stopPrank();
​
vm.startPrank(w2);
vault.deposit(5 ether, w2);
vault.joinEvent(10); // snapshot shares
vm.stopPrank();
​
vm.startPrank(loser);
vault.deposit(5 ether, loser);
vault.joinEvent(10); // snapshot shares (counts towards totalWinnerShares)
// Now loser cancels, burning their shares
vault.cancelParticipation();
vm.stopPrank();
​
// Advance time and finalize winner
vm.warp(end + 1);
vm.startPrank(owner);
vault.setWinner(10);
vm.stopPrank();
​
// Withdraws from w1 and w2: each gets LESS than expected due to stale denominator
uint256 before1 = token.balanceOf(w1);
uint256 before2 = token.balanceOf(w2);
​
vm.startPrank(w1); vault.withdraw(); vm.stopPrank();
vm.startPrank(w2); vault.withdraw(); vm.stopPrank();
​
uint256 after1 = token.balanceOf(w1);
uint256 after2 = token.balanceOf(w2);
​
// Assert: payouts are smaller than the correct equal split of finalizedVaultAsset among actual winners
// (Exact check depends on fee & share math; this illustrates the drift effect.)
assertLt(after1 - before1, after2 - before2 + 1); // both under expected ideal
}
}

Recommended Mitigation

  • Freeze share transfers / burns during the event window

+ error transfersDisabledDuringEvent();
+ bool public transfersFrozen;
​
// Enable freezing at join; disable after finalization
function joinEvent(uint256 countryId) public {
...
+ transfersFrozen = true;
}
​
function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
...
_getWinnerShares();
_setFinallizedVaultBalance();
+ transfersFrozen = false; // unfreeze after finalization
...
}
​
// Override _beforeTokenTransfer hook (ERC20) to block transfers when frozen
function _beforeTokenTransfer(address from, address to, uint256 amount) internal override {
super._beforeTokenTransfer(from, to, amount);
+ if (transfersFrozen && from != address(0) && to != address(0)) {
+ revert transfersDisabledDuringEvent();
+ }
}
Updates

Appeal created

bube Lead Judge 21 days ago
Submission Judgement Published
Validated
Assigned finding tags:

`cancelParticipation` Leaves Stale Winner Data

CancelParticipation burns shares but leaves the address inside usersAddress and keeps userSharesToCountry populated.

Support

FAQs

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

Give us feedback!