BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: low
Invalid

Winning users can get locked out and losing users can withdraw rewards if the owner forgets to call `setCountry`

darkfox

Description

The owner is expected to call setCountry immediately after the contract has been deployed to set up the teams that users can join.

However, if the owner does not set the countries, users can still join the event using any country ID from 0 to 47. If users join the event and the countries are never set, then all the selected teams of users will be empty strings (""). This allows any user to withdraw assets from the vault after the event ends, since all teams, including the winning team, would be represented by the empty string.

In another scenario, if a user joins the event before the owner calls setCountry, that user will not be able to win because their recorded country will be an empty string. They would need to call joinEvent again after the countries are set to have a valid chance of winning.

// every entry would start as ""
string[48] public teams;
​
function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
@> if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
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:

This will occur only if the owner fails to call setCountry immediately after deployment. It would generally be good practice to include this step in a deployment script.

Impact:

Scenario 1: If the countries are never set, any user can withdraw vault assets after the event since every country name is an empty string. This would allow losing users to withdraw funds and could deplete the vault.

Scenario 2: If the countries are set after users have already joined, those early users will be unable to win because their recorded country remains an empty string. They would need to rejoin before the event starts to have a valid team.

Scenario 3: If the owner sets the countries after the event has already started, no users will correspond to these countries, meaning no one will be able to withdraw vault assets even if they selected the correct team index.

Proof of Concept

Add this test of the first scenario to the test suite in test/briVault.t.sol.

function testAnyoneCanWithdrawIfOwnerForgetsToSetCountries() public {
// Owner does not set the countries
uint256 depositAmount = 5e18;
uint256 base = 10000;
uint256 depositFee = (depositAmount * participationFeeBsp) / base;
uint256 user1StartingAmount = mockToken.balanceOf(user1);
​
// Teams can still be joined since the teams array is string[48]
// Each team will be ""
assert(keccak256(abi.encodePacked(briVault.teams(0))) == keccak256(abi.encodePacked("")));
​
// getCountry() will revert because all teams are length 0
vm.expectRevert(BriVault.invalidCountry.selector);
briVault.getCountry(0);
​
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
briVault.joinEvent(0);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user2);
briVault.joinEvent(1);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
​
vm.prank(owner);
briVault.setWinner(1);
​
vm.prank(user1);
briVault.withdraw();
​
vm.expectRevert();
vm.prank(user2);
briVault.withdraw();
​
// everyone had the same share of the vault and only 1 user was on team 1
// meaning that user 1 got all of the assets in the vault
assert(mockToken.balanceOf(user1) == user1StartingAmount - depositFee + 2 * (depositAmount - depositFee));
}

This test shows that a user on a losing team (country ID 0) can still withdraw after the event ends since both teams are represented by the empty string. This also prevents the user who selected the winning index (user2) from withdrawing their rightful winnings.

Next, add the test of the second scenario.

function testJoinEventBeforeSetCountriesWillMakeItImpossibleToWin() public {
uint256 depositAmount = 5e18;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
briVault.joinEvent(0);
vm.stopPrank();
​
vm.prank(owner);
briVault.setCountry(countries);
​
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user2);
briVault.joinEvent(0);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
​
vm.expectRevert(BriVault.didNotWin.selector);
// user 1 joined the correct team index
vm.prank(user1);
briVault.withdraw();
}

This test shows that a user who joined before the countries were set cannot withdraw their winnings, even if they selected the correct team index.

Recommended Mitigation

This potential issue can be mitigated in multiple ways:

  1. Including setCountry in the deploy script to ensure countries are set alongside contract deployment.

  2. Set a flag to mark when the countries are set. When this flag has not been marked, user facing functions like deposit and joinEvent should be inaccessible.

  3. Set the countries array in the constructor. This will mitigate the risk by setting up the array during deployment. This is the best option.

- constructor (IERC20 _asset, uint256 _participationFeeBsp, uint256 _eventStartDate, address _participationFeeAddress, uint256 _minimumAmount, uint256 _eventEndDate) ERC4626 (_asset) ERC20("BriTechLabs", "BTT") Ownable(msg.sender) {
+ constructor (IERC20 _asset, uint256 _participationFeeBsp, uint256 _eventStartDate, address _participationFeeAddress, uint256 _minimumAmount, uint256 _eventEndDate, string[48] memory _countries) ERC4626 (_asset) ERC20 ("BriTechLabs", "BTT") Ownable(msg.sender) {
if (_participationFeeBsp > PARTICIPATIONFEEBSPMAX){
revert limiteExceede();
}
participationFeeBsp = _participationFeeBsp;
eventStartDate = _eventStartDate;
eventEndDate = _eventEndDate;
participationFeeAddress = _participationFeeAddress;
minimumAmount = _minimumAmount;
_setWinner = false;
+ setCountry(_countries)
}
​
- function setCountry(string[48] memory countries) public onlyOwner {
+ function setCountry(string[48] memory countries) internal {
for (uint256 i = 0; i < countries.length; ++i) {
teams[i] = countries[i];
}
emit CountriesSet(countries);
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!