Function `refinanceFromLoanExecutionData()` does not check `executionData.tokenId == loan.nftCollateralTokenId`

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The refinanceFromLoanExecutionData() function is used to refinance a loan from LoanExecutionData. It allows borrowers to use outstanding offers for new loans to refinance their current loan. This function essentially combines two actions: it processes the repayment for the previous loan and then emits a new loan.

The key difference is that the same NFT is used as collateral for the new loan, so it does not need to be transferred out of the protocol and then transferred back in. However, there is no check to ensure that the NFT id of the old loan matches the NFT id of the new execution data.

Therefore, the new loan may have a collateral NFT that does not match the NFT that the lender requested in their offers.

/// @dev We first process the incoming offers so borrower gets the capital. After that, we process repayments.
///      NFT doesn't need to be transfered (it was already in escrow)
(uint256 newLoanId, uint256[] memory offerIds, Loan memory loan, uint256 totalFee) =
_processOffersFromExecutionData(
    borrower,
    executionData.principalReceiver,
    principalAddress,
    nftCollateralAddress,
    executionData.tokenId, // @audit No check if matched with loan.nftCollateralTokenId
    executionData.duration,
    offerExecution
);

Proof of Concept

As we can see, executionData.tokenId is passed to the _processOffersFromExecutionData() function instead of loan.nftCollateralTokenId. This function performs all the checks to ensure that the lenders' offers accept this NFT.

function _processOffersFromExecutionData(
    address _borrower,
    address _principalReceiver,
    address _principalAddress,
    address _nftCollateralAddress,
    uint256 _tokenId,
    uint256 _duration,
    OfferExecution[] calldata _offerExecution
) private returns (uint256, uint256[] memory, Loan memory, uint256) {
  ...
  _validateOfferExecution(
      thisOfferExecution,
      _tokenId,
      offer.lender,
      offer.lender,
      thisOfferExecution.lenderOfferSignature,
      protocolFee.fraction,
      totalAmount
  );
  ...
}

Eventually, it calls the _checkValidators() function to check the NFT token ID.

function _checkValidators(LoanOffer calldata _loanOffer, uint256 _tokenId) private {
    uint256 offerTokenId = _loanOffer.nftCollateralTokenId;
    if (_loanOffer.nftCollateralTokenId != 0) {
        if (offerTokenId != _tokenId) {
            revert InvalidCollateralIdError();
        }
    } else {
        uint256 totalValidators = _loanOffer.validators.length;
        if (totalValidators == 0 && _tokenId != 0) {
            revert InvalidCollateralIdError();
        } else if ((totalValidators == 1) && (_loanOffer.validators[0].validator == address(0))) {
            return;
        }
        for (uint256 i = 0; i < totalValidators;) {
            IBaseLoan.OfferValidator memory thisValidator = _loanOffer.validators[i];
            IOfferValidator(thisValidator.validator).validateOffer(_loanOffer, _tokenId, thisValidator.arguments);
            unchecked {
                ++i;
            }
        }
    }
}

However, since executionData.tokenId is passed in, an attacker could pass in a valid tokenId (an NFT id that will be accepted by all lender offers). But in reality, the loan.nftCollateralTokenId will be the NFT kept in escrow.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check to ensure that executionData.tokenId is equal to loan.nftCollateralTokenId.

Assessed type

Invalid Validation

[c4-judge]

0xA5DF marked the issue as duplicate of #14

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report