Root + Impact
Description
When users deposit multiple times the stakedAsset mapping is overwritten (not accumulated) and then when the user cancel his participation the refunded amount is less than deposit.
The Problem:
stakedAsset[receiver] is overwritten with each deposit
_mint() correctly accumulates shares
This creates a mismatch between tracked stake and actual shares
function deposit(uint256 assets, address receiver) public override returns (uint256) {
uint256 stakeAsset = assets - fee;
@> stakedAsset[receiver] = stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
_mint(msg.sender, participantShares);
}
function cancelParticipation () public {
@> 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
Impact: High
Proof of Concept
The Exploit:
User deposits 5 ETH → stakedAsset = 4.925 ETH, shares = 4.925 ETH
User deposits 5 ETH → stakedAsset = 4.925 ETH (overwritten!), shares = 9.85 ETH
User deposits 5 ETH → stakedAsset = 4.925 ETH (overwritten!), shares = 14.775 ETH
User calls cancelParticipation()
User receives back: 4.925 ETH (only last deposit)
User loses: 9.85 ETH (66.67% of total deposit)
Vault keeps: 9.85 ETH as "ghost funds"
function test_MultipleDeposits_CancelTheft() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 20 ether);
briVault.deposit(5 ether, user1);
briVault.deposit(5 ether, user1);
briVault.deposit(5 ether, user1);
uint256 userSharesBeforeCancel = briVault.balanceOf(user1);
uint256 trackedStake = briVault.stakedAsset(user1);
uint256 vaultBalanceBeforeCancel = mockToken.balanceOf(address(briVault));
uint256 userBalanceBeforeCancel = mockToken.balanceOf(user1);
briVault.cancelParticipation();
uint256 userBalanceAfterCancel = mockToken.balanceOf(user1);
uint256 vaultBalanceAfterCancel = mockToken.balanceOf(address(briVault));
uint256 refundAmount = userBalanceAfterCancel - userBalanceBeforeCancel;
vm.stopPrank();
assertEq(refundAmount, trackedStake, "Refund should only be tracked stake");
assertEq(briVault.balanceOf(user1), 0, "All shares should be burned");
assertLt(refundAmount, userSharesBeforeCancel, "Refund less than actual shares");
uint256 stolenFunds = vaultBalanceBeforeCancel - vaultBalanceAfterCancel;
uint256 unreturnedFunds = userSharesBeforeCancel - refundAmount;
assertGt(stolenFunds, 0, "Stolen funds > 0");
assertGt(vaultBalanceAfterCancel, 0, "Vault should have leftover funds");
assertGt(unreturnedFunds, 0, "User should lose funds");
}
Recommended Mitigation
Make stakedAsset mapping to accumulate instead to override the staked assets.
function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0), "Invalid receiver");
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
uint256 fee = _getParticipationFee(assets);
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;
}
