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

deposit() Overwrites stakedAsset Mapping, Causing Fund Loss on Multiple Deposits

calvinkimani

Description

The overwriting of stakedAsset[] mapping in BriVault.sol::deposit() will cause a loss of funds for users as they will make multiple deposits before the event, and upon calling cancelParticipation(), only receive a refund for the last deposit amount, losing all previous deposits.

In BriVault.sol:207-237, the deposit() function has an accounting mismatch:

function deposit(uint256 assets, address receiver) public override returns (uint256) {
// ... validation ...
uint256 stakeAsset = assets - fee;
​
stakedAsset[receiver] = stakeAsset; // Line 222: OVERWRITES previous value!
​
uint256 participantShares = _convertToShares(stakeAsset);
// ... transfers ...
_mint(msg.sender, participantShares); // Line 231: ACCUMULATES shares
​
return participantShares;
}

The issue is:

  • stakedAsset[receiver] is overwritten on each call (line 222: = assignment, not +=)

  • Shares are accumulated via _mint() (line 231)

This creates a mismatch where:

  • User deposits 3 times: 10 ETH, 20 ETH, 5 ETH (total ~34.475 ETH after fees)

  • stakedAsset[user] = 4.925 ETH (only last deposit minus fee)

  • balanceOf(user) = 34.475 shares (all deposits accumulated)

When cancelParticipation() is called (lines 275-289):

function cancelParticipation () public {
// ...
uint256 refundAmount = stakedAsset[msg.sender]; // Line 280: Only last deposit!
stakedAsset[msg.sender] = 0;
uint256 shares = balanceOf(msg.sender); // Line 284: All shares
_burn(msg.sender, shares); // Line 286: Burns ALL shares
IERC20(asset()).safeTransfer(msg.sender, refundAmount); // Line 288: Refunds only last deposit!
}

The contract burns all shares but only refunds the last deposit amount, permanently locking the difference.

Risk

The user suffers an approximate loss equal to all deposits except the most recent one. The lost funds become permanently stuck in the contract as there is no mechanism to recover them.

Example calculation:

  • Deposit 1: 10 ETH → 9.85 ETH net

  • Deposit 2: 20 ETH → 19.7 ETH net

  • Deposit 3: 5 ETH → 4.925 ETH net

  • Total deposited: 34.475 ETH net (35 ETH gross)

  • Refund received: 4.925 ETH

  • Loss: 29.55 ETH (~86% loss)

This vulnerability affects honest users who:

  • Want to increase their bet amount incrementally

  • Accidentally call deposit() multiple times

  • Receive tokens from others who deposit on their behalf

POC

User makes multiple deposits, but stakedAsset[] is overwritten each time while shares accumulate.
When user cancels, they only receive refund for the last deposit amount, losing all previous deposits.

function test_LostFunds_MultipleDeposits() public {
vm.startPrank(victim);
token.approve(address(vault), 100 ether);
​
// First deposit: 10 ETH
vault.deposit(10 ether, victim);
uint256 shares1 = vault.balanceOf(victim);
​
// Second deposit: 20 ETH
vault.deposit(20 ether, victim);
uint256 shares2 = vault.balanceOf(victim);
​
// Third deposit: 5 ETH
vault.deposit(5 ether, victim);
uint256 shares3 = vault.balanceOf(victim);
uint256 stakedAsset3 = vault.stakedAsset(victim);
​
// Calculate expected net deposit (35 ETH - 1.5% fees)
uint256 totalNetDeposit = (10 ether * 9850) / 10000 +
(20 ether * 9850) / 10000 +
(5 ether * 9850) / 10000;
​
// Verify accounting mismatch
assertTrue(
vault.stakedAsset(victim) < totalNetDeposit,
"stakedAsset should be less than total deposited"
);
assertGt(shares3, shares2, "Shares should accumulate");
assertGt(shares2, shares1, "Shares should accumulate");
​
// User cancels participation
uint256 balanceBeforeCancel = token.balanceOf(victim);
vault.cancelParticipation();
uint256 refunded = token.balanceOf(victim) - balanceBeforeCancel;
​
vm.stopPrank();
​
// Assertions
assertEq(refunded, stakedAsset3, "Refund should only be for last deposit");
assertTrue(refunded < totalNetDeposit, "User should lose funds");
assertEq(vault.balanceOf(victim), 0, "All shares should be burned");
​
// Verify significant loss
uint256 stuckFunds = totalNetDeposit - refunded;
assertGt(stuckFunds, totalNetDeposit / 2, "Should lose more than half");
}

Mitigation

Change stakedAsset[] assignment to accumulate instead of overwrite:

function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
​
uint256 stakeAsset = assets - fee;
​
- stakedAsset[receiver] = stakeAsset;
+ stakedAsset[receiver] += stakeAsset;
​
uint256 participantShares = _convertToShares(stakeAsset);
​
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
_mint(msg.sender, participantShares);
​
emit deposited (receiver, stakeAsset);
​
return participantShares;
}

This ensures that stakedAsset[] correctly tracks the cumulative net deposits, matching the accumulated share minting behavior.

Updates

Appeal created

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

`stakedAsset` Overwritten on Multiple Deposits

Vault tracks only a single deposit slot per user and overwrites it on every call instead of accumulating the total.

Support

FAQs

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

Give us feedback!