Tranche deposit/withdrawal denial of service

[kitty-the-kat]

It is possible for the utilisation ratio of the two tranches to be greater than utilisationThreshold. If this scenario occurs and a user wishes to withdraw or deposit to a tranche with an amount that does not bring the tranches back under the utilisationThreshold, this can result in a denial of service for most users where a single whale-sized deposit or withdrawal must happen in order to bring the utilisation ratio below utilisationThreshold.

Technical Details

The current code is written in a way that assumes the utilisation ratio remains below the utilisationThreshold unless a deposit or withdrawal will move it above the threshold. In fact, the utilisation ratio can achieve a value greater than utilisationThreshold in a couple of ways:

1. The owner can set utilisationThreshold to any value with setUtilizationThreshold(). If the threshold is set to a value less than the current utilisation ratio, it can break assumptions about how the protocol should work and prevent tranche deposits or withdrawals.
2. Losses for the Junior Tranche may result in a scenario where the utilisation ratio exceeds the utilisationThreshold. Note that the Junior tranche can experience a loss even when distributeProfit() is called because the yield paid to the Senior Tranche depositors may exceed the profit earned.

If the utilisation ratio exceeds the utilisationThreshold, a GTranche deposit() or withdraw() that attempts to move the utilisation ratio in the correct direction may revert if it does not bring the ratio below utilisationThreshold. This behaviour breaks an assumption in the protocol documentation that such actions should always be allowed.

Impact

Medium. The tranche design requires the Junior Tranche to always allow deposits and Senior Tranche should always allow withdrawals, but these requirements can be broken in edge cases.

Recommendation

Allow GTranche deposit() and withdrawal() to happen even if trancheUtilization > utilisationThreshold. If trancheUtilization > utilisationThreshold, then the trancheUtilization should be reduced as a result of the deposit or withdrawal action when compared to the value of trancheUtilization before the deposit or withdrawal so that the trancheUtilization is changed in a directionally correct manner.

[kitty-the-kat]

The deposit function only checks the utilization when the deposit is for senior tranche, and withdrawals does the same for junior tranche, so Im not sure I fully understand what the problem reported here is? is it that utilization threshold can be broken and that stop deposits from senior side and withdrawals from junior - then this is the expected behaviour. Or is it that the owner can change the utilization ratio and lock in the Junior tranche?

[engn33r]

@kitty-the-kat the following is from the technical documentation

9. Deposits (given reasonable circumstances)
  9.1 The junior tranche should always be able to deposit
  9.2 Senior tranche should not be able to deposit if the utilization ratio is broken
10. Withdrawals (given reasonable circumstances)
  10.1 The junior tranche should not be able to withdraw if the utilization ratio is broken
  10.2 Senior tranche should always be able to withdraw

The Impact section describes that the risk here is based around the breaking of these assumptions, specifically 9.1 and 10.1. Perhaps exceeding the utilisationThreshold is not considered "reasonable circumstances", but there are two different ways that can easily cause this scenario.

Additionally, a temporary denial of service is normally considered a medium impact issue. In this case there is a reason that the protocol is preventing senior tranche withdrawals, but the key point is that the documentation by and large indicates that senior tranche funds can always be withdrawn, which is inaccurate.

At a minimum, the technical documentation and user-facing documentation should be updated to reflect the behaviour in this edge case.