Root + Impact

Description

• If we talk in simple terms, this protocol lets users join the tournament/event after they deposit some assets, takes a fee from them, and distributes the assets to the winners.

• Interestingly enough, there's a question one would ask, "What would happen if there were no winner?". Turns out, it will be a major critical scenario as only those users are allowed to withdraw who are termed as winners.

• With this, all assets are literally stuck in the contract with no escape route. The shares users hold are nothing but some funny pixels on the screen. At the end, this protocol will act like a black hole for funds.

  function withdraw() external winnerSet {

  if (block.timestamp < eventEndDate) {

  revert eventNotEnded();

  }

  ​

  @> // No logic for non-winners, in case everyone loses

  ​

  if (

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

  keccak256(abi.encodePacked(winner))

  ) {

  @> revert didNotWin(); // Reverts for all non-winners

  }

  uint256 shares = balanceOf(msg.sender);

  ​

  uint256 vaultAsset = finalizedVaultAsset;

  uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);

  _burn(msg.sender, shares);

  ​

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

  ​

  emit Withdraw(msg.sender, assetToWithdraw);

  }

Risk

Likelihood: Medium/Low

• Reaching such a condition won't be a piece of cake. In case a lot of people tend to participate, then the chances of all teams being selected increase.

• Only requires one team to be unpicked

Impact: High

• Total loss of all user funds (post-fee)

• Irrecoverable - no user or owner can access funds

• Shares become worthless.

• Erodes trust in the protocol among users.

• Break core protocol promise.

Proof of Concept

• Add this test_ZeroWinners_LockAllFunds test in briVault.t.sol:

  function test_ZeroWinners_LockAllFunds() public {

  // Setup

  vm.prank(owner);

  briVault.setCountry(countries);

  ​

  // Setup: 3 users deposit and join wrong teams

  for (uint i = 1; i <= 3; i++) {

  address user = address(uint160(i));

  mockToken.mint(user, 1 ether);

  vm.startPrank(user);

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

  briVault.deposit(1 ether, user);

  briVault.joinEvent(10); // All pick team 10

  vm.stopPrank();

  }

  ​

  // Event starts

  vm.warp(eventStartDate + 1);

  ​

  // Event ends, set winner

  vm.warp(eventEndDate + 1);

  vm.prank(owner);

  briVault.setWinner(47);

  console.log(

  "Total Winner Shares (snapshots):",

  briVault.totalWinnerShares()

  );

  console.log("Finalized Vault Assets:", briVault.finalizedVaultAsset());

  ​

  // All withdrawals fail

  for (uint i = 1; i <= 3; i++) {

  address user = address(uint160(i));

  vm.prank(user);

  vm.expectRevert(); // error(didNotWin)

  briVault.withdraw();

  }

  ​

  console.log();

  console.log("Every withdrawal failed!!!");

  ​

  // Vault still holds funds

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

  }
  
  

• Run the above test using:

  forge test --mt test_ZeroWinners_LockAllFunds -vv
  
  

• Logs:

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

  [PASS] test_ZeroWinners_LockAllFunds() (gas: 2131312)

  Logs:

  Total Winner Shares (snapshots): 0

  Finalized Vault Assets: 2955000000000000000

  ​

  Every withdrawal failed!!!

  Vault leftover: 2955000000000000000

  ​

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

Recommended Mitigation

• Apply an if-else condition in the withdraw() function, which refunds in case we have no winner, i.e. totalWinnerShares == 0

  function withdraw() external winnerSet {

  // ...

  ​

  + if (totalWinnerShares == 0) {

  + // Refund to user or to some charity, as keeping assets here is a waste anyway

  + } else {

  + // Rest of the logic we already have for winners

  + }

  ​

  // ...

  }
  
  

• Additionally, we can auto-refund all with the help of ChainLink Automation after a timeout if totalWinnerShares == 0