ak1 - BalancedVault.sol#L211 : claim reset the `_unclaimed[account]` when there is less amount of fund.

[sherlock-admin]

ak1

medium

BalancedVault.sol#L211 : claim reset the _unclaimed[account] when there is less amount of fund.

Summary

BalancedVault.sol#L211 : claim computes the pro rata based shared when if (totalCollateral.lt(unclaimedTotal))

It does not check whether it claims the unclaimedAmount of account fully or not.

When there are lesser amount of collateral, the calculated claim value would be way lesser.

Vulnerability Detail

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

Impact

Loss of claim amount to user.

Code Snippet

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

Tool used

Manual Review

Recommendation

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

    if( claimAmount  < unclaimedAmount ) ---------------------------------->>>> add
        _unclaimed[account] = unclaimedAmount -claimAmount  ; ------------->> add

    _rebalance(context, claimAmount);

    asset.push(account, claimAmount);
}

[aktech297]

Escalate for 10 USDC.

First of all, thanks a lot for the lead judge who has done wonderful job in explaining the reason for non-reward. This will help me to rectify my mistakes.

Lets come to escalation now,

Lead judge comment is See issue 002 of Veridise report. By design. sending pro rata when funds are not enough

I understood the pro-rata based transfer of funds. but that should not completely reset the user unclaimed balance.

The previous report mentions transfer of pro-rata based claiming. but that does not say like resetting full claim map.

_unclaimed[account] = UFixed18Lib.ZERO;

Lets consider some of the following situations which leads me to take this as valid issue.

1. When there is no fund available. User will not gain anything, but their unclaimed value is reset to zero.
2. When user is eligible to claim some 1000 units, but due to low funds availability, they may get very few, it can be simply 1 units.

imo, this should not happen. Because users are affected very much with considerable loss of claimable value.

Instead of fully resetting the value to zero, the code could send the available pro-rata shares and then that transferred value could be used to update the unclaimed map. like what was the solution in mentioned in this submission.

[sherlock-admin]

Escalate for 10 USDC.

First of all, thanks a lot for the lead judge who has done wonderful job in explaining the reason for non-reward. This will help me to rectify my mistakes.

Lets come to escalation now,

Lead judge comment is See issue 002 of Veridise report. By design. sending pro rata when funds are not enough

I understood the pro-rata based transfer of funds. but that should not completely reset the user unclaimed balance.

The previous report mentions transfer of pro-rata based claiming. but that does not say like resetting full claim map.

_unclaimed[account] = UFixed18Lib.ZERO;

Lets consider some of the following situations which leads me to take this as valid issue.

1. When there is no fund available. User will not gain anything, but their unclaimed value is reset to zero.
2. When user is eligible to claim some 1000 units, but due to low funds availability, they may get very few, it can be simply 1 units.

imo, this should not happen. Because users are affected very much with considerable loss of claimable value.

Instead of fully resetting the value to zero, the code could send the available pro-rata shares and then that transferred value could be used to update the unclaimed map. like what was the solution in mentioned in this submission.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[KenzoAgada]

Thanks for the kind words ak 🙂

Coming to the escalation, I still don't think that the issue is valid. It does not add anything which is not already mentioned in the abovementioned VAR002 in Veridise report. Veridise did also mention the zeroing out of _unclaimed.

Also note that the solution suggested here would just allow the user to call claim again and receive his missing funds. So this will become a FCFS race, instead of the pro-rata (proportional) approach which Perennial mentioned it decided to use.

[aktech297]

Thanks for the feedback..

As a user, I never wanted to loose my funds. to me, it is also a form of loss of funds which would be a cause of concern.

Anyway its up to design choice of the protocol team.

Thanks!

[jacksanford1]

Ok, leaving as invalid

[jacksanford1]

Result: Invalid Unique Kenzo's last comment is the best reasoning for not upgrading the issue to valid.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• aktech297: rejected