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

An user can be pushed multiple times in the userAddress list

dpedpe

An user can join the event multiple times with the same staked assets in briVault::userAddress and the shares at the time his last join are added to the briVault::totalWinnerShares multiple times.

Description

  • Normally the bet of the user should only be added once to the briVault::totalWinnerShares, but if the user appears in the briVault::userAddress several times he's amount or share will be added multiple times into the total share for the winners.

// Root cause in the codebase with @> marks to highlight the relevant section
function joinEvent(uint256 countryId) public {
//@audit if the user deposited twice and withdrawed once, they won't be able to join the event again.
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();
}
​
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);
}
​
function _getWinnerShares() internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i) {
@> address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
​

Risk

Likelihood: Medium

  • Reason 1: A user who does not know this issue may join the game many times and appear many times in the userAddress array.

  • Reason 2: An attacker can dilute the shares of the winner by joining multiple times, but it is not beneficial for him and it will cost him a lot of gas fee.

Impact: High

  • Impact 1: The briVault::totalWinnerShares will be wrong and the participant will not be able to receive the correct amount of prize.


Proof of Concept

In this example, user1 was able to join 50000 times by depositing once.

//this can be run inside briVault.t.sol
function testJoin50000() public {
vm.pauseGasMetering();
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
//USER 1 DEPOSITS AND JOIN EVENT
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
vm.stopPrank();
uint256 numberOfJoin = 50000;
for (uint256 i = 0; i < numberOfJoin; i++) {
vm.prank(user1);
briVault.joinEvent(10);
}
vm.warp(eventEndDate + 1);
vm.roll(block.number + 1);
assertEq(briVault.numberOfParticipants(), numberOfJoin);
}

Recommended Mitigation

Ensure that the stackedAsset of the user is updated when joining the game and check if the user has already joined the event before adding him to the userAddress array.

mapping(address=>bool) userJoined;
function joinEvent(uint256 countryId) public {
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();
}
​
userToCountry[msg.sender] = teams[countryId];
​
- uint256 participantShares = balanceOf(msg.sender);
+ uint256 participantShares = stakedAsset[msg.sender];
+ stakedAsset[msg.sender] = 0;
- userSharesToCountry[msg.sender][countryId] = participantShares;
+ userSharesToCountry[msg.sender][countryId] += participantShares;
+ if(!userJoined){
usersAddress.push(msg.sender);
numberOfParticipants++;
+}
​
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
Updates

Appeal created

bube Lead Judge 19 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!