immeas - last repayments are calculated incorrectly for "irregular" loan durations

[sherlock-admin]

immeas

medium

last repayments are calculated incorrectly for "irregular" loan durations

Summary

When taking a loan, a borrower expects that at the end of each payment cycle they should pay paymentCycleAmount. This is not true for loans that are not a multiple of paymentCycle.

Vulnerability Detail

Imagine a loan of 1000 that is taken for 2.5 payment cycles (skip interest to keep calculations simple).

A borrower would expect to pay 400 + 400 + 200

This holds true for the first installment.

But lets look at what happens at the second installment, here's the calculation of what is to pay in V2Calculations.sol:

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol#L93-L101

File: libraries/V2Calculations.sol

 93:        // Cast to int265 to avoid underflow errors (negative means loan duration has passed)
 94:        int256 durationLeftOnLoan = int256(
 95:            uint256(_bid.loanDetails.loanDuration)
 96:        ) -
 97:            (int256(_timestamp) -
 98:                int256(uint256(_bid.loanDetails.acceptedTimestamp)));
 99:        bool isLastPaymentCycle = durationLeftOnLoan <
100:            int256(uint256(_bid.terms.paymentCycle)) || // Check if current payment cycle is within or beyond the last one
101:            owedPrincipal_ + interest_ <= _bid.terms.paymentCycleAmount; // Check if what is left to pay is less than the payment cycle amount

Simplified the first calculation says timeleft = loanDuration - (now - acceptedTimestamp) and then if timeleft < paymentCycle we are within the last payment cycle.

This isn't true for loan durations that aren't multiples of the payment cycles. This code says the last payment cycle is when you are one payment cycle from the end of the loan. Which is not the same as last payment cycle as my example above shows.

PoC:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import { UpgradeableBeacon } from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";

import { AddressUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";

import { TellerV2 } from "../contracts/TellerV2.sol";
import { Payment } from "../contracts/TellerV2Storage.sol";
import { CollateralManager } from "../contracts/CollateralManager.sol";
import { LenderCommitmentForwarder } from "../contracts/LenderCommitmentForwarder.sol";
import { CollateralEscrowV1 } from "../contracts/escrow/CollateralEscrowV1.sol";
import { Collateral, CollateralType } from "../contracts/interfaces/escrow/ICollateralEscrowV1.sol";

import { ReputationManagerMock } from "../contracts/mock/ReputationManagerMock.sol";
import { LenderManagerMock } from "../contracts/mock/LenderManagerMock.sol";
import { MarketRegistryMock } from "../contracts/mock/MarketRegistryMock.sol";

import {TestERC20Token} from "./tokens/TestERC20Token.sol";

import "lib/forge-std/src/Test.sol";

contract LoansTest is Test {
    using AddressUpgradeable for address;

    MarketRegistryMock marketRegistry;

    TellerV2 tellerV2;
    LenderCommitmentForwarder lenderCommitmentForwarder;
    CollateralManager collateralManager;

    TestERC20Token principalToken;

    address alice = address(0x1111);

    uint256 marketId = 0;

    function setUp() public {
        tellerV2 = new TellerV2(address(0));

        marketRegistry = new MarketRegistryMock();

        lenderCommitmentForwarder = new LenderCommitmentForwarder(address(tellerV2),address(marketRegistry));

        collateralManager = new CollateralManager();
        collateralManager.initialize(address(new UpgradeableBeacon(address(new CollateralEscrowV1()))), address(tellerV2));

        address rm = address(new ReputationManagerMock());
        address lm = address(new LenderManagerMock());
        tellerV2.initialize(0, address(marketRegistry), rm, address(lenderCommitmentForwarder), address(collateralManager), lm);

        marketRegistry.setMarketOwner(address(this));
        marketRegistry.setMarketFeeRecipient(address(this));

        tellerV2.setTrustedMarketForwarder(marketId,address(lenderCommitmentForwarder));

        principalToken = new TestERC20Token("Principal Token", "PRIN", 12e18, 18);
    }

    function testLoanInstallmentsCalculatedIncorrectly() public {
        // payment cycle is 1000 in market registry

        uint256 amount = 1000;
        principalToken.transfer(alice,amount);

        vm.startPrank(alice);
        principalToken.approve(address(tellerV2),2*amount);
        uint256 bidId = tellerV2.submitBid(
            address(principalToken),
            marketId,
            amount,
            2500, // 2.5 payment cycles
            0, // 0 interest to make calculations easier
            "",
            alice
        );
        tellerV2.lenderAcceptBid(bidId);
        vm.stopPrank();

        // jump to first payment cycle end
        vm.warp(block.timestamp + 1000);
        Payment memory p = tellerV2.calculateAmountDue(bidId);
        assertEq(400,p.principal);

        // borrower pays on time
        vm.prank(alice);
        tellerV2.repayLoanMinimum(bidId);

        // jump to second payment cycle
        vm.warp(block.timestamp + 1000);
        p = tellerV2.calculateAmountDue(bidId);

        // should be 400 but is full loan
        assertEq(600,p.principal);
    }
}

The details of this finding are out of scope but since it makes TellerV2, in scope, behave unexpectedly I believe this finding to be in scope.

Impact

A borrower taking a loan might not be able to pay the last payment cycle and be liquidated. At the worst possible time since they've paid the whole loan on schedule up to the last installment. The liquidator just need to pay the last installment to take the whole collateral.

This requires the loan to not be a multiple of the payment cycle which might sound odd. But since a year is 365 days and a common payment cycle is 30 days I imagine there can be quite a lot of loans that after 360 days will end up in this issue.

There is also nothing stopping an unknowing borrower from placing a bid or accepting a commitment with an odd duration.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol#L94-L101

Tool used

Manual Review

Recommendation

First I thought that you could remove the lastPaymentCycle calculation all together. I tried that and then also tested what happened with "irregular" loans with interest.

Then I found this in the EMI calculation:

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/libraries/NumbersLib.sol#L123

File: libraries/NumbersLib.sol

132:        uint256 n = Math.ceilDiv(loanDuration, cycleDuration);

EMI, which is designed for mortgages, assumes the payments is a discrete number of the same amortization essentially. I.e they don't allow "partial" periods at the end, because that doesn't make sense for a mortgage.

In Teller this is allowed which causes some issues with the EMI calculation since the above row will always round up to a full number of payment periods. If you also count interest, which triggers the EMI calculation: The lender, in an "irregular" loan duration, would get less per installment up to the last one which would be bigger. The funds would all be paid with the correct interest in the end just not in the expected amounts.

My recommendation now is:

either

• don't allow loan durations that aren't a multiple of the period, at least warn about it UI-wise, no one will lose any money but the installments might be split in unexpected amounts.
• Do away with EMI all together as DeFi loans aren't the same as mortgages. The defaulting/liquidation logic only cares about time since last payment.
• Do more math to make EMI work with irregular loan durations. This nerd sniped me:

More math:

From the link in the comment, https://en.wikipedia.org/wiki/Equated_monthly_installment you can follow one of the links in that wiki page to a derivation of the formula: http://rmathew.com/2006/calculating-emis.html

In the middle we have an equation which describes the owed amount at a time $P_n$:

$$P_n=Pt^n-E\frac{(t^n-1)}{t-n}$$ where $t=1+r$ and $r$ is the monthly interest rate ($apy*C/year$).

Now, from here, we want to calculate the loan at a time $P_{n + \Delta}$:

$$P{n + \Delta}=Pt^nt\Delta-E\frac{t^n-1}{t-1}t_\Delta-kE$$

Where $k$ is $c/C$ i.e. the ratio of partial cycle compared to a full cycle.

Same with $t\Delta$ which is $1+r\Delta$, ($r_\Delta$ is also equal to $kr$, ratio of partial cycle rate to full cycle rate, which we'll use later).

Reorganize to get $E$ from above:

$$ E = P r \frac{t^nt\Delta}{t\Delta \frac{t^n-1}{t-1} + k} $$

Now substitute in $1+r$ in place of $t$ and $1+r\Delta$ instead of $t\Delta$ and multiply both numerator and denominator with $r$:

$$ E = P \frac{r (1+r)^n(1+r\Delta)}{(1+r\Delta)((1+r)^n - 1) + kr} $$

and $kr = r_\Delta$ gives us:

$$ E = P r (1+r)^n \frac{(1+r\Delta)}{(1+r\Delta)((1+r)^n - 1) + r_\Delta} $$

To check that this is correct, $r\Delta = 0$ (no extra cycle added) should give us the regular EMI equation. Which we can see is true for the above. And $r\Delta = r$ (a full extra cycle added) should give us the EMI equation but with $n+1$ which we can also see it does.

Here are the code changes to use this, together with changes to V2Calculations.sol to calculate the last period correctly:

diff --git a/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol b/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol
index 1cce8da..1ad5bcf 100644
--- a/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol
+++ b/teller-protocol-v2/packages/contracts/contracts/libraries/V2Calculations.sol
@@ -90,30 +90,15 @@ library V2Calculations {
         uint256 owedTime = _timestamp - uint256(_lastRepaidTimestamp);
         interest_ = (interestOwedInAYear * owedTime) / daysInYear;

-        // Cast to int265 to avoid underflow errors (negative means loan duration has passed)
-        int256 durationLeftOnLoan = int256(
-            uint256(_bid.loanDetails.loanDuration)
-        ) -
-            (int256(_timestamp) -
-                int256(uint256(_bid.loanDetails.acceptedTimestamp)));
-        bool isLastPaymentCycle = durationLeftOnLoan <
-            int256(uint256(_bid.terms.paymentCycle)) || // Check if current payment cycle is within or beyond the last one
-            owedPrincipal_ + interest_ <= _bid.terms.paymentCycleAmount; // Check if what is left to pay is less than the payment cycle amount
-
         if (_bid.paymentType == PaymentType.Bullet) {
-            if (isLastPaymentCycle) {
-                duePrincipal_ = owedPrincipal_;
-            }
+            duePrincipal_ = owedPrincipal_;
         } else {
             // Default to PaymentType.EMI
             // Max payable amount in a cycle
             // NOTE: the last cycle could have less than the calculated payment amount
-            uint256 maxCycleOwed = isLastPaymentCycle
-                ? owedPrincipal_ + interest_
-                : _bid.terms.paymentCycleAmount;

             // Calculate accrued amount due since last repayment
-            uint256 owedAmount = (maxCycleOwed * owedTime) /
+            uint256 owedAmount = (_bid.terms.paymentCycleAmount * owedTime) /
                 _bid.terms.paymentCycle;
             duePrincipal_ = Math.min(owedAmount - interest_, owedPrincipal_);
         }

And then NumbersLib.sol:

diff --git a/teller-protocol-v2/packages/contracts/contracts/libraries/NumbersLib.sol b/teller-protocol-v2/packages/contracts/contracts/libraries/NumbersLib.sol
index f34dd9c..8ca48bc 100644
--- a/teller-protocol-v2/packages/contracts/contracts/libraries/NumbersLib.sol
+++ b/teller-protocol-v2/packages/contracts/contracts/libraries/NumbersLib.sol
@@ -120,7 +120,8 @@ library NumbersLib {
                 );

         // Number of payment cycles for the duration of the loan
-        uint256 n = Math.ceilDiv(loanDuration, cycleDuration);
+        uint256 n = loanDuration/ cycleDuration;
+        uint256 rest = loanDuration%cycleDuration;

         uint256 one = WadRayMath.wad();
         uint256 r = WadRayMath.pctToWad(apr).wadMul(cycleDuration).wadDiv(
@@ -128,8 +129,16 @@ library NumbersLib {
         );
         uint256 exp = (one + r).wadPow(n);
         uint256 numerator = principal.wadMul(r).wadMul(exp);
-        uint256 denominator = exp - one;

-        return numerator.wadDiv(denominator);
+        if(rest==0) {
+            // duration is multiple of cycle
+            uint256 denominator = exp - one;
+            return numerator.wadDiv(denominator);
+        }
+        // duration is an uneven cycle
+        uint256 rDelta = WadRayMath.pctToWad(apr).wadMul(rest).wadDiv(daysInYear);
+        uint256 n1 = numerator.wadMul(one + rDelta);
+        uint256 denom = ((one + rDelta).wadMul(exp - one)) + rDelta;
+        return n1.wadDiv(denom);
     }
 }

[ethereumdegen]

It seems that in that example of a loan with 3 cycles, 400 then 400 and then 200, if the borrower is more than halfway through the second installment (second 400) , they would be considered to be in the 'lastPaymentCycle' incorrectly and would 'owe' 600 instead of 400. We will investigate this more for a fix.

[cducrest]

Escalate for 10 USDC

Issue is from out of scope contract. Both vulnerability and fix are in out-of-scope contract. Awarding issues for contracts not in scope incentivize poor scoping from protocol in the future and lead to more work for auditors with less pay (protocols pay for smaller scope and receive audits for bigger scopes).

Multiple incorrect behaviours could be spotted in MarketRegistry or ReputationManager as well but were not reported because they are out of scope.

[sherlock-admin]

Escalate for 10 USDC

Issue is from out of scope contract. Both vulnerability and fix are in out-of-scope contract. Awarding issues for contracts not in scope incentivize poor scoping from protocol in the future and lead to more work for auditors with less pay (protocols pay for smaller scope and receive audits for bigger scopes).

Multiple incorrect behaviours could be spotted in MarketRegistry or ReputationManager as well but were not reported because they are out of scope.

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.

[ethereumdegen]

Github PR: Issue 328 - new repayment logic for v2 calculations

[hrishibhat]

Escalation rejected

Valid medium. The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

[sherlock-admin]

Escalation rejected

Valid medium. The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[IAm0x52]

Fix looks good. Final repayment period is now calculated via modulo which allows it to correctly detect the final payment cycle for loans of irregular duration