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

Reentrancy in withdraw / redeem flows allows draining vault assets

eze001

Root + Impact

Description

  • The withdrawal path performs external token transfers (interactions) before updating internal accounting (effects). This ordering allows a malicious contract or token to reenter the vault contract and call withdraw/redeem repeatedly, draining more assets than entitled. The contract also lacks ReentrancyGuard protections around these sensitive functions.

  • Explain the specific issue or problem in one or more sentences

function withdraw(uint256 assets) external {
uint256 shares = _sharesOf[msg.sender];
​
@> assetToken.transfer(msg.sender, assets); // external interaction happens first
​
@> _sharesOf[msg.sender] = _sharesOf[msg.sender] - shares; // state updated after transfer
}
​

Risk

Likelihood:

  • The issue occurs whenever a withdraw or redeem function executes an external token transfer before decrementing the caller's recorded shares or balances.

  • It occurs whenever a user deposits via a contract that can call back into the vault during token transfer (for example via ERC-777 hooks or a malicious recipient that triggers a reentrant call).

Impact:

  • Impact 1: A malicious actor can repeatedly reenter withdraw logic and withdraw more than their entitled share, potentially draining all assets from the vault.

  • Impact 2: Vault accounting becomes inconsistent (negative balances or mismatched totals), resulting in denial-of-service for honest users and irrecoverable loss of funds.

Proof of Concept

Explanation:
three minimal contracts: a token that calls back on transfer, a vulnerable vault that transfers before state updates, and an attacker that reenters via the token callback.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
/// Minimal token that calls tokenCallback on recipient if present
contract MaliciousToken {
mapping(address => uint256) public balanceOf;
function mint(address to, uint256 amt) external { balanceOf[to] += amt; }
function transfer(address to, uint256 amt) external returns (bool) {
require(balanceOf[msg.sender] >= amt);
balanceOf[msg.sender] -= amt;
balanceOf[to] += amt;
// callback if recipient is a contract implementing tokenCallback
(bool ok, ) = to.call(abi.encodeWithSignature("tokenCallback(address,uint256)", msg.sender, amt));
(void(ok));
return true;
}
function transferFrom(address from, address to, uint256 amt) external returns (bool) {
return transfer(to, amt); // simplified for PoC (no allowance checks)
}
}
​
/// Vulnerable vault: transfers token BEFORE updating shares
contract VulnerableVault {
MaliciousToken public token;
mapping(address => uint256) public shares;
constructor(MaliciousToken _t) { token = _t; }
function deposit(uint256 amt) external {
require(token.transferFrom(msg.sender, address(this), amt));
shares[msg.sender] += amt;
}
function withdraw(uint256 amt) external {
require(shares[ms]()
​

Recommended Mitigation

Fix reentrancy by updating internal state before any external token transfers (checks → effects → interactions), so a callback finds the caller’s balance already reduced. Add OpenZeppelin’s ReentrancyGuard (nonReentrant) as defense‑in‑depth and use SafeERC20 for robust token transfers. Test the patch against malicious-token/ERC‑777-style callbacks to confirm the exploit is blocked.

- // vulnerable withdraw implementation (external transfer before state update)
- function withdraw(uint256 assets) external {
- uint256 shares = _sharesOf[msg.sender];
- require(shares > 0, "no shares");
-
- // Interaction happens before Effects — vulnerable to reentrancy
- assetToken.transfer(msg.sender, assets);
-
- // Effects executed after external call
- _sharesOf[msg.sender] = _sharesOf[msg.sender] - shares;
- teamTotalShares[userTeam[msg.sender]] -= shares;
- totalAssets -= assets;
- emit Withdraw(msg.sender, assets, shares);
- }
+ // Add protections: ReentrancyGuard + Checks-Effects-Interactions + SafeERC20
+ using SafeERC20 for IERC20;
+
+ // contract should inherit OpenZeppelin's ReentrancyGuard
+ // contract MyVault is ReentrancyGuard { ... }
+
+ function withdraw(uint256 assets) external nonReentrant {
+ uint256 shares = _sharesOf[msg.sender];
+ require(shares > 0, "no shares");
+
+ // --- Checks & Effects first (prevent reentrancy)
+ _sharesOf[msg.sender] = _sharesOf[msg.sender] - shares;
+ teamTotalShares[userTeam[msg.sender]] -= shares;
+ totalAssets -= assets;
+
+ // --- Interactions last (external transfer)
+ IERC20(assetToken).safeTransfer(msg.sender, assets);
+
+ emit Withdraw(msg.sender, assets, shares);
+ }
​
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!