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

[H-01] Deposit bookkeeping overwrites previous user stake

0xch1d3r4n

Root + Impact

Root cause:

The deposit() function directly assigns a new value to stakedAsset[receiver] rather than accumulating it.

Each new deposit replaces the user’s previous recorded stake instead of adding to it, breaking the expected cumulative accounting mode

Impact:

Users who deposit multiple times will lose visibility and accounting of their earlier deposits.

Refunds, cancellations, or withdrawals relying on stakedAsset will only return the most recent deposit value, effectively discarding all prior funds and creating an accounting mismatch between token balances and share supply.

Description

  • Normal Behavior:

    Under correct operation, each new deposit made by a user should accumulate with their previous deposits. The protocol should continuously update stakedAsset[user] to reflect the total amount of collateral the user has staked in the vault, ensuring that all future withdrawals and refunds are based on their full contribution history.

  • Issue:

    Instead of accumulating deposits, the deposit() function overwrites the user’s previous balance in stakedAsset[receiver] each time a new deposit is made. This breaks accounting consistency, as only the most recent deposit is recorded. Earlier deposits are effectively forgotten in storage, leading to incorrect user balances and potential under-refunds during withdrawals or cancellations.

// function deposit(uint256 assets, address receiver) public override returns (uint256) {
...
uint256 fee = _getParticipationFee(assets);
uint256 stakeAsset = assets - fee;
​
- // @> Root cause: The user's previous deposit is overwritten instead of accumulated
// @> stakedAsset[receiver] = stakeAsset;
}
​
​

Risk

Likelihood:

Reason 1: This occurs whenever a user makes multiple deposits, since each new call to deposit() directly reassigns stakedAsset[receiver] without considering any existing balance.

Reason 2: The contract provides no accumulation logic or validation to prevent overwriting, so this mis-accounting behavior is guaranteed under normal user interaction.

Impact:

Impact 1: Users lose credit for earlier deposits, as only the latest deposit remains in storage, breaking consistency between user balances and actual token holdings.

Impact 2: Refunds, cancellations, and withdrawals return incorrect amounts, leading to potential underpayment or permanently locked funds for affected users.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;
​
import "forge-std/Test.sol";
​
/// Minimal ERC20 mintable for tests
contract ERC20Mintable {
string public name = "Mock";
string public symbol = "MCK";
uint8 public decimals = 18;
uint256 public totalSupply;
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
​
function mint(address to, uint256 amount) external {
balanceOf[to] += amount;
totalSupply += amount;
}
​
function approve(address spender, uint256 amount) external returns (bool) {
allowance[msg.sender][spender] = amount;
return true;
}
​
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
if (from != msg.sender) {
uint256 al = allowance[from][msg.sender];
require(al >= amount, "insufficient-allowance");
allowance[from][msg.sender] = al - amount;
}
require(balanceOf[from] >= amount, "insufficient-balance");
balanceOf[from] -= amount;
balanceOf[to] += amount;
return true;
}
}
​
/// Minimal interface of the vault (adjust to real contract if different)
interface IBriVault {
function deposit(uint256 assets, address receiver) external returns (uint256);
function asset() external view returns (address);
function stakedAsset(address user) external view returns (uint256); // public mapping getter
}
​
/// Replace `BriVault` below with the actual contract name and constructor signature
/// Example constructor used in conversation:
/// constructor(IERC20 _asset, uint256 _participationFeeBsp, uint256 _eventStartDate,
/// address _participationFeeAddress, uint256 _minimumAmount, uint256 _eventEndDate)
​
contract H01_DepositOverwrite_PoC is Test {
ERC20Mintable token;
address alice = address(0xBEEF);
address deployer = address(this);
​
IBriVault vault;
​
function setUp() public {
token = new ERC20Mintable();
// mint tokens for alice
token.mint(alice, 1000 ether);
​
// Deploy the real vault contract here.
// -------------------------
// Replace the next commented line with your vault deployment line, for example:
// BriVault v = new BriVault(IERC20(address(token)), 300, block.timestamp + 1 days, deployer, 1e18, block.timestamp + 10 days);
// vault = IBriVault(address(v));
// -------------------------
//
// For the PoC template we assume `vault` is assigned to a deployed vault instance.
//
// NOTE: You must replace above with actual deployment from your repo so the test runs against real code.
}
​
/// @notice Demonstrates that multiple deposits result in vault holding sum(assets) but stakedAsset equals only the last deposit.
function test_deposit_overwrite_behavior() public {
// PRECONDITIONS: ensure vault is deployed in setUp by replacing the placeholder deployment.
​
// sanity check - ensure dev replaced the vault deployment
address vaultAddr = address(vault);
require(vaultAddr != address(0), "Replace placeholder with actual vault deployment in setUp()");
​
// 1) Alice approves vault to take tokens
vm.startPrank(alice);
token.approve(vaultAddr, 500 ether);
​
// 2) Alice deposits 100 tokens
uint256 first = 100 ether;
vault.deposit(first, alice);
​
// 3) Alice deposits another 200 tokens
uint256 second = 200 ether;
vault.deposit(second, alice);
vm.stopPrank();
​
// EXPECTED (correct behavior): stakedAsset[alice] == 300 ether and vault balance == 300 ether
// ACTUAL (buggy behavior): stakedAsset[alice] == 200 ether (last deposit only) while vault balance == 300 ether
​
uint256 recorded = vault.stakedAsset(alice);
uint256 vaultBalance = ERC20Mintable(vault.asset()).balanceOf(vaultAddr);
​
// assert vault holds full tokens deposited
assertEq(vaultBalance, first + second, "Vault token balance should equal sum of deposits");
​
// assert that stored stakedAsset equals only the last deposit (shows overwrite)
// If contract was correct this assertion would be first + second
assertEq(recorded, second, "stakedAsset should equal last deposit (bug demonstrates overwrite)");
}
}

Recommended Mitigation

- stakedAsset[receiver] = stakeAsset;
+ stakedAsset[receiver] += stakeAsset;
Updates

Appeal created

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