Spam function refinancePartial() could DOS refinance by passing empty _renegotiationOffer.trancheIndex list

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The Gondi protocol has created an open market where anyone can refinance a loan, provided the new loan term is better than the previous one by a certain percentage (5-10%). To interact with an existing loan (either by refinancing or renegotiating), users need to input the loanId. The refinanced or renegotiated loan is then tracked by a new loan ID.

The function refinancePartial() allows anyone to refinance some tranches of an existing loan. The caller specifies which tranches to refinance by providing a _renegotiationOffer.trancheIndex list. This list can be empty. If the _renegotiationOffer.trancheIndex list is empty, a new loan ID is simply created to track the loan without altering any parameters of the existing loan.

This could be exploited by an attacker to prevent other users from interacting with a specific loan. The attacker could call refinancePartial() with an empty trancheIndex list, incurring no cost beyond the gas cost. If the attacker sees a transaction in the mempool attempting to refinance or renegotiate a loan, they can front-run the transaction and call the refinancePartial() function. This changes the loan ID, invalidating the offer in the other transaction and causing it to revert.

function refinancePartial(RenegotiationOffer calldata _renegotiationOffer, Loan memory _loan)
    external
    returns (uint256, Loan memory)
{
    if (msg.sender != _renegotiationOffer.lender) {
        revert InvalidCallerError();
    }
    if (_isLoanLocked(_loan.startTime, _loan.startTime + _loan.duration)) {
        revert LoanLockedError();
    }

    uint256 loanId = _renegotiationOffer.loanId;
    _baseLoanChecks(loanId, _loan);
    _baseRenegotiationChecks(_renegotiationOffer, _loan);

    uint256 newLoanId = _getAndSetNewLoanId();
    uint256 totalProtocolFee;
    uint256 totalAnnualInterest;
    uint256 totalRefinanced;
    /// @dev bring to mem
    ImprovementMinimum memory minimum = _minimum;
    /// @dev We iterate over all tranches to execute repayments.
    for (uint256 i; i < _renegotiationOffer.trancheIndex.length;) {
        ...
    }

    /// @dev A partial refinance cannot reduce principal. If it's higher, we added as the most junior tranche.
    if (_renegotiationOffer.principalAmount < totalRefinanced) {
        revert InvalidRenegotiationOfferError();
    } else if (_renegotiationOffer.principalAmount > totalRefinanced) {
        _addTrancheFromPartial(_loan, _renegotiationOffer, totalRefinanced, totalAnnualInterest);
    }

    _handleProtocolFeeForFee(
        _loan.principalAddress, _renegotiationOffer.lender, totalProtocolFee, _protocolFee.recipient
    );

    _loans[newLoanId] = _loan.hash();
    delete _loans[loanId];

    /// @dev Here reneg fee is always 0
    emit LoanRefinanced(_renegotiationOffer.renegotiationId, loanId, newLoanId, _loan, 0);

    return (newLoanId, _loan);
}

Proof of Concept

Consider the parameters the attacker needs to input to spam refinancePartial() without incurring any cost.

_renegotiationOffer.trancheIndex.length = 0;
=> totalProtocolFee = 0
=> totalRefinanced = 0

_renegotiationOffer.principalAmount = 0;
=> _addTrancheFromPartial() is not executed because _renegotiationOffer.principalAmount == totalRefinanced

As we can see, users won't pay any protocol fee or refinance any tranche, yet a newLoanId is still created and the existing loan is set to this newLoanId.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check to ensure _renegotiationOffer.trancheIndex.length > 0.

Assessed type

DoS

[c4-judge]

0xA5DF marked the issue as primary issue

[0xA5DF]

Similar to #35, keeping it separate for now
I'm considering downgrading to low ( code-423n4/org#143 )

[0xend]

https://github.com/pixeldaogg/florida-contracts/pull/368

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Marking as low, this would require lots of resources and there's no significant motivation for the attacker

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moved to #70

[minhquanym]

@0xA5DF Can you help to check if this could be considered as dup of #35 please?

This could be exploited by an attacker to prevent other users from interacting with a specific loan.

[0xA5DF]

Yeah, the root cause is similar (despite being present in different functions), I'd dupe this with 25% due to not identifying the more significant impact

[c4-judge]

This previously downgraded issue has been upgraded by 0xA5DF

[c4-judge]

0xA5DF marked the issue as duplicate of #35

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as partial-25