A malicious user can take on a loan using an existing borrower's collateral in refinanceFromLoanExecutionData()

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

A malicious user can take on a loan using an existing borrower's collateral in refinanceFromLoanExecutionData().

Proof of Concept

In MultiSourceLoan.sol, refinanceFromLoanExecutionData() doesn't check whether _loan.borrower == _loanExecutionData.borrower, which is open rooms for exploits.

BorrowerB(malicious) can sign a _loanExecutionData offer and initiate a refinanceFromLoanExecutionData() call with BorrowerA's loan. BorrowerB will use BorrowerA's collateral for his loan.

There are (2) vulnerabilities here: (1) _validateExecutionData will not check whether _loan.borrower == _executionData.borrower. In addition, it will directly bypass the check on executionData's borrower signature as long as msg.sender!=_loan.borrower.

(2) refinanceFromLoanExecutionData() doesn't check whether the new loanExecutiondata (_loanExecutionData) has the same nft tokenId(executionData.tokenId) as the existing loan(_loan.nftCollateralTokenId).

As a result, if _loanExecutionData.borrower(BorrowerB) initiates refinanceFromLoanExecutionData() call, the following would happen: a) msg.sender != _loan.borrower(BorrwerA), this bypass _validateExecutionData's signature check. Also, tt will not revert because no checks on address _borrower(loan.borrower==_loanExecutionData.borrower;

b)There is no check on _loanExecutionData.tokenId . As long as _loan and _loanExecutionData has the same principalAddress and the same nftCollateralAddress, _processOffersFromExecutionData() will succeed in transferring principal loans to BorrowerB.

c) as long as the _loan.borrower(BorrowerA) has the funds for repayment. (Note: BorrowerA might have approved MultiSourceLoan.sol for asset transfer if they are ready for repayments.), the tx will succeed. And BorrowerA's collateral will continually be locked for BorrowerB's new loan.

Note that the above steps can also happen in a front-running scenario, where BorrowerB sees that BorrowerA approves MultiSourceLoan.sol for loan repayments and front-run BorrowerA's repayment with a new loan.

Tools Used

Manual

Recommended Mitigation Steps

In _validateExecutionData, consider adding checks to ensure address _borrower == _executionData.borrower.

Assessed type

Other

[c4-judge]

0xA5DF marked the issue as primary issue

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report

[0xend]

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

[minhquanym]

Hi @0xA5DF, I believed this issue has incorrect argument.

(1) _validateExecutionData will not check whether _loan.borrower == _executionData.borrower. In addition, it will directly bypass the check on executionData's borrower signature as long as msg.sender!=_loan.borrower.

msg.sender != _loan.borrower(BorrwerA), this bypass _validateExecutionData's signature check.

In the codebase, we can see the logic clearly show that in case msg.sender != _borrower, the signature will be checked. Please forgive me if I'm missing something here.

if (msg.sender != _borrower) {
    _checkSignature(
        _executionData.borrower, _executionData.executionData.hash(), _executionData.borrowerOfferSignature
    );
}

[flowercrimson]

Hey @minhquanym , you missed the fact that _borrower passed in _validateExecutionData is _loan.borrower. So msg.sender !=_borrower is only checking if msg.sender != _loan.borrower, but there is no check on whether _loan.borrower == _executionData.borrower.

[minhquanym]

So when you say "bypass signature check", you mean the check is still performed but it checked the wrong signer right? I got it now. Thank you