The attackers front-running `repayloans` so that the debt cannot be repaid

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The attackers make it impossible for borrowers to repay their debts, and the collateral is liquidated when the debts mature.

Proof of Concept

repayLoan, need to check the loanId, if the id is inconsistent will revert.

    function repayLoan(LoanRepaymentData calldata _repaymentData) external override nonReentrant {
        uint256 loanId = _repaymentData.data.loanId;
        Loan calldata loan = _repaymentData.loan;
        .....
@>      _baseLoanChecks(loanId, loan);
        .....
    }

    function _baseLoanChecks(uint256 _loanId, Loan memory _loan) private view {
        if (_loan.hash() != _loans[_loanId]) {
            revert InvalidLoanError(_loanId);
        }
        if (_loan.startTime + _loan.duration < block.timestamp) {
            revert LoanExpiredError();
        }
    }

The problem is that _loans[_loanId] can change, for example, when mergeTranches delete the old loanId and write the new one.

    _loans[loanId] = loanMergedTranches.hash();
    delete _loans[_loanId];

An attacker can use the front-running attack method, when repayLoan is called, execute the mergeTranches function in advance, and make the id in _loans updated. In this case, the repayLoan execution will fail due to inconsistent _loanId.

If the attacker keeps using this attack, the borrower's debt will not be repaid, eventually causing the collateral to be liquidated.

In addition to the mergeTranches function, the attacker can also call addNewTranche, and the borrower can also call the refinance-related function, again causing _loanId to be updated.

An attacker can also use the same method to attack refinance related functions, making refinance unable to execute.

An attacker can also use the same method to attack the liquidateLoan function, making it impossible for debts to be cleared.

Tools Used

vscode, manual

Recommended Mitigation Steps

Do not delete _loanId

Assessed type

DoS

[0xA5DF]

I have some doubts about severity, since this requires too many resources from the attacker (see https://github.com/code-423n4/org/issues/143), and the addNewTranche() requires the lender's signature (and when using mergeTranches() alone the attacker would eventually run out of tranches to merge) Currently marking as med, I'll might consider downgrading to low

[c4-judge]

0xA5DF marked the issue as primary issue

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)

[0xend]

I think this is low (agree with judge for thsoe reasons).

[C4-Staff]

@0xend Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[0xA5DF]

I think there are too many limitations on this one, and the motivation for the attacker isn't very high - they're not going to get the entire principal from this. Marking as low, but I'm open to hear arguments for med

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xend]

Given the limit on tranches the attacker can only run this a handful of times

[0xA5DF]

Moved to #45

[c4-judge]

0xA5DF marked the issue as grade-c

[zhaojio]

I think it's a high risk Because anyone can be an attacker, so Lender can be an attacker, If the lender does not want the borrower to repay the debt, the lender can use addNewTranche/mergeTranches and to attack repayLoans and make the borrower's loan impossible to repay, especially when the loan is about to expire. This causes the borrower's NFT to be loss, so it would have a high impact.

When _liquidateLoan, if _canClaim == true, the borrower can get the NFT directly:

    function _liquidateLoan....{
       ....
        if (_canClaim) {
            ERC721(_loan.nftCollateralAddress).transferFrom(
                address(this), _loan.tranche[0].lender, _loan.nftCollateralTokenId
            );
            emit LoanForeclosed(_loanId);

            liquidated = true;
        } 
    ....
    }

     function liquidateLoan(uint256 _loanId, Loan calldata _loan)...  {
       .....
        (bool liquidated, bytes memory liquidation) = _liquidateLoan(
            _loanId, _loan, _loan.tranche.length == 1 && !getLoanManagerRegistry.isLoanManager(_loan.tranche[0].lender)
        );
       ......
     }

An attacker/lender can use mergeTranches to make _loan.tranche.length == 1

The key issue is that loanId will be reset.

[0xA5DF]

You're right that the lender has a high motivation to execute this attack You're also right that when the borrower attempts to repay close to the expiry time this attack becomes feasible While some conditions are required in order for this to work, it still seems pretty likely to happen.

Due to those reasons I'm reinstating high severity

(side note: I think that a better mitigation would be to not allow functions that change the loan ID to run near the expiry time)

[c4-judge]

This previously downgraded issue has been upgraded by 0xA5DF

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xend]

No specific PR here since it's addressed when limiting addNewTranche to only be able to be called by the borrower + checking in refinancePartial that there's at least one tranche being refinanced. This ends up limiting the # of times a loan can be locked by the lender (tranches are locked for some time after a refinance for future ones)