Root + Impact

Description

• The briVault::joinEvent function lets anyone join the event after depositing assets, by passing their favourite countryId. Moreover, it doesn't restrict anyone from calling this function again, in case someone feels like changing their choice, or wants to deposit some more. And maybe, that's a good feature (hopefully!).

• But that's where a critical vulnerability lies. Well, joinEvent does update the user's new country choice on line 257, as well as update the participationShares accordingly on line 260-261. However, it also pushes the user into the briVault::usersAddress array (on line 263), again.

  function joinEvent(uint256 countryId) public {

  // ...

  ​

  // line 257

  userToCountry[msg.sender] = teams[countryId];

  ​

  // lines 260-261

  uint256 participantShares = balanceOf(msg.sender);

  @> userSharesToCountry[msg.sender][countryId] = participantShares;

  ​

  // line 263

  @> usersAddress.push(msg.sender);

  ​

  // ...

  }
  
  

• It will create issues later when the owner sets the winner. In case the country selected by the user (who called the joinEvent() again, or multiple times) wins, then their participationShares will be added multiple times to totalWinnerShares in the _getWinnerShares() function.

• This will lead to totalWinnerShares value inflated more than it should, and as it acts like a denominator during the time of withdrawal, the winners will be receiving much less than they actually should.

  function _getWinnerShares () internal returns (uint256) {

  ​

  @> for (uint256 i = 0; i < usersAddress.length; ++i){ // Loops over `usersAddress`

  address user = usersAddress[i];

  totalWinnerShares += userSharesToCountry[user][winnerCountryId];

  }

  @> return totalWinnerShares; // A increased value

  }

  ​

  // ...

  ​

  function withdraw() external winnerSet {

  // ...

  ​

  uint256 shares = balanceOf(msg.sender);

  ​

  uint256 vaultAsset = finalizedVaultAsset;

  @> uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares); // `totalWinnerShares` act as denominator here

  ​

  // ...

  }

Risks

Likelihood: High/Medium

• It's easy to make such a mistake by any user, but not every user will make such a decision to deposit again

• No restriction on calling joinEvent multiple times tho

Impact: High

• Trims down every winner's payout, almost bringing the system to its knees.

• It's like, everyone pays the price for one user's silly mistake.

• Vault keeps excess funds locked

Proof of Concept

• Here's how this vulnerability unfolds:

  1. Total 4 users: user1, user2, user3, and user4

  2. First 3 users deposit 5e18 tokens, and join the event with countryId - 10, 11, 47, respectively.

  3. Then the 4th user, i.e. user4 deposits 5e18 tokens and calls joinEvent(47).

  4. After a while, something came to his mind, and he thought of depositing 5e18 more tokens. And then he called joinEvent(47) again, in order to get the newly added tokens recorded under userSharesToCountry.

  5. However, this proved harmful as user4 as well as his shares are counted two times, thus increasing the value of totalWinnerShares that acts as a denominator to distribute assets at the time of withdrawal.

  6. Due to this, both user3 and user4 receive much less in assets than they should. Overall, they are at a loss despite being winners.

  7. And the leftover assets are clearly stuck in the contract...

  
  

• Here's the test_UserCanJoinMultipleTimes test, demonstrating the above thing. However, to make the best use of this test, follow these steps:

  • First, run this test as it is and observe the logs.

  • Next, comment out the lines where the 2nd deposit and joinEvent call are made by user4. Looks like this:

    vm.startPrank(user4);

    mockToken.approve(address(briVault), type(uint256).max);

    briVault.deposit(5 ether, user4);

    briVault.joinEvent(47);

    vm.stopPrank();
    
    

  • Compare both logs.

  
  

• Add it in briVault.t.sol:

  function test_UserCanJoinMultipleTimes() public {

  // setup

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  uint256 initialUser3Balance = mockToken.balanceOf(user3);

  uint256 initialUser4Balance = mockToken.balanceOf(user4);

  ​

  // User1 deposits `5e18` tokens and joins the event

  vm.startPrank(user1);

  mockToken.approve(address(briVault), type(uint256).max);

  briVault.deposit(5 ether, user1);

  briVault.joinEvent(10);

  vm.stopPrank();

  ​

  // User2 deposits `5e18` tokens and joins the event

  vm.startPrank(user2);

  mockToken.approve(address(briVault), type(uint256).max);

  briVault.deposit(5 ether, user2);

  briVault.joinEvent(11);

  vm.stopPrank();

  ​

  // User3 deposits `5e18` tokens and joins the event

  vm.startPrank(user3);

  mockToken.approve(address(briVault), type(uint256).max);

  briVault.deposit(5 ether, user3);

  briVault.joinEvent(47);

  vm.stopPrank();

  ​

  console.log("Vault token balance:", mockToken.balanceOf(address(briVault)));

  console.log("totalParticipantShares:", briVault.totalParticipantShares());

  ​

  // User4 deposits `5e18` tokens and joins the event

  vm.startPrank(user4);

  mockToken.approve(address(briVault), type(uint256).max);

  briVault.deposit(5 ether, user4);

  briVault.joinEvent(47);

  vm.stopPrank();

  ​

  // User4 deposits again, for some reason

  vm.startPrank(user4);

  mockToken.approve(address(briVault), type(uint256).max);

  briVault.deposit(5 ether, user4);

  briVault.joinEvent(47);

  vm.stopPrank();

  ​

  console.log();

  console.log("Vault token balance:", mockToken.balanceOf(address(briVault)));

  console.log("totalParticipantShares:", briVault.totalParticipantShares());

  ​

  // Event starts

  vm.warp(eventStartDate + 1);

  ​

  // Event ends, and owner sets the winner

  vm.warp(eventEndDate + 1);

  vm.prank(owner);

  briVault.setWinner(47);

  ​

  vm.prank(user4);

  briVault.withdraw();

  ​

  vm.prank(user3);

  briVault.withdraw();

  ​

  console.log();

  console.log("User3 Assets balance:", mockToken.balanceOf(user3));

  if (initialUser3Balance > mockToken.balanceOf(user3)) {

  console.log("User3 lost", initialUser3Balance - mockToken.balanceOf(user3), "tokens, despite winning");

  } else {

  console.log("User3 gained", mockToken.balanceOf(user3) - initialUser3Balance, "tokens");

  }

  ​

  console.log();

  console.log("User4 Assets balance:", mockToken.balanceOf(user4));

  if (initialUser4Balance > mockToken.balanceOf(user4)) {

  console.log("User4 lost", initialUser4Balance - mockToken.balanceOf(user4), "tokens, despite winning");

  } else {

  console.log("User4 gained", mockToken.balanceOf(user4) - initialUser4Balance, "tokens");

  }

  console.log();

  console.log("totalWinnerShares:", briVault.totalWinnerShares());

  }
  
  

• Run it using the following command:

  forge test --mt test_UserCanJoinMultipleTimes -vv
  
  

• Logs

  • On the default form of test:

    Ran 1 test for test/briVault.t.sol:BriVaultTest

    [PASS] test_UserCanJoinMultipleTimes() (gas: 2371300)

    Logs:

    Vault token balance: 14775000000000000000

    totalParticipantShares: 14775000000000000000

    ​

    Vault token balance: 24625000000000000000

    totalParticipantShares: 29550000000000000000

    ​

    User3 Assets balance: 19925000000000000000

    User3 lost 75000000000000000 tokens, despite winning

    ​

    User4 Assets balance: 19850000000000000000

    User4 lost 150000000000000000 tokens, despite winning

    ​

    totalWinnerShares: 24625000000000000000

    ​

    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.47ms (1.87ms CPU time)
    
    

  • When the 2nd deposit of user4 is commented out:

    Ran 1 test for test/briVault.t.sol:BriVaultTest

    [PASS] test_UserCanJoinMultipleTimes() (gas: 2296479)

    Logs:

    Vault token balance: 14775000000000000000

    totalParticipantShares: 14775000000000000000

    ​

    Vault token balance: 19700000000000000000

    totalParticipantShares: 19700000000000000000

    ​

    User3 Assets balance: 24850000000000000000

    User3 gained 4850000000000000000 tokens

    ​

    User4 Assets balance: 24850000000000000000

    User4 gained 4850000000000000000 tokens

    ​

    totalWinnerShares: 9850000000000000000

    ​

    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.48ms (1.17ms CPU time)
  
  

• The outputs clearly show:

  
  

  Scenario	User3 Change	User4 Change	totalWinnerShares
  Single Join	+4.85e18	+4.85e18	9.85e18 (real)
  Multiple Joins	–0.075e18	–0.15e18	24.625e18 (inflated)

Recommended Mitigation

Totally depends on how the protocol wants to fix it; there are two options:

1. Literally deny any user calling the joinEvent function again. Can be done in such a way:

   + mapping (address => bool) hasJoined;

   // ...

   ​

   function joinEvent(uint256 countryId) public {

   + if (hasJoined[msg.sender]) {

   + revert();

   + }

   ​

   // ...

   ​

   + hasJoined[msg.sender] = true;

   ​

   // ...

   }
   
   

2. Or, if the protocol wants the users to call joinEvent as many times as they want, such that users can change the choice if needed, then add a user to usersAddress for just one time. Can be done in such a way:

   + mapping (address => bool) hasJoined;

   ​

   // ...

   ​

   function joinEvent(uint256 countryId) public {

   // ...

   ​

   - usersAddress.push(msg.sender);

   ​

   + if (!hasJoined(msg.sender)) {

   + usersAddress.push(msg.sender);

   + hasJoined[msg.sender] = true;

   + }

   }