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

Single‑deposit assumption (overwrites stake)

s3mvl4d

Description

  • When a participant deposits multiple times before the event starts, the vault should accumulate their stake so refunds and eligibility checks reflect the sum of all deposits.

  • In deposit(...), the contract overwrites the previous stake instead of accumulating it. As a result, a user who deposits multiple times will have only the last deposit amount recorded in stakedAsset, breaking refunds (cancelParticipation) and checks that rely on the stake value.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
...
uint256 stakeAsset = assets - fee;
​
// @> BUG: overwrites any previous stake for `receiver`
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);
...
}

Risk

Likelihood: Medium

  • Participants often top‑up deposits (UX retries, incremental deposits). Each subsequent deposit will wipe the earlier recorded stake.

  • Scripts or bots splitting deposits into chunks (common in testing/CTF) will trigger this behavior immediately.

Impact: Medium

  • Under‑refund: On cancelParticipation(), the user receives only the last stake instead of the total of all deposits, losing funds.

  • Eligibility/accounting errors: Logic that checks stakedAsset[msg.sender] to gate joinEvent() or compute totals will read an incorrect (too small) amount.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "./MockErc20.t.sol";
​
contract SingleDepositOverwriteTest is Test {
BriVault vault;
MockERC20 token;
​
address owner = makeAddr("owner");
address user = makeAddr("user");
address fee = makeAddr("fee");
​
uint256 start; uint256 end;
​
function setUp() public {
start = block.timestamp + 2 days;
end = start + 30 days;
​
token = new MockERC20("Mock", "M");
token.mint(user, 20 ether);
​
vm.startPrank(owner);
vault = new BriVault(IERC20(address(token)), 150, start, fee, 0.0002 ether, end);
vm.stopPrank();
​
vm.startPrank(user);
token.approve(address(vault), type(uint256).max);
}
​
function test_OverwriteStakeOnMultipleDeposits() public {
// First deposit
uint256 s1 = vault.deposit(5 ether, user);
// Second deposit (top-up)
uint256 s2 = vault.deposit(3 ether, user);
​
// Stake mapping holds only last deposit value
assertEq(vault.stakedAsset(user), 3 ether - ((3 ether * 150) / 10000), "only last stake recorded");
​
// Shares minted for both deposits exist, but refund on cancel returns only last stake
vault.cancelParticipation();
// User got back less than sum of both stakes → overwrite bug confirmed
}
}
​

Recommended Mitigation

  • Accumulate stake rather than overwriting.

- // Record stake for receiver (overwrites previous)
- stakedAsset[receiver] = stakeAsset;
+ // Accumulate stake for receiver
+ stakedAsset[receiver] += 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!