tsvetanovv - A user may lose funds if he uses the `claim()` function

[sherlock-admin]

tsvetanovv

high

A user may lose funds if he uses the claim() function

Summary

In BalancedVault.sol we have claim() function:

    function claim(address account) external {
        (EpochContext memory context, ) = _settle(account);

        UFixed18 unclaimedAmount = _unclaimed[account];
        UFixed18 unclaimedTotal = _totalUnclaimed;
        _unclaimed[account] = UFixed18Lib.ZERO;
        _totalUnclaimed = unclaimedTotal.sub(unclaimedAmount);
        emit Claim(msg.sender, account, unclaimedAmount);

        // pro-rate if vault has less collateral than unclaimed
        UFixed18 claimAmount = unclaimedAmount;
        UFixed18 totalCollateral = _assets();
        if (totalCollateral.lt(unclaimedTotal)) claimAmount = claimAmount.muldiv(totalCollateral, unclaimedTotal);

        _rebalance(context, claimAmount);

        asset.push(account, claimAmount);
    }

This function claim all claimable assets for a specific account and sends those assets to the account. But under certain circumstances, users may lose funds.

Vulnerability Detail

First, the function retrieves the unclaimed amount for the specified account and the total unclaimed amount.

UFixed18 unclaimedAmount = _unclaimed[account];

After this sets the _unclaimed[account] to zero, meaning that the account is now considered to have claimed its assets, even if the actual transfer hasn't happened yet, and emit events.

_unclaimed[account] = UFixed18Lib.ZERO;

emit Claim(msg.sender, account, unclaimedAmount);

After this checks if there's enough collateral in the vault to cover all unclaimed assets. If there isn't, it reduces the amount the user can claim proportionally based on the total available collateral and total unclaimed assets.

if (totalCollateral.lt(unclaimedTotal)) claimAmount = claimAmount.muldiv(totalCollateral, unclaimedTotal);

The problem is, if the vault doesn't have enough collateral to cover all the unclaimed assets, then the amount each user can claim is pro-rated. This results in the user potentially losing some funds since the unclaimed amount for their account is set to zero after the claim.

Impact

If totalCollateral is less than unclaimedTotal, users only receive a pro-rated amount, with no possibility of recovering the remaining amount, even if someone rescues the vault by adding enough collateral to cover all users. This leads to a loss of user funds.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L207-L228

Tool used

Manual Review

Recommendation

You need to track the "lost" amount when funds are pro-rated. If the vault becomes solvent again, the users could then claim their lost amounts.