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

Multiple deposits overwrite stake leading to loss of funds

ynyesto

Description

The deposit() function allows users to deposit assets multiple times. When a user deposits, the contract records the staked amount in the stakedAsset mapping for the receiver address. However, the implementation uses assignment instead of accumulation, which means each subsequent deposit overwrites the previous stake amount rather than adding to it.Explain the specific issue or problem in one or more sentences.

`stakedAsset[receiver] = stakeAsset;` overwrites any existing value instead of accumulating deposits. This means if a user deposits 5 ETH and then deposits 3 ETH again, only the 3 ETH is recorded. When the user calls `cancelParticipation()`, they only receive the last deposit amount back, resulting in permanent loss of the previous deposits.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
// ... validation code ...
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset; // @> VULN: Overwrites instead of accumulating
// @> Should be: stakedAsset[receiver] += stakeAsset;
// ... rest of function ...
}

Risk

Likelihood:

  • Users may deposit multiple times during the deposit window before the event starts

  • The contract has no mechanism to prevent or warn about multiple deposits

  • Users expecting their deposits to accumulate will lose funds

Impact:

  • Permanent loss of funds for users who deposit multiple times

  • Users only receive refund for the last deposit amount when cancelling

  • No way to recover overwritten deposit amounts

Proof of Concept

The PoC test below proves that a user who makes two deposits and then cancels their bet only gets refunded for the second deposit.

function testMultipleDepositsOverwriteStake() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
// First deposit: 5 ETH (fee is 1.5%, so stakedAsset = 5 * 0.985 = 4.925 ETH)
uint256 firstDeposit = 5 ether;
briVault.deposit(firstDeposit, user1);
uint256 firstStake = briVault.stakedAsset(user1);
console2.log("After first deposit, stakedAsset:", firstStake);
// Second deposit: 3 ETH (fee is 1.5%, so stakedAsset = 3 * 0.985 = 2.955 ETH)
// Should accumulate to 4.925 + 2.955 = 7.88 ETH, but overwrites to 2.955 ETH
uint256 secondDeposit = 3 ether;
briVault.deposit(secondDeposit, user1);
uint256 secondStake = briVault.stakedAsset(user1);
console2.log("After second deposit, stakedAsset:", secondStake);
// Calculate expected accumulated value (after fees)
uint256 expectedAccumulated = firstStake + (secondDeposit * (10000 - participationFeeBsp) / 10000);
console2.log("Expected accumulated stakedAsset:", expectedAccumulated);
// Verify vulnerability: stakedAsset is overwritten, not accumulated
assertEq(secondStake, secondDeposit * (10000 - participationFeeBsp) / 10000, "VULN: stakedAsset overwritten instead of accumulated");
assertNotEq(secondStake, expectedAccumulated, "Expected accumulated value, but got overwritten value");
// User cancels - only gets back the last deposit amount, loses first deposit
uint256 balanceBeforeCancel = mockToken.balanceOf(user1);
briVault.cancelParticipation();
uint256 balanceAfterCancel = mockToken.balanceOf(user1);
uint256 refunded = balanceAfterCancel - balanceBeforeCancel;
console2.log("Refunded amount:", refunded);
assertEq(refunded, secondStake);
assertNotEq(refunded, expectedAccumulated);
vm.stopPrank();
}
​

Recommended Mitigation

Either make the bets accumulate or allow only one deposit per user. However, please note that simply changing the assignment to accumulation (`+=`) is not sufficient, as it would introduce another bug: if a user calls `joinEvent()`, transfers shares away, calls `cancelParticipation()`, and then repeats this process, the `usersAddress` array will accumulate multiple entries for the same user while `userSharesToCountry` accumulates. This would lead to `totalWinnerShares` being inflated when `_getWinnerShares()` iterates over `usersAddress` entries, allowing the malicious user to get a larger prize than they should if they win the be. The fix must also ensure proper cleanup in `cancelParticipation()`.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
// ... existing code ...
uint256 stakeAsset = assets - fee;
- stakedAsset[receiver] = stakeAsset;
+ stakedAsset[receiver] += stakeAsset;
// ... rest of function ...
}
​
OR ALLOW ONLY ONE DEPOSIT:
​
- function deposit(uint256 assets, address receiver) public override returns (uint256) {
+ function deposit(uint256 assets, address receiver) public onlyOneDeposit override returns (uint256) {
// ... existing code ...
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset;
// ... rest of function ...
}
​
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!