last repayments are calculated incorrectly for "irregular" loan durations
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`.\\nImagine a loan of `1000` that is taken for 2.5 payment cycles (skip interest to keep calculations simple).\\nA borrower would expect to pay `400` + `400` + `200`\\nThis holds true for the first installment.\\nBut lets look at what happens at the second installment, here's the calculation of what is to pay in V2Calculations.sol:\\n```\\nFile: libraries/V2Calculations.sol\\n\\n 93:        // Cast to int265 to avoid underflow errors (negative means loan duration has passed)\\n 94:        int256 durationLeftOnLoan = int256(\\n 95:            uint256(_bid.loanDetails.loanDuration)\\n 96:        ) -\\n 97:            (int256(_timestamp) -\\n 98:                int256(uint256(_bid.loanDetails.acceptedTimestamp)));\\n 99:        bool isLastPaymentCycle = durationLeftOnLoan <\\n           int256(uint256(_bid.terms.paymentCycle)) || // Check if current payment cycle is within or beyond the last one\\n           owedPrincipal_ + interest_ <= _bid.terms.paymentCycleAmount; // Check if what is left to pay is less than the payment cycle amount\\n```\\n\\nSimplified the first calculation says `timeleft = loanDuration - (now - acceptedTimestamp)` and then if `timeleft < paymentCycle` we are within the last payment cycle.\\nThis 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.\\nPoC:\\n```\\n// SPDX-License-Identifier: MIT\\npragma solidity ^0.8.0;\\n\\nimport { UpgradeableBeacon } from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";\\n\\nimport { AddressUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";\\n\\nimport { TellerV2 } from "../contracts/TellerV2.sol";\\nimport { Payment } from "../contracts/TellerV2Storage.sol";\\nimport { CollateralManager } from "../contracts/CollateralManager.sol";\\nimport { LenderCommitmentForwarder } from "../contracts/LenderCommitmentForwarder.sol";\\nimport { CollateralEscrowV1 } from "../contracts/escrow/CollateralEscrowV1.sol";\\nimport { Collateral, CollateralType } from "../contracts/interfaces/escrow/ICollateralEscrowV1.sol";\\n\\nimport { ReputationManagerMock } from "../contracts/mock/ReputationManagerMock.sol";\\nimport { LenderManagerMock } from "../contracts/mock/LenderManagerMock.sol";\\nimport { MarketRegistryMock } from "../contracts/mock/MarketRegistryMock.sol";\\n\\nimport {TestERC20Token} from "./tokens/TestERC20Token.sol";\\n\\nimport "lib/forge-std/src/Test.sol";\\n\\ncontract LoansTest is Test {\\n    using AddressUpgradeable for address;\\n\\n    MarketRegistryMock marketRegistry;\\n\\n    TellerV2 tellerV2;\\n    LenderCommitmentForwarder lenderCommitmentForwarder;\\n    CollateralManager collateralManager;\\n    \\n    TestERC20Token principalToken;\\n\\n    address alice = address(0x1111);\\n\\n    uint256 marketId = 0;\\n\\n    function setUp() public {\\n        tellerV2 = new TellerV2(address(0));\\n\\n        marketRegistry = new MarketRegistryMock();\\n\\n        lenderCommitmentForwarder = new LenderCommitmentForwarder(address(tellerV2),address(marketRegistry));\\n        \\n        collateralManager = new CollateralManager();\\n        collateralManager.initialize(address(new UpgradeableBeacon(address(new CollateralEscrowV1()))), address(tellerV2));\\n\\n        address rm = address(new ReputationManagerMock());\\n        address lm = address(new LenderManagerMock());\\n        tellerV2.initialize(0, address(marketRegistry), rm, address(lenderCommitmentForwarder), address(collateralManager), lm);\\n\\n        marketRegistry.setMarketOwner(address(this));\\n        marketRegistry.setMarketFeeRecipient(address(this));\\n\\n        tellerV2.setTrustedMarketForwarder(marketId,address(lenderCommitmentForwarder));\\n\\n        principalToken = new TestERC20Token("Principal Token", "PRIN", 12e18, 18);\\n    }\\n\\n\\n    function testLoanInstallmentsCalculatedIncorrectly() public {\\n        // payment cycle is 1000 in market registry\\n        \\n        uint256 amount = 1000;\\n        principalToken.transfer(alice,amount);\\n     \\n        vm.startPrank(alice);\\n        principalToken.approve(address(tellerV2),2*amount);\\n        uint256 bidId = tellerV2.submitBid(\\n            address(principalToken),\\n            marketId,\\n            amount,\\n            2500, // 2.5 payment cycles\\n            0, // 0 interest to make calculations easier\\n            "",\\n            alice\\n        );\\n        tellerV2.lenderAcceptBid(bidId);\\n        vm.stopPrank();\\n\\n        // jump to first payment cycle end\\n        vm.warp(block.timestamp + 1000);\\n        Payment memory p = tellerV2.calculateAmountDue(bidId);\\n        assertEq(400,p.principal);\\n\\n        // borrower pays on time\\n        vm.prank(alice);\\n        tellerV2.repayLoanMinimum(bidId);\\n\\n        // jump to second payment cycle\\n        vm.warp(block.timestamp + 1000);\\n        p = tellerV2.calculateAmountDue(bidId);\\n\\n        // should be 400 but is full loan\\n        assertEq(600,p.principal);\\n    }\\n}\\n```\\n\\nThe 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.
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.\\nThen I found this in the EMI calculation:\\n```\\nFile: libraries/NumbersLib.sol\\n\\n       uint256 n = Math.ceilDiv(loanDuration, cycleDuration);\\n```\\n\\nEMI, 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.\\nIn 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.\\nMy recommendation now is:\\neither\\ndon'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.\\nDo 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.\\nDo more math to make EMI work with irregular loan durations. This nerd sniped me:\\nMore math:\\nIn the middle we have an equation which describes the owed amount at a time $P_n$:\\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$).\\nNow, from here, we want to calculate the loan at a time $P_{n + \\Delta}$:\\n$$P_{n + \\Delta}=Pt^nt_\\Delta-E\\frac{t^n-1}{t-1}t_\\Delta-kE$$\\nWhere $k$ is $c/C$ i.e. the ratio of partial cycle compared to a full cycle.\\nSame 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).\\nReorganize to get $E$ from above:\\n$$ E = P r \\frac{t^nt_\\Delta}{t_\\Delta \\frac{t^n-1}{t-1} + k} $$\\nNow substitute in $1+r$ in place of $t$ and $1+r_\\Delta$ instead of $t_\\Delta$ and multiply both numerator and denominator with $r$:\\n$$ E = P \\frac{r (1+r)^n(1+r_\\Delta)}{(1+r_\\Delta)((1+r)^n - 1) + kr} $$\\nand $kr = r_\\Delta$ gives us:\\n$$ E = P r (1+r)^n \\frac{(1+r_\\Delta)}{(1+r_\\Delta)((1+r)^n - 1) + r_\\Delta} $$\\nTo 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.\\nHere are the code changes to use this, together with changes to `V2Calculations.sol` to calculate the last period correctly:\\n```\\ndiff // Remove the line below\\n// Remove the line below\\ngit a/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/V2Calculations.sol b/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/V2Calculations.sol\\nindex 1cce8da..1ad5bcf 100644\\n// Remove the line below\\n// Remove the line below\\n// Remove the line below\\n a/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/V2Calculations.sol\\n// Add the line below\\n// Add the line below\\n// Add the line below\\n b/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/V2Calculations.sol\\n@@ // Remove the line below\\n90,30 // Add the line below\\n90,15 @@ library V2Calculations {\\n         uint256 owedTime = _timestamp // Remove the line below\\n uint256(_lastRepaidTimestamp);\\n         interest_ = (interestOwedInAYear * owedTime) / daysInYear;\\n \\n// Remove the line below\\n        // Cast to int265 to avoid underflow errors (negative means loan duration has passed)\\n// Remove the line below\\n        int256 durationLeftOnLoan = int256(\\n// Remove the line below\\n            uint256(_bid.loanDetails.loanDuration)\\n// Remove the line below\\n        ) // Remove the line below\\n\\n// Remove the line below\\n            (int256(_timestamp) // Remove the line below\\n\\n// Remove the line below\\n                int256(uint256(_bid.loanDetails.acceptedTimestamp)));\\n// Remove the line below\\n        bool isLastPaymentCycle = durationLeftOnLoan <\\n// Remove the line below\\n            int256(uint256(_bid.terms.paymentCycle)) || // Check if current payment cycle is within or beyond the last one\\n// Remove the line below\\n            owedPrincipal_ // Add the line below\\n interest_ <= _bid.terms.paymentCycleAmount; // Check if what is left to pay is less than the payment cycle amount\\n// Remove the line below\\n\\n         if (_bid.paymentType == PaymentType.Bullet) {\\n// Remove the line below\\n            if (isLastPaymentCycle) {\\n// Remove the line below\\n                duePrincipal_ = owedPrincipal_;\\n// Remove the line below\\n            }\\n// Add the line below\\n            duePrincipal_ = owedPrincipal_;\\n         } else {\\n             // Default to PaymentType.EMI\\n             // Max payable amount in a cycle\\n             // NOTE: the last cycle could have less than the calculated payment amount\\n// Remove the line below\\n            uint256 maxCycleOwed = isLastPaymentCycle\\n// Remove the line below\\n                ? owedPrincipal_ // Add the line below\\n interest_\\n// Remove the line below\\n                : _bid.terms.paymentCycleAmount;\\n \\n             // Calculate accrued amount due since last repayment\\n// Remove the line below\\n            uint256 owedAmount = (maxCycleOwed * owedTime) /\\n// Add the line below\\n            uint256 owedAmount = (_bid.terms.paymentCycleAmount * owedTime) /\\n                 _bid.terms.paymentCycle;\\n             duePrincipal_ = Math.min(owedAmount // Remove the line below\\n interest_, owedPrincipal_);\\n         }\\n```\\n\\nAnd then NumbersLib.sol:\\n```\\ndiff // Remove the line below\\n// Remove the line below\\ngit a/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/NumbersLib.sol b/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/NumbersLib.sol\\nindex f34dd9c..8ca48bc 100644\\n// Remove the line below\\n// Remove the line below\\n// Remove the line below\\n a/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/NumbersLib.sol\\n// Add the line below\\n// Add the line below\\n// Add the line below\\n b/teller// Remove the line below\\nprotocol// Remove the line below\\nv2/packages/contracts/contracts/libraries/NumbersLib.sol\\n@@ // Remove the line below\\n120,7 // Add the line below\\n120,8 @@ library NumbersLib {\\n                 );\\n \\n         // Number of payment cycles for the duration of the loan\\n// Remove the line below\\n        uint256 n = Math.ceilDiv(loanDuration, cycleDuration);\\n// Add the line below\\n        uint256 n = loanDuration/ cycleDuration;\\n// Add the line below\\n        uint256 rest = loanDuration%cycleDuration;\\n \\n         uint256 one = WadRayMath.wad();\\n         uint256 r = WadRayMath.pctToWad(apr).wadMul(cycleDuration).wadDiv(\\n@@ // Remove the line below\\n128,8 // Add the line below\\n129,16 @@ library NumbersLib {\\n         );\\n         uint256 exp = (one // Add the line below\\n r).wadPow(n);\\n         uint256 numerator = principal.wadMul(r).wadMul(exp);\\n// Remove the line below\\n        uint256 denominator = exp // Remove the line below\\n one;\\n \\n// Remove the line below\\n        return numerator.wadDiv(denominator);\\n// Add the line below\\n        if(rest==0) {\\n// Add the line below\\n            // duration is multiple of cycle\\n// Add the line below\\n            uint256 denominator = exp // Remove the line below\\n one;\\n// Add the line below\\n            return numerator.wadDiv(denominator);\\n// Add the line below\\n        }\\n// Add the line below\\n        // duration is an uneven cycle\\n// Add the line below\\n        uint256 rDelta = WadRayMath.pctToWad(apr).wadMul(rest).wadDiv(daysInYear);\\n// Add the line below\\n        uint256 n1 = numerator.wadMul(one // Add the line below\\n rDelta);\\n// Add the line below\\n        uint256 denom = ((one // Add the line below\\n rDelta).wadMul(exp // Remove the line below\\n one)) // Add the line below\\n rDelta;\\n// Add the line below\\n        return n1.wadDiv(denom);\\n     }\\n }\\n```\\n
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.\\nThis 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.\\nThere is also nothing stopping an unknowing borrower from placing a bid or accepting a commitment with an odd duration.
```\\nFile: libraries/V2Calculations.sol\\n\\n 93:        // Cast to int265 to avoid underflow errors (negative means loan duration has passed)\\n 94:        int256 durationLeftOnLoan = int256(\\n 95:            uint256(_bid.loanDetails.loanDuration)\\n 96:        ) -\\n 97:            (int256(_timestamp) -\\n 98:                int256(uint256(_bid.loanDetails.acceptedTimestamp)));\\n 99:        bool isLastPaymentCycle = durationLeftOnLoan <\\n           int256(uint256(_bid.terms.paymentCycle)) || // Check if current payment cycle is within or beyond the last one\\n           owedPrincipal_ + interest_ <= _bid.terms.paymentCycleAmount; // Check if what is left to pay is less than the payment cycle amount\\n```\\n