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

Post-Join Top-Ups Cause Winner Withdrawals to Revert

galer ah

Root + Impact

Description

joinEvent snapshots balanceOf(msg.sender) once and stores it in userSharesToCountry[msg.sender][countryId] (src/briVault.sol:260-262). Subsequent deposits increase the share balance, but the mapping remains stale. _getWinnerShares later uses the outdated value, so the denominator is smaller than the actual winning supply. The custom withdraw() multiplies the current (larger) share balance by the vault asset snapshot, causing SafeERC20.safeTransfer to revert because the computed amount exceeds available funds.

function joinEvent(uint256 countryId) public {
@> uint256 participantShares = balanceOf(msg.sender);
@> userSharesToCountry[msg.sender][countryId] = participantShares;
}
​
function _getWinnerShares() internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i) {
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
}

Risk

Likelihood: High – adding funds after joining is intuitive for users who want to scale their bet.

Impact:

  • Any winner who tops up after joining becomes unable to withdraw.

  • All winners on the same team are locked out because totalWinnerShares is understated.

Proof of Concept

  1. User deposits 5 ether, calls joinEvent(team).

  2. User deposits another 2 ether before eventStartDate.

  3. Owner finalizes in favour of that team.

  4. withdraw() reverts with "ERC20: transfer amount exceeds balance".
    (See test/BriVaultAttack.t.sol:202, testSecondDepositAfterJoinBreaksWinnerWithdrawal.)

function testSecondDepositAfterJoinBreaksWinnerWithdrawal() public {
briVault.deposit(FIRST_DEPOSIT, winner);
briVault.joinEvent(0);
briVault.deposit(SECOND_DEPOSIT, winner); // balance increases, mapping stale
​
vm.prank(owner);
briVault.setWinner(0);
​
vm.prank(winner);
vm.expectRevert("ERC20: transfer amount exceeds balance");
briVault.withdraw();
}

Recommended Mitigation

  1. Refresh userSharesToCountry whenever a participant’s share balance changes (additional deposits, mints, burns).

  2. Alternatively, discard the cached mapping and compute payout shares from live balances during withdrawal.

  3. Extend testing to cover multi-deposit scenarios and ensure the denominator tracks the circulating supply.

Proposed patch (Solidity-like pseudocode):

function deposit(uint256 assets, address receiver) public override returns (uint256 shares) {
- stakedAsset[receiver] += stakeAsset;
+ stakedAsset[receiver] += stakeAsset;
@@
- _afterShareChange(receiver);
+ _afterShareChange(receiver);
}
​
function _afterShareChange(address user) internal {
@@
- userSharesToCountry[user][country] = balanceOf(user);
+ userSharesToCountry[user][country] = balanceOf(user);
}
​
function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
@@
- _getWinnerShares();
+ totalWinnerShares = _computeWinnerShares();
}
Updates

Appeal created

bube Lead Judge 16 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
bube Lead Judge 12 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Deposits after joining the event are not correctly accounted

Support

FAQs

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

Give us feedback!