BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Calculating the shares based on the balanceOf(msg.sender) can result in unfair distribution of shares.

hawksvision

Calculating the shares based on the balanceOf(msg.sender) can result in unfair distribution of shares.

Description

  • Consider that two users are betting the same amount on a specific team to win and upon winning both of them will receive equal amount of assets from the pooled assets as they both bet the same amount on the winning team.

  • In this scenario user1 enter the tournament and places the bet on a team lets say 2 ethers then user2 enter and places his bet on a different team lets say he places 5 ethers then he changes his mind and places the bet on the winning team and he also places 2 ethers for the winning team.

  • as per the rules when the tournament is over both users should receive equal amount of assets because they both places same amount of bet on the winning team but since the user2 has already placed 5 ethers on a losing team and the calculation of the asset to withdraw uses balanceOf(msg.sender) which would eventually increase the number of shares for user2 instead of just the 2 shares he placed on the winning asset.


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);
}// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • there is a chance of user changing his mind and depositing on the winning team even though they intially placed the bet on a different team - they will actually be able to minimize the loss or retrive most of the misplaced bet by betting on the winning team.

Impact:

  • user1 will recieve less amount of assets even though they bet the same as user2 on the same team.

Proof of Concept

  1. Three users enter the tournament and places bets of 2 ethers each. users 1 and 2 places the bets on the winning team and user 3 places bet on the losing team.

  2. Upon the end of tournament both users 1 and 2 will receive equal amount of split which would be around 3 ethers each. This is the normal scenerio.

  3. In the exploit scenario - user 1 places bet on the winning team whereas the user 2 places a bet on a losing team first and due to change of mind he places a bet on the winning team after the intial bet.

  4. in this case upon tournament end user2 will actually recieve more assets as the asset to be withdrawn is calculated using balanceOf(msg.sender) instead of userSharesToCountry[msg.sender][winnerCountryId]. This will also decrease the amount of assets received by the user1 as well.

function test_withDrawIssue() public{
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
//user1 enters the tournament and bets on the winning team
vm.startPrank(user1);
mockToken.approve(address(briVault), 2 ether);
uint256 user1Shares = briVault.deposit(2 ether, user1);
briVault.joinEvent(10);
uint256 balanceBeforuser1 = mockToken.balanceOf(user1);
vm.stopPrank();
//user2 enters the tournament and bets on a losing team
vm.startPrank(user2);
mockToken.approve(address(briVault), 15 ether);
briVault.deposit(15 ether, user2);
briVault.joinEvent(30);
vm.stopPrank();
​
//user2 changes his mind and places the bet on the winning team
//note: the bet amount on the winning team is same as that of the user1
vm.startPrank(user2);
mockToken.approve(address(briVault), 2 ether);
uint256 user2Shares = briVault.deposit(2 ether, user2);
briVault.joinEvent(10);
uint256 balanceBeforuser2 = mockToken.balanceOf(user2);
uint256 user2BriBalance = briVault.balanceOf(user2);
vm.stopPrank();
//user3 enters and bets on a losing team
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
uint256 user3Shares = briVault.deposit(5 ether, user3);
briVault.joinEvent(30);
vm.stopPrank();
//tournament ends
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
uint256 finalAsset=briVault.finalizedVaultAsset();
console.log(briVault.finalizedVaultAsset());
vm.stopPrank();
//the amount user1 should receive is calculated
uint256 withdrawalAmountOfUser1 = Math.mulDiv(user1Shares, finalAsset, (user1Shares + user2Shares));
vm.startPrank(user2);
briVault.withdraw();
vm.stopPrank();
​
vm.startPrank(user1);
briVault.withdraw();
vm.stopPrank();
// the user1 receives the amount lesser than expected amount
assertLt(mockToken.balanceOf(user1), balanceBeforuser1 + withdrawalAmountOfUser1);
​
}

Recommended Mitigation

use userSharesToCountry[msg.sender][winnerCountryId] instead of the balanceOf(msg.sender) calculate the winning shares by the user.

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 shares = userSharesToCountry[msg.sender][winnerCountryId];
​
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);
}
- remove this code
+ add this code
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Inflation attack

Support

FAQs

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

Give us feedback!