Function `_checkStrictlyBetter()` does not check for `ImprovementMinimum`

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/loans/MultiSourceLoan.sol#L835-L846

Vulnerability details

Impact

The ImprovementMinimum defines the minimum improvement (in BPS) required for a strict improvement when users refinance an existing loan.

struct ImprovementMinimum {
    uint256 principalAmount;
    uint256 interest;
    uint256 duration;
}

At deployment, its value is set as

ImprovementMinimum internal _minimum = ImprovementMinimum(500, 100, 100);

The code snippets above demonstrate that three improvements are required when users refinance: principal amount, interest, and loan duration. However, the function _checkStrictlyBetter() currently checks only the APR (interest) in _checkTrancheStrictly(). The other principalAmount and endTime are checked to be larger than the previous values but not checked for minimum improvement in the function _checkStrictlyBetter().

Proof of Concept

As you can see, the _checkTrancheStrictly() checks the APR to be improved by __minimum.interest. However, the duration is only checked to be _offerEndTime < _loanEndTime in _checkStrictlyBetter().

function _checkTrancheStrictly(
    bool _isStrictlyBetter,
    uint256 _currentAprBps,
    uint256 _targetAprBps,
    ImprovementMinimum memory __minimum
) private pure {
    /// @dev If _isStrictlyBetter is set, and the new apr is higher, then it'll underflow.
    if (
        _isStrictlyBetter
            && ((_currentAprBps - _targetAprBps).mulDivDown(_PRECISION, _currentAprBps) < __minimum.interest)
    ) {
        revert InvalidRenegotiationOfferError();
    }
}

// @audit not check ImprovementMinimum for principalAmount and endTime
if (
    (
        (_offerPrincipalAmount - _loanPrincipalAmount > 0)
            && (
                (_loanAprBps * _loanPrincipalAmount - _offerAprBps * _offerPrincipalAmount).mulDivDown(
                    _PRECISION, _loanAprBps * _loanPrincipalAmount
                ) < minimum.interest
            )
    ) || (_offerFee > 0) || (_offerEndTime < _loanEndTime) 
) {
    revert NotStrictlyImprovedError();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add check to ensure the required minimum improvement when users refinance a loan.

Assessed type

Invalid Validation

[c4-judge]

0xA5DF marked the issue as primary issue

[0xend]

We should get rid of this one. This was left as legacy (only improving apr matters).

[0xA5DF]

Thanks, it indeed seems from the code that those checks were supposed to run, therefore this is a valid issue. However, regarding severity - I fail to see why this is a significant issue - how does this impact the borrower or other users?

Marking as low for now

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xend]

No impact on borrower/lender

[0xend]

https://github.com/pixeldaogg/florida-contracts/pull/361 changing

[0xA5DF]

Moving to #70

[c4-judge]

0xA5DF marked the issue as grade-c