search

code-423n4 / 2024-04-gondi-findings

0 stars 0 forks source link

The check on unlocked loan during refinance can be bypassed in addNewTranche #75

Closed c4-bot-5 closed 7 months ago

c4-bot-5 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

The check on unlocked loan during refinance can be bypassed in addNewTranche

Proof of Concept

In src/lib/loans/MultiSourceLoan.sol, when a lender initiated a refinance, checks (isLoanlocked()) need to be added to ensure only unlocked loans can be refinanced.

However, this check can be bypassed through addNewTranche, which can also be initiated by the lender.

In addNewTranche(), there is no check on whether msg.sender is borrower or lender, only the lender signature (_renegotiationOfferSignature) is checked. As a result, a lender can initiate the refinance flow through addNewTranche() at any time bypassing isLoanlocked() check.

//src/lib/loans/MultiSourceLoan.sol
    function addNewTranche(
        RenegotiationOffer calldata _renegotiationOffer,
        Loan memory _loan,
        bytes calldata _renegotiationOfferSignature
    ) external nonReentrant returns (uint256, Loan memory) {
...
        //@audit no check on isLoanLocked(), and no check on msg.sender
        _baseLoanChecks(loanId, _loan);
        _baseRenegotiationChecks(_renegotiationOffer, _loan);
        _checkSignature(_renegotiationOffer.lender, _renegotiationOffer.hash(), _renegotiationOfferSignature);
...
        Loan memory loanWithTranche = _addNewTranche(newLoanId, _loan, _renegotiationOffer);
...

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

For comparison, in refinanceFull or refinancePartial flows isLoanLocked() is checked when lender initiated the call.

Tools Used

Manual

Recommended Mitigation Steps

Add check for isLoanLocked() in addNewTranche flow.

Assessed type

Other

0xA5DF commented 7 months ago

The report isn't very clear, I need more details - an attack scenario and the impact (what are the consequences of bypassing this check? What assets are impacted?) Marking as insufficient proof for now, the warden can step up and explain this better (no need to wait for sponsor review or PJQA)

c4-judge commented 7 months ago

0xA5DF marked the issue as unsatisfactory: Insufficient proof

flowercrimson commented 7 months ago

I realized that I made a mistake in this issue: (1) lock feature is used to guarantee a refinance lender will have a loan for at least a minimum duration before another lender can refinance the loan. (2) There are two flows where unlock is not checked: refinanceFromLoanExecutionData and addNewTranche. In my issue, I note that addNewTranche is vulnerable, but since addNewTranche doesn't not close previous tranches. Whether a lock feature is added or not has no material impact. (3) doc: https://app.gitbook.com/o/4HJV0LcOOnJ7AVJ77p8e/s/W2WSJrV6PSLWo4p8vIGq/refinancing#refinancing-lock-ups