0x73696d616f - OCC_Modular::amountOwed() calculates lateFee incorrectly if more than 1 payment was missed

[sherlock-admin4]

0x73696d616f

high

OCC_Modular::amountOwed() calculates lateFee incorrectly if more than 1 payment was missed

Summary

loans[id].APRLateFee is the annualized late fee accrued on the outstanding principal of a loan. However, the code overcalculates the late fee, leading to significantly higher fees in case of late payments delayed by more than paymentInterval since the first payment due time (essentially missing more than 1 payment).

Vulnerability Detail

loans[id].APRLateFee is, as stated in the docs 1, 2, respectively.

The APR charged on the outstanding principal if payment is late.

In the event of a missed payment, late fee interest begins to accrue on the outstanding principal.

However, if more than one payment is missed, the fee is not applied on the outstanding principal, but on n times this amount, where n is the number of missed payments for bullet loans; or outstandingPrincipal/n + outstandingPrincipal*(n-1)/n + ...) for amortization loans. Thus, for example, if a borrower misses 2 payments, in bullet loans (simpler calculations), and the apr is 10%, the actual late fees will be close to 20% (sligthly less as block.timestamp - loans[id].paymentDueBy is lower for the second payment).

The reason is that OCC_Modular::amountOwed() always calculates the late fee to be applied on block.timestamp - loans[id].paymentDueBy, and payments always have to be payed in sequence, that is, if a borrower misses 2 payments, it has to call OCC_Modular::makePayment() twice.

This issue is also confirmed by the fact that OCC_Modular::callLoan() only charges late fees on the first missed payment and then closes the loan, which is not what would happen in OCC_Modular::makePayment(), as described above.

Add the following test toTest_OCC_Modular.sol (forked at block 19718909). As can be seen, the fee paid is almost double the expected.

function test_POC_OCC_Late_Fee_Bigger_APR() public {
    uint96 random = 10_000e18 + 1001;
    bool choice = true; // bullet

    (uint256 _loanID_DAI,,,) = simulateITO_and_createOffers_and_acceptOffers(random, choice);

    // Pre-state DAI.
    (,, uint256[10] memory _preInfo) = OCC_Modular_DAI.loanInfo(_loanID_DAI);

    hevm.warp(block.timestamp + _preInfo[3] + _preInfo[5] * _preInfo[6] + 1);

    (, uint256 interestOwed, uint256 lateFee,) = OCC_Modular_DAI.amountOwed(_loanID_DAI);

    assertEq(lateFee, 54560352529838914262189); // 10000000000000000001002*(3437305549 - 1718409600)*1001/(86400*365*10000)

    assert(tim.try_makePayment(address(OCC_Modular_DAI), _loanID_DAI));

    (, interestOwed, lateFee,) = OCC_Modular_DAI.amountOwed(_loanID_DAI);

    assertEq(lateFee, 54521958009290969056706); // 10000000000000000001002 * (3437305549 - 1718409600 - 1209600)*1001/(86400*365*10000)
}

Impact

Loan late fee is incorrect and will punish borrowers more than supposed by a large amount.

Code Snippet

OCC_Modular::amountOwed()

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

Vscode

Foundry

Recommendation

Late fees should only apply on the first payment when several payments are missing so the APR is correctly calculated. To do this, a variable can be added to struct loan, lastPayment, which tracks the latest payment and applies the late fee only on block.timestamp - loans[id].lastPayment. For example:

struct Loan {
    ...
    uint256 lastPayment;
}
...
function acceptOffer(uint256 id) external nonReentrant {
    ...
    loans[id].lastPayment = block.timestamp - block.timestamp % 7 days + 9 days + loans[id].paymentInterval;
    ...
}
...
function amountOwed(uint256 id) public view returns (
    uint256 principal, uint256 interest, uint256 lateFee, uint256 total
) {
    ...
    if (block.timestamp > loans[id].paymentDueBy && loans[id].state == LoanState.Active) {
        lateFee = loans[id].principalOwed * (block.timestamp - loans[id].lastPayment) *
            loans[id].APRLateFee / (86400 * 365 * BIPS);
    }
    ...
}
...
function makePayment(uint256 id) external nonReentrant {
    ...
    loans[id].lastPayment = block.timestamp;
    ...
}

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.