Unreliable Senior Tranche yield

[kitty-the-kat]

The Senior Tranche is designed to earn a fixed amount of yield. This yield is not always accounted for in the PnL calculations.

Technical Details

PnLFixedRate.sol does not account for seniorProfit in two cases:

1. In distributeLoss() when the loss is greater than or equal to the value of the Junior Tranche and could cause losses for the Senior Tranche
2. In distributeProfit() when the utilisation ratio is greater than utilisationThreshold

Impact

Medium. The PnLFixedRate.sol calculations do not account for seniorProfit in less common edge cases.

Recommendation

The amount seniorProfit should likely be summed into loss[1] or profit[1] respectively. Otherwise, when distributeAssets() is called and fixedRate.lastDistribution is updated without the seniorProfit getting added to either tranche, users who deposited into the Senior Tranche will lose out on the yield they should have received since the previous distribution.

[kitty-the-kat]

disputed: If I understand correctly then the issue is that senior tranche doesnt get any APY under the two listed circumstances above - if this is the case than that is intendent behaviour. The protection is only up to whatever the junior tranche offers, anything beyond that will result in losses for the senior tranche. The loss of APY when falling below the utilizationThreshold is intended to act as a protection to the junior tranche, and encourage senior tranche holders to pull out (as they now get 0% APY), so also intended. If theres anything Im missing, please let me know!

[engn33r]

I can see the points from the comments by @kitty-the-kat, this might be intended behaviour. I will elaborate further to make sure the suggestions are clear. If the explanations below were already understood, I will remove this issue because it may be a non-issue.

The suggestion for distributeLoss() is to make this modification

- loss[1] = _amount - _trancheBalances[0];
+ loss[1] = _amount - _trancheBalances[0] + seniorProfit;

The reason this change may make sense is because the return values of distributeLoss() and distributeProfit() is how the tranche balances are updated. The senior tranche should always receive the fixed rate yield calculated by _calc_rate(). What makes this particular case interesting is that seniorProfit is used in the if statement, so not including it in the following math means the value is considered for the branching logic but then ignore in one of the branches.

The suggestion for distributeProfit() is pointing out that currently when the utilisationThreshold is exceeded, the senior tranche receives no profit. Would it make more sense to pay the senior tranche the profit it is due if the utilization were at the maximum? Perhaps the senior tranche cannot receive the profit when the utilization is greater than the maximum utilisationThreshold, but why not cap the profits to the equivalent profits at 100% of utilisationThreshold? The suggested edit would go into this else statement.
https://github.com/groLabs/GSquared-foundry/blob/f1831bdb353d4a0b3a8937087e1663f73b75e905/src/pnl/PnLFixedRate.sol#L159-L161

With that said, I can understand why the design might choose a zero senior tranche yield when over the utilisationThreshold provides a useful incentive to withdraw. But this should be documented because it breaks the "fixed yield" assumption for the senior tranche and may be considered unexpected behaviour by users.