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

Multiple Deposit Asset Overwrite Vulnerability

jericho

Root + Impact

Description

  • The deposit function overwrites the stakedAsset[receiver] value instead of accumulating it when the same user makes multiple deposits. This causes loss of previously deposited assets and corrupts the internal accounting system.

// Root cause in the codebase with @> marks to highlight the relevant section
function deposit(uint256 assets, address receiver) public override returns (uint256) {
// ... validation logic ...
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset; // @> Overwrites previous value instead of accumulating
uint256 participantShares = _convertToShares(stakeAsset);
_mint(msg.sender, participantShares);
​
// ... transfer logic ...
return participantShares;
}

Risk

Likelihood:

  • Users may naturally make multiple deposits over time as they acquire more funds

  • The contract doesn't prevent multiple deposits from the same address

  • The overwrite behavior is not immediately apparent to users

  • This violates the principle of least surprise in financial applications

Impact:

  • Permanent loss of funds - previous deposits are overwritten and become unrecoverable

  • Broken accounting - internal state becomes inconsistent with actual token balances

  • Unfair reward distribution - users making multiple deposits get incorrect share allocations

  • Contract becomes unusable - the fundamental accounting mechanism is corrupted


Proof of Concept

verify that user1's stakedAsset balance should be 2 ether minus 2*fee after making two deposits

function test_multipleDepositOverwrite() public {
vm.prank(owner);
briVault.setCountry(countries);
​
// User makes first deposit
vm.startPrank(user1);
mockToken.approve(address(briVault), 2 ether);
briVault.deposit(1 ether, user1);
​
// Verify first deposit is recorded
uint256 fee = (1 ether * participationFeeBsp) / 10000;
assertEq(briVault.stakedAsset(user1), 1 ether - fee);
uint256 firstShares = briVault.balanceOf(user1);
​
// User makes second deposit
briVault.deposit(1 ether, user1);
​
// Check results - stakedAsset is overwritten, not accumulated
assertEq(briVault.stakedAsset(user1), 1 ether - fee); // @> Should be 2 ether - 2*fee, but is only 1 ether - fee
assertEq(briVault.balanceOf(user1), firstShares * 2); // @> Shares are correctly accumulated
}

Recommended Mitigation

This simple change from assignment (=) to accumulation (+=) ensures that multiple deposits from the same address are properly recorded and accounted for, maintaining consistency between the asset tracking and share minting mechanisms.

@@ -219,7 +229,7 @@ contract BriVault is ERC4626, Ownable {
uint256 stakeAsset = assets - fee;
- stakedAsset[receiver] = stakeAsset;
+ stakedAsset[receiver] += stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
Updates

Appeal created

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