sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

roguereddwarf - Borrower.sol: Health check uses stale liabilities #51

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

roguereddwarf

medium

Borrower.sol: Health check uses stale liabilities

Summary

The Borrower uses the Borrower._getLiabilities function to query its current liabilities. The liabilities are then used in the BalanceSheet.isHealthy function to determine whether the Borrower is healthy.

The problem is that the Borrower._getLiabilities function returns stale liabilities without accruing interest.

When interest has not been accrued for a long time, it's possible that there is a significant difference between the liabilities without interest accrued and liabilities with interest accrued.

As a result, the Borrower might be able to hold significantly more debt compared to its assets than it should be able to.

The risk is that due to market volatility, the protocol may end up with bad debt.

The loan to value (LTV) ratio depends on the implied volatility to essentially ensure that there can be no bad debt. Not keeping the LTV makes it more likely that the margin isn't big enough to cover sudden market movements.

This is also a problem for liquidations as it means that liquidations can occur that do not repay all liabilities but still take up all the ante. Thereby the Borrower ends up in a state with liabilities but no ante such that there is no incentive to liquidate it again should it become necessary. This means bad debt can accrue.

Vulnerability Detail

The Borrower uses the _getLiabilities function in multiple places. One of them is the Borrower.modify function.

The Borrower.modify function calls BalanceSheet.isHealthy to check that the Borrower cannot exceed the current maximum loan to value (LTV) ratio.

Say the LTV is 90%, meaning for $90 in loan, there must be at least $100 in collateral.

If the interest is not accrued and the actual loan is now $91, then the real loan to value ratio has increased to 91%. This means that the Borrower should have to pay back some of the loan to pass the health check.

The maximum duration for which interest is not accrued is limited to 1 week which limits the amount of interest that is not accounted for.

For Borrowers that have taken out large loans though this is not negligible. For a loan of $10 million and a yearly interest rate of 3.7% (~0.07% per week), this would be a difference of $7000 in interest.

As described in the Summary, using stale liabilities also impacts liquidations and can lead to bad debt as there is no incentive to liquidate Borrowers if all the ante is used up.

Impact

The Borrower can have a higher loan to value ratio than it should be able to due to calculating its liabilities without accruing interest.

LTV is based on current market conditions and if the real LTV is higher (i.e. the actual debt is higher) the collateral might not be sufficient to cover the loan in cases of quick market movements.

With regards to liquidations, using stale liabilities can lead to bad debt.

Code Snippet

https://github.com/aloelabs/aloe-ii/blob/c71e7b0cfdec830b1f054486dfe9d58ce407c7a4/core/src/Borrower.sol#L299-L327

https://github.com/aloelabs/aloe-ii/blob/c71e7b0cfdec830b1f054486dfe9d58ce407c7a4/core/src/libraries/BalanceSheet.sol#L48-L80

Tool used

Manual Review

Recommendation

The recommendation is to change the Borrower._getLiabilities function to call LENDER.borrowBalance instead of LENDER.borrowBalanceStored:

function _getLiabilities() private view returns (uint256 amount0, uint256 amount1) {
    amount0 = LENDER0.borrowBalance(address(this));
    amount1 = LENDER1.borrowBalance(address(this));
}

Duplicate of #41

sherlock-admin2 commented 1 year ago

1 comment(s) were left on this issue during the judging contest.

MohammedRizwan commented:

valid issue