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

deposit() fails for multiple deposits from the same user

crazycelery

stakedAsset in deposit() only records the last stakeAsset made - a user is not able to get all their stakeAssets out with cancelParticipation() if they deposited more than once

Description

  • cancelParticipation() should get the total of a user's stakeAssets out if they deposited multiple times with deposit().

  • stakedAsset in deposit() only records the last stakeAsset amount made.

    • This results in cancelParticipation() only using the last recorded stakeAssets amount for the refundAmount.

    • All previous stakeAsset amounts are ignored.

    • A user who deposited more than once will not be able to get a full refund.

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:

  • A user calls deposits()more than once.

Impact:

  • On calling cancelParticipation() a user who deposited more than once will not be able to get their full refund.

Proof of Concept

Place this in test/briVault.t.sol:

function test_cancelAfterMultipleDeposits () public {
​
vm.startPrank(user1);
​
console.log("user1 original MTK balance");
console.log(mockToken.balanceOf(address(user1)));
​
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
​
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
​
briVault.cancelParticipation();
vm.stopPrank();
​
assertEq(briVault.stakedAsset(user1), 0 ether);
​
console.log("user1 after cancelParticipation() MTK balance");
console.log(mockToken.balanceOf(address(user1)));
}
​

  • user1deposits two 5 ether MTK amounts

Run with

forge test -vvv --match-test test_cancelAfterMultipleDeposits

Sample output

Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_cancelAfterMultipleDeposits() (gas: 194867)
Logs:
user1 original MTK balance
20000000000000000000
user1 after cancelParticipation() MTK balance
14925000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.44ms (617.46µs CPU time)
​
Ran 1 test suite in 11.46ms (3.44ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

  • user1started off with 20 eth, depostied a total of 10 eth but did not get 19.7 eth (20 eth * 0.985) back after the refund.

Recommended Mitigation

In src/briVault.solmake stakedAsset record the total sum of stakeAsset for each receiver.

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

bryanconquer 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!