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

Malicious Owner Can Rug Pull the Entire Vault by Setting Self as The Winner

nugen

[H-1] Malicious Owner Can Rug Pull the Entire Vault by Setting Self as The Winner

Description

  • A malicious owner can compromise the vault via briVault::setWinner.

  • Leaving the selection of the winner to the owner carries a significant risk of centralization.

  • The owner can manipulate the event in their favor and eventually drain the vault.

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();
}
​
winnerCountryId = countryIndex;
@> winner = teams[countryIndex];
​
_setWinner = true;
​
_getWinnerShares();
​
_setFinallizedVaultBalance();
​
emit WinnerSet(winner);
​
return winner;
}

Risk

Likelihood:

  • owner might compromise their private key and whoever has access to private key might exploit the event.

  • There may be a motivation to act maliciously (especially if large amounts of money are involved).

Impact:

  • owner can drain the vault.

  • users can lose all their funds.

Proof of Concepts

  1. users & malicious owner make their deposits, join the event and set their bets

  2. owner assigns his bet as the winner

  3. owner wins the event, users lose all their funds

function test_owner_can_drain_vault_by_setting_self_as_winner() public {
uint256 ownerBalance_beforeMinting = briToken.balanceOf(
uint256 balanceBeforeEventStarts = mockToken.balanceOf(address(briVault));
uint256 depositAmount = 5 ether;
​
// victims make deposits
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user2);
vm.stopPrank();
​
vm.startPrank(user3);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user3);
vm.stopPrank();
​
// countries are set
vm.prank(owner);
briVault.setCountry(countries);
​
// victims make their bets
vm.prank(user1);
briVault.joinEvent(2);
​
vm.prank(user2);
briVault.joinEvent(3);
​
vm.prank(user3);
briVault.joinEvent(5);
​
uint256 vaultBalanceBefore = mockToken.balanceOf(address(briVault));
​
// malicious acting starts
// later, the `owner` will claim his pick as the winner.
uint256 ownerDepositAmount = 1 ether;
uint256 ownerInitialBalance = mockToken.balanceOf(owner)
vm.startPrank(owner);
mockToken.mint(owner, ownerDepositAmount);
mockToken.approve(address(briVault),ownerDepositAmount);
briVault.deposit(ownerDepositAmount, owner);
briVault.joinEvent(0); // owner picks a country (usa), later he will make this 'the winning bet'
vm.stopPrank();
​
uint256 ownerShares = briVault.balanceOf(owner);
​
// event starts and ends
vm.warp(eventStartDate + 1);
vm.warp(eventEndDate + 1);
​
// owner sets his pick (0 --> usa) as 'the winning bet'
vm.prank(owner);
briVault.setWinner(0);
​
uint256 ownerBalanceBefore = mockToken.balanceOf(owner);
​
// owner withdraws
vm.prank(owner);
briVault.withdraw();
​
// balance checks
uint256 ownerBalanceAfter = mockToken.balanceOf(owner);
uint256 vaultBalanceAfter = mockToken.balanceOf(address(briVault));
uint256 ownerProfit = ownerBalanceAfter - ownerBalanceBefore;
​
// assertions
assertGt(ownerBalanceAfter, ownerBalanceBefore);
assertEq(ownerProfit, ownerBalanceAfter - ownerBalanceBefore);
assertEq(vaultBalanceAfter, 0);
}
​

Recommended Mitigation

  • You can prevent this manipulation by implementing the randomness from a decentralized oracle such as ChainLink

  1. First, within the root directory of your project, run forge install smartcontractkit/chainlink-brownie-contracts --no-commit

  2. Then, add @chainlink/=/lib/chainlink-brownie-contracts/ to your remappings.txt.

  3. Then, make the provided mitigations below within briVault.sol:

​
+import "@chainlink/contracts/src/v0.8/shared/interfaces/AggregatorV3Interface.sol":
​
contract BriVault is ERC4626, Ownable {
​
+ AggregatorV3Interface public chainlinkOracle;
​
- function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
+ function setWinner() external {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
+ (,int256 winnerIndex,,,) = chainlinkOracle.latestRoundData();
- require(countryIndex < teams.length, "Invalid country index");
+ require(winnerIndex >= 0 && uint256 (winnerIndex) < teams.length, "Invalid oracle data.");
​
if (_setWinner) {
revert WinnerAlreadySet();
}
​
- winnerCountryId = countryIndex;
+ winnerCountryId = uint256(winnerIndex);
- winner = teams[countryIndex];
+ winner = teams[winnerCountryId];
​
_setWinner = true;
_getWinnerShares();
_setFinallizedVaultBalance();
emit WinnerSet(winner);
return winner;
}
}
}
Updates

Appeal created

bube Lead Judge 21 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.

The owner can be participant

The owner is trusted.

Support

FAQs

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

Give us feedback!