Root + Impact

Description

• This contract includes two functions, briVault::deposit and briVault::joinEvent. The functionality of these two is clearly understood with just their names.

• One thing to note here is, no user is actually obliged to join the event after they make a deposit. It's under his own free will whether to join the event or not. Maybe he just wants to hold shares, or is hoping for some yield generation, as there's an indication of it in the protocol description, and protocolFlow.txt — talking about DeFi integrations.

• But this proves to be risky for any user. Despite his free will of not being a participant, his assets still get involved in the winning accounting, i.e. given to the winners of the tournament.

• It's like, "I deposited my tokens to hold shares — I never bet — but my money was given to strangers".

• The sole reason it happens is due to the fact that _setFinalizedVaultBalance considers all assets in the vault, and not just the ones that were gambled.

  // line 134 in `setWinner()`

  @> _setFinallizedVaultBalance();

  ​

  ...

  ​

  function _setFinallizedVaultBalance () internal returns (uint256) {

  if (block.timestamp <= eventStartDate) {

  revert eventNotStarted();

  }

  ​

  // considers every asset in the vault

  @> return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));

  }

  ​

  ...

  ​

  // lines 307-308 in `withdraw()`

  uint256 vaultAsset = finalizedVaultAsset;

  @> uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares); // donated to winners

Risk

Likelihood: High/Medium

• There's no obligation that joinEvent MUST be called.

• Anyone can forget, or simply doesn't want to join for some reason.

Impact: Medium

• Users who forget or choose not to join still fund the prize pool

• Winners get inflated payouts (free money from non-bettors)

• Shares of non-joiners become worthless

• Violates "betting" model — turns vault into forced donation

Proof of Concept

• Add the test_NonJoiner_SubsidizeWinners test in briVault.t.sol:

  function test_NonJoiner_SubsidizeWinners() public {

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  console.log("User1 assets balance, before deposit:", mockToken.balanceOf(user1));

  ​

  // User1: deposits, does NOT join

  vm.startPrank(user1);

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

  briVault.deposit(10 ether, user1);

  vm.stopPrank();

  ​

  console.log("User1 assets balance, after deposit:", mockToken.balanceOf(user1));

  ​

  ​

  // User2: deposits, join

  vm.startPrank(user2);

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

  briVault.deposit(10 ether, user2);

  briVault.joinEvent(10);

  vm.stopPrank();

  ​

  // User3: deposits, join

  vm.startPrank(user3);

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

  briVault.deposit(10 ether, user3);

  briVault.joinEvent(10);

  vm.stopPrank();

  ​

  console.log();

  console.log("Total deposits made by User1, User2 and User3:", mockToken.balanceOf(address(briVault)));

  ​

  // Event starts

  vm.warp(eventStartDate + 1);

  // Event ends, owner sets the winner

  vm.warp(eventEndDate + 1);

  vm.prank(owner);

  briVault.setWinner(10);

  ​

  uint256 finalizedVaultAssets = briVault.finalizedVaultAsset();

  console.log("Value of FinalizedVaultAssets:", finalizedVaultAssets);

  ​

  uint256 initialBalanceUser2 = mockToken.balanceOf(user2);

  uint256 initialBalanceUser3 = mockToken.balanceOf(user3);

  ​

  // User2 withdraws

  vm.prank(user2);

  briVault.withdraw();

  ​

  // User3 withdraws

  vm.prank(user3);

  briVault.withdraw();

  ​

  uint256 finalBalanceUser2 = mockToken.balanceOf(user2);

  uint256 finalBalanceUser3 = mockToken.balanceOf(user3);

  ​

  uint256 gainUser2 = finalBalanceUser2 - initialBalanceUser2;

  uint256 gainUser3 = finalBalanceUser3 - initialBalanceUser3;

  ​

  console.log();

  console.log("Total gained by user2 and user3:", gainUser2 + gainUser3);

  console.log("As we can see, user2 and user3 got it all");

  ​

  assertEq(finalizedVaultAssets, (gainUser2 + gainUser3));

  ​

  console.log();

  console.log("User1 assets balance, after event:", mockToken.balanceOf(user1));

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

  }
  
  

• Run it using:

  forge test --mt test_NonJoiner_SubsidizeWinners -vv
  
  

• Logs:

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

  [PASS] test_NonJoiner_SubsidizeWinners() (gas: 2042394)

  Logs:

  User1 assets balance, before deposit: 20000000000000000000

  User1 assets balance, after deposit: 10000000000000000000

  ​

  Total deposits made by User1, User2 and User3: 29550000000000000000

  Value of FinalizedVaultAssets: 29550000000000000000

  ​

  Total gained by user2 and user3: 29550000000000000000

  As we can see, user2 and user3 got it all

  ​

  User1 assets balance, after event: 10000000000000000000

  Vault balance, after event: 0

  ​

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

Recommended Mitigation:

• Rather than simply considering IERC20(asset()).balanceOf(address(this)); as finalizedVaultAsset, the protocol can keep track of only those assets that have been part of the event.

• There's a state variable that's been introduced in this contract, called stakedAmount, but never used. I assume that it was intended to be helpful in such a scenario.

  uint256 public stakedAmount;

  ​

  ...

  ​

  function _setFinallizedVaultBalance () internal returns (uint256) {

  if (block.timestamp <= eventStartDate) {

  revert eventNotStarted();

  }

  ​

  - return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));

  + return finalizedVaultAsset = stakedAmount;

  }

  ​

  ...

  ​

  function joinEvent(uint256 countryId) public {

  // ...

  ​

  numberOfParticipants++;

  totalParticipantShares += participantShares;

  + stakedAmount += stakedAsset[msg.sender];

  }

  ​
  
  

• Alternatively, if this feels too much, then the protocol could combine both deposit and joinEvent such that it becomes mandatory for anyone to participate in the event after he deposits — if that's what the protocol wants.