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

CRITICAL-02: Deposit Function Overwrites Previous Deposits Instead of Accumulating

mostafapahlevani93

Root + Impact

When a user calls deposit() multiple times, each call overwrites the stakedAsset value instead of accumulating it, causing permanent loss of previously deposited funds.

Description

Normal behavior expects that if a user deposits 100 tokens and then deposits 50 more tokens, their total staked amount should be 150 tokens tracked in stakedAsset[user].

The current implementation uses direct assignment (=) instead of accumulation (+=), so the second deposit overwrites the first deposit amount. The user loses access to their initial deposit while shares are correctly accumulated.

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;
​
// @> This overwrites previous deposits instead of accumulating
stakedAsset[receiver] = stakeAsset;
​
uint256 participantShares = _convertToShares(stakeAsset);
​
IERC20(asset()).safeTransferFrom(
msg.sender,
participationFeeAddress,
fee
);
​
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
// @> Shares are correctly accumulated via _mint
_mint(msg.sender, participantShares);
​
emit deposited(receiver, stakeAsset);
​
return participantShares;
}

Risk

Likelihood:

  • Users depositing in multiple transactions to manage their investment gradually will trigger this bug

  • No on-chain validation prevents multiple deposits from the same user

  • Common user behavior in DeFi to dollar-cost-average or add to positions over time

Impact:

  • Direct permanent loss of funds for users making multiple deposits

  • stakedAsset[user] tracking becomes incorrect, affecting refund calculations in cancelParticipation()

  • Users who deposit 1000 tokens in two 500-token transactions only have 500 tokens recorded

  • If they cancel participation, they only receive refund for the last deposit (500 tokens), losing 500 tokens permanently

  • Breaks user trust and creates unpredictable behavior

  • Lost tokens remain locked in contract with no recovery mechanism

Proof of Concept

// User deposits 1000 tokens
vault.deposit(1000e18, user);
// stakedAsset[user] = 990 (after 1% fee)
​
// User deposits 500 more tokens
vault.deposit(500e18, user);
// stakedAsset[user] = 495 (OVERWRITES! Lost 990 tokens)
​
// User cancels participation
vault.cancelParticipation();
// Refund = 495 tokens only
// Lost: 990 tokens permanently locked in contract

Recommended Mitigation

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;
}
Updates

Appeal created

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