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

briVault::deposit incorrectly handles the stakedAsset value stored

dpedpe

briVault::deposit overwrites the values of briVault::stakedAsset very time the user deposits. Causing multiple deposits results in incorrect value for briVault::stakedAsset.

Description

  • Normally, briVault::stakedAsset should store the amount of shares that the user has deposited.

  • However, briVault:deposit only stores the last deposit by overwriting the value stored in briVault::stakedAsset with the new deposit amount everytime.

// Root cause in the codebase with @> marks to highlight the relevant section
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: High

  • Reason 1: When an user deposit multiple times , this happens.


Impact: Medium

  • Impact 1: The user will not be able to use briVault::cancelParticipationto withdraw effectively and all his remaining asset will be lost since briVault::cancelParticipation will burn all his vault token.


Proof of Concept

This proof of code can be run inside the provided briVault.t.sol and shows that after depositing 6, 4, 5 tokens in 3 deposits, the user can only retrieve 5 token - fee which is the amount of the last deposit.

//briVault.t.sol
function testMultipleDepositThenWithdrww() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
vm.startPrank(user1);
uint256 fee = (5 ether * participationFeeBsp) / 10000;
mockToken.approve(address(briVault), 15 ether);
briVault.deposit(6 ether, user1);
briVault.deposit(4 ether, user1);
briVault.deposit(5 ether, user1);
uint256 balanceBeforeUser1 = mockToken.balanceOf(user1);
briVault.cancelParticipation();
vm.stopPrank();
assertEq(
mockToken.balanceOf(user1),
balanceBeforeUser1 + 5 ether - fee
);
}

Recommended Mitigation

The recommended solution would be to add the amount of the new deposit to briVault::stakedAssetinstead of overwritting it.

// Root cause in the codebase with @> marks to highlight the relevant section
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!