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

Multiple deposits for the same user will overwrite `stakedAsset`

darkfox

Description

The protocol allows users to deposit assets multiple times. Each deposit increases the user’s vault shares and potential payout if their team wins.

However, when recording the user’s stakedAsset in the deposit function, the protocol overwrites the existing value instead of adding to it. This behavior causes an issue if the user decides to withdraw their assets before the event starts through cancelParticipation. In this case, the user will only be refunded the amount from their most recent deposit (minus the participation fee), effectively losing their earlier deposits.

This issue does not affect users who call joinEvent and participate in the event normally, since their shares are properly accounted for in the vault. The problem specifically affects users who make multiple deposits but later choose to cancel their participation before the event begins.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
...
​
uint256 stakeAsset = assets - fee;
​
@> 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;
}
​
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){
revert eventStarted();
}
​
@> uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
_burn(msg.sender, shares);
​
@> IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood:

This occurs whenever a user makes multiple deposits before deciding to cancel their participation, which is a realistic and easily reproducible scenario.

Impact:

The user will lose any assets from deposits before the most recent one, as the protocol only refunds the last recorded deposit amount. This would result in direct financial loss, where earlier deposits would be distributed between the winning team.

Proof of Concept

Add this test to the test suite in test/briVault.t.sol.

function testMultipleDepositsOverwritesStakedAssetsAndWillRefundWrongAmountUponCancelParticipation() public {
vm.prank(owner);
briVault.setCountry(countries);
​
uint256 depositAmount = 5e18;
uint256 base = 10000;
uint256 feePerDeposit = (depositAmount * participationFeeBsp) / base;
uint256 depositAmountMinusFee = depositAmount - feePerDeposit;
uint256 user1AmountBeforeDeposits = mockToken.balanceOf(user1);
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 2*depositAmount);
briVault.deposit(depositAmount, user1);
briVault.deposit(depositAmount, user1);
uint256 amountAfterDeposits = mockToken.balanceOf(user1);
briVault.cancelParticipation();
vm.stopPrank();
​
// user1 should be refunded both deposits minus fees but instead is refunded only one of the deposits minus both fees
assert(mockToken.balanceOf(user1) == amountAfterDeposits + depositAmountMinusFee );
assert(mockToken.balanceOf(user1) == user1AmountBeforeDeposits - depositAmount - feePerDeposit);
assert(mockToken.balanceOf(user1) < user1AmountBeforeDeposits - 2 * feePerDeposit);
}

This test demonstrates that the user only receives a refund for their latest deposit rather than the total of all deposits.

Recommended Mitigation

Accumulate deposits instead of overwriting the previous value in stakedAsset.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
...
​
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!