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

Incorrect logic for refund amount in briVault::cancelParticipation

dpedpe

Using briVault::stakedAsset to determine the amount to refund only refunds the amount that the user has deposited in the last deposit.

Description

  • The briVault::cancelParticipation should refund the total amount that the user has deposited. However, it uses briVault::stakedAsset which only records the amount of the last deposit and not the full deposited amount.

// Root cause in the codebase with @> marks to highlight the relevant section
function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
//@audit what if the user deposited more than once, they will lose their previous deposits.
//@audit the user could
@> 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: High

  • Reason 1: When a user who deposited multiple times decides to exit the event using briVault::cancelParticipation, this happens.

Impact: High/Medium

  • Impact 1: When a user who deposited multiple times wants to withdraw, he will only get the refund for his last deposit. He will have to call manually the ERC4626:withdraw to withdraw his money. Which might not be possible later on since being able to callERC4626:withdraw directly is a known vulnerability that is going to be fixed.


Proof of Concept

Here is a test that can be run in the provided briVault.t.sol . It shows that when the user deposits twice, first time 5 tokens and second time 7 tokens, calling briVault::cancelParticipation only refunds the amount of the second deposit - fee of second deposit.

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

Recommended Mitigation

It is recommended to use the balance of the user which tracks the total shares deposited instead of briVault::stakedAsset which only tracks the last deposit. The following solution does not take into account other vulnerabilities in the function.

// Root cause in the codebase with @> marks to highlight the relevant section
function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
//@audit what if the user deposited more than once, they will lose their previous deposits.
//@audit the user could
- uint256 refundAmount = stakedAsset[msg.sender];
+ uint256 refundAmount = balanceOf(msg.sender);
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
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!