SilverChariot - When making a payment in OCC_Modular, interest is always calculated for one whole paymentInterval

[sherlock-admin4]

SilverChariot

medium

When making a payment in OCC_Modular, interest is always calculated for one whole paymentInterval

Summary

The loan APR is applied to the paymentInterval in OCC_Modular.amountOwed(). This will result in users paying more or less interest depending on the conditions around the loan at the moment of repayment.

Vulnerability Detail

In amountOwed() the APR is always applied to paymentInterval.

        interest = loans[id].principalOwed * loans[id].paymentInterval * loans[id].APR / (86400 * 365 * BIPS);

For better understanding the issue, I will give several examples:

1) Having gracePeriod > paymentInterval

Suppose paymentInterval = 5 days and gracePeriod = 20 days. Calling callLoan() after 25 days will make the user pay for only one paymentInterval, i.e 5 days instead of 25.

2) Using makePayment() in the beginning of an interval.

If a user want to make a payment in the beginning of a given interval, they will be charged interest for the whole interval, instead for only the time they have borrowed the loan for.

Impact

Miscalculation of interest.

Code Snippet

    function amountOwed(uint256 id) public view returns (
        uint256 principal, uint256 interest, uint256 lateFee, uint256 total
    ) {
        // 0 == Bullet.
        if (loans[id].paymentSchedule == 0) {
            if (loans[id].paymentsRemaining == 1) { principal = loans[id].principalOwed; }
        }
        // 1 == Amortization (only two options, use else here).
        else { principal = loans[id].principalOwed / loans[id].paymentsRemaining; }

        // Add late fee if past loans[id].paymentDueBy.
        if (block.timestamp > loans[id].paymentDueBy && loans[id].state == LoanState.Active) {
            lateFee = loans[id].principalOwed * (block.timestamp - loans[id].paymentDueBy) *
                loans[id].APRLateFee / (86400 * 365 * BIPS);
        }
        interest = loans[id].principalOwed * loans[id].paymentInterval * loans[id].APR / (86400 * 365 * BIPS);
        total = principal + interest + lateFee;
    }

Tool used

Manual Review

Recommendation

Add a new field lastInterestPaid to the Loan struct and use it to make the calculations.

Duplicate of #97

[sherlock-admin4]

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

panprog commented:

medium, dup of #97. This one assumes that makePayment is incorrect, comparing it to callLoan (which is assumed correct). Per my understanding, it's callLoan that is incorrect. Since this issue brings up the callLoan difference from makePayment, I consider this the same core reason.