_previewInterest function does not check that accrualFactor is greater than 0 before using it to update borrowIndex. This is highly vulnerable

Summary

the _previewInterest function does not check that accrualFactor is greater than 0 before using it to update borrowIndex. This could potentially cause major vulnerabilities

Vulnerability Detail

The key lines are:

   uint256 accrualFactor = rateModel.getAccrualFactor({
     utilization: (1e18 * oldBorrows) / oldInventory,
     dt: block.timestamp - cache.lastAccrualTime
   });

   cache.borrowIndex = (cache.borrowIndex * accrualFactor) / ONE;

If accrualFactor is 0, then cache.borrowIndex will be 0 after this update. This is problematic because borrowIndex tracks the total interest growth on borrows over time. Setting it to 0 would incorrectly reset all interest accrued. A borrowIndex of 0 could allow borrowers to repay loans that no longer exist, creating a loss for the protocol. Or it could prevent lenders from withdrawing the full value of their deposits.

To buttress more point:

Specifically, if rateModel.getAccrualFactor were to return 0, it would cause borrowIndex to be set to 0 in the line:

   cache.borrowIndex = (cache.borrowIndex * accrualFactor) / ONE;

Setting borrowIndex to 0 could have negative impacts: • It would effectively wipe out all accumulated interest for borrowers, making their borrows worth nothing. This could benefit borrowers at the expense of lenders. • It could potentially enable borrowers to withdraw more assets than they deposited if their borrow balance was accumulated at a higher borrowIndex.

Impact

The vulnerability of not checking that accrualFactor is greater than 0 is that it could allow the borrowIndex to be incorrectly set to 0. This would have a high severity impact: • It would wipe out all accumulated interest for borrowers, making their borrows worth nothing. This would unfairly benefit borrowers at the expense of lenders. • It could enable borrowers to withdraw more assets than they deposited if their borrow balance was accumulated at a higher borrowIndex. This would incorrectly drain assets from the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Ledger.sol#L351-L356

Tool used

Manual Review

Recommendation

the code should add:

   require(accrualFactor > 0, "Invalid accrual factor");

before updating borrowIndex. This will revert the transaction if accrualFactor is invalid, preventing the borrowIndex corruption.

sherlock-admin2 commented 1 year ago

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

panprog commented:

invalid, because accrualFactor is returned by library, which is calculated as 1 + rateModel return value, so accrualFactor can not be less than 1

MohammedRizwan commented:

invalid issue

haydenshively commented 1 year ago

The comments from panprog and MohammedRizwan are correct.

SafeRateLib guarantees that the accrualFactor is non-zero (and in fact, >= 1e12). We have tests for this in RateModel.t.sol.

sherlock-audit / 2023-10-aloe-judging

feelereth - _previewInterest function does not check that accrualFactor is greater than 0 before using it to update borrowIndex. This is highly vulnerable #107