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

Multiple Deposits Overwrite stakedAsset Mapping

0xsnoweth

Root + Impact

Description

  • Normal Behavior: When a user calls the deposit() function, the stakedAsset[receiver] state variable should track the total amount of assets the user has deposited (minus participation fees). This mapping is used by cancelParticipation() to determine the refund amount.


  • Vulnerability: The deposit() function uses the assignment operator (=) instead of the accumulation operator (+=) at line 222, causing subsequent deposits to overwrite the previous stakedAsset[receiver] value rather than accumulating it. When a user deposits multiple times, only the most recent deposit amount is tracked, while shares are correctly minted for all deposits. This creates a critical mismatch between the user's actual share balance and their recorded staked amount.

function deposit(
uint256 assets,
address receiver
) public override returns (uint256) {
require(receiver != address(0));
​
​
​
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
​
// charge on a percentage basis points
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
​
uint256 stakeAsset = assets - fee;
​
// Should be using (+=)
@> 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;
}

Risk

Likelihood: HIGH

  • Reason 1: Users naturally deposit multiple times to increase their stake before the tournament starts, especially if they want to adjust their position based on odds or team performance during the deposit phase.


  • Reason 2: The protocol provides no warning or documentation that multiple deposits will cause fund loss, making this a silent failure that users cannot reasonably anticipate.

Impact: HIGH

  • Impact 1: Users lose their first deposit amount permanently when calling cancelParticipation(), receiving only the refund for their last deposit despite having shares for both deposits.


  • Impact 2: The stakedAsset mapping becomes desynchronized with actual share balances, breaking the protocol's accounting invariant that stakedAsset[user] should equal total deposited amount minus fees.


Proof of Concept

function test_user_deposits_twice() public {
address ahmed = makeAddr("ahmed");
mockToken.mint(ahmed, 100 ether);
​
vm.startPrank(ahmed);
mockToken.approve(address(briVault), type(uint256).max);
uint256 assetToDeposit = 50 ether;
uint256 secondAssetToDeposit = 50 ether;
briVault.deposit(assetToDeposit, ahmed);
uint256 stakedAssetBefore = briVault.stakedAsset(ahmed);
console.log("Ahmed's asset depositing for the first time: ", stakedAssetBefore);
// 49.250000000000000000
​
//now we will try to call the function a second time for the purposes of increasing our existing shares:
briVault.deposit(secondAssetToDeposit, ahmed);
uint256 stakedAssetAfter = briVault.stakedAsset(ahmed);
console.log("Ahmed's asset depositing for a second time: ", stakedAssetAfter);
assertEq(stakedAssetAfter, 49250000000000000000, "BUG: second deposit overwrites first");
assertNotEq(stakedAssetAfter, 98500000000000000000, "stakedAsset should be 98.5 ETH but it's not");
}

we get the output:

Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_user_deposits_twice() (gas: 217043)
Logs:
Ahmed's asset depositing for the first time: 49250000000000000000
Ahmed's asset depositing for a second time: 49250000000000000000
​

Recommended Mitigation

Change the assignment operator to accumulation operator:

function deposit(
uint256 assets,
address receiver
) public override returns (uint256) {
require(receiver != address(0));
​
​
​
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
​
// charge on a percentage basis points
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 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!