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

`BriVault::deposit`, function doesn't increment user stakedAsset mappings after multiple deposits, resulting in an accounting error, user might not get full funds back when canceling participaty

hcrlen

BriVault::deposit, function doesn't increment user stakedAsset mappings after multiple deposits, resulting in an accounting error

Description

  • A user stakedAsset is a mapping that tracks user , this should increment user asset after depositing

  • but the function doesnt increment that mapping

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

Risk: High, user actual asset inside the vault become mismatched with the accounting

Likelihood:

  • High, user can add deposit anytime, as long as it is before the event starting time

Impact:

  • User can lost their funds when they want to cancle participation

Proof of Concept

add this to your test suite:

the correct behaviour was that user stakedAsset should increment, but inside this test suite the mapping doesnt increment, it just rewrites the data

function test_multipleDepositBadAccounting() public {
vm.prank(owner);
briVault.setCountry(countries);
uint256 deposit = 0.00023 ether;
//user 1 deposit
vm.startPrank(user1);
mockToken.approve(address(briVault), deposit * 2);
briVault.deposit(deposit, user1);
uint256 user1stakedAsset = briVault.stakedAsset(user1);
briVault.deposit(deposit, user1);
vm.stopPrank();
uint256 user1stakedAssetAfter = briVault.stakedAsset(user1);
assertEq(user1stakedAsset, user1stakedAssetAfter);
}

Recommended Mitigation

by adding a + sign we can mitigate this issue.


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!