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

Loss of funds for users when trying to cancel their participation

iamgeorgi

A logical error in briVault::deposit() can cause incorrect accounting of a user’s total deposited assets when multiple deposits are made. This will result in users receiving less than their entitled balance - specifically, only the amount from the most recent deposit minus fees—when calling briVault::cancelParticipation, potentially leading to permanent loss of previously deposited funds.

Description

  • Normal behavior - If a user makes multiple deposits, the total amount of deposited assets must be accurately tracked in the stakedAsset mapping. If the user later on decided to cancel participation, they should recover the full sum of their deposits minus the applicable fees.

  • Issue - Depositing multiple times is not being handled correctly which leads to loss of funds if a user wants to quit and cancels their participation.

​
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;
​
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:

  • Every user that decided to deposit more than once and after that for some reason wants to quit and get his assets back.

Impact:

  • Users can't recover fully their funds and suffer a loss


Proof of Concept

  1. User1 deposits some amount of tokens - 5e18

  2. User1 deposits one more time - 4e18

  3. User1 decided he wants to cancel participation

  4. His shares are burned, and he receives back 4e18 - depositFee, resulting in a loss relative to his original deposit amount.

function test_lossOfFundsWhenQuitting() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
uint256 shares1 = briVault.balanceOf(user1);
mockToken.approve(address(briVault), 4 ether);
briVault.deposit(4 ether, user1);
shares1 = briVault.balanceOf(user1);
console.log("user's assets => ",briVault.stakedAsset(user1));
briVault.cancelParticipation();
assertEq(0, briVault.balanceOf(user1));
assertNotEq(0, mockToken.balanceOf(address(briVault)));
}
// after second deposit the console log for the stakedAsset mapping
// clearly shows the amount , should be (5 ether - fee) + (4 ether - fee) but instead is
// 4 ether - fee.
// console::log("user's assets => ", 3940000000000000000
​

Recommended Mitigation

In the briVault.deposit() consider updating the stakedAsset mapping correctly

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!