0xadrii - Claiming loan NFT prevents lenders from closing loan and retrieving collateral

[sherlock-admin4]

0xadrii

high

Claiming loan NFT prevents lenders from closing loan and retrieving collateral

Summary

Lenders won’t be able to call lenderCloseLoan/lenderCloseLoanWithRecipient if the loan NFT has already been claimed due to wrongly checking sender against bid.lender, instead of getLoanLender.

Vulnerability Detail

When a user claims a loan NFT by calling claimLoanNFT, the bid.lender field will be set to USING_LENDER_MANAGER:

// TellerV2.sol

function claimLoanNFT(uint256 _bidId)
        external
        acceptedLoan(_bidId, "claimLoanNFT")
        whenNotPaused
    {
        ...

        // set lender address to the lender manager so we know to check the owner of the NFT for the true lender
        bid.lender = address(USING_LENDER_MANAGER); 

        // mint an NFT with the lender manager
        lenderManager.registerLoan(_bidId, sender);
    }

This makes Teller no longer able to check for who the lender of a loan is by directly accessing bid.lender, because it will return the USING_LENDER_MANAGER. As a workaround, the getLoanLender function has been added, which will return the owner of the claimed loan NFT instead of USING_LENDER_MANAGER so that checks against the lender can be properly performed:

// TellerV2.sol
function getLoanLender(uint256 _bidId)
        public
        view
        returns (address lender_)
    {
        lender_ = bids[_bidId].lender;

        if (lender_ == address(USING_LENDER_MANAGER)) {
            return lenderManager.ownerOf(_bidId);
        }

        //this is left in for backwards compatibility only
        if (lender_ == address(lenderManager)) {
            return lenderManager.ownerOf(_bidId);
        }
    }

Let’s now examine the lenderCloseLoanWithRecipient and lenderCloseLoan functions, which allow lenders to close defaulted loans and get the borrower’s collateral. Both of them internally call the _lenderCloseLoanWithRecipient function:

// TellerV2.sol

function _lenderCloseLoanWithRecipient(
        uint256 _bidId,
        address _collateralRecipient
    ) internal acceptedLoan(_bidId, "lenderClaimCollateral") {
        require(isLoanDefaulted(_bidId), "Loan must be defaulted.");

        Bid storage bid = bids[_bidId];
        bid.state = BidState.CLOSED;

        address sender = _msgSenderForMarket(bid.marketplaceId);
        require(sender == bid.lender, "only lender can close loan");

        ...

        collateralManager.lenderClaimCollateral(_bidId);

        emit LoanClosed(_bidId);
    }

As we can see, _lenderCloseLoanWithRecipient will check the sender of the call by directly comparing it with bid.lender instead of calling getLoanLender. This is wrong because, as mentioned before, bid.lender will be USING_LENDER_MANAGER if the loan NFT has been claimed, so the function will always revert with “only lender can close loan” reason.

This issue also occurs for the setRepaymentListenerForBid function:

// TellerV2.sol

function setRepaymentListenerForBid(uint256 _bidId, address _listener) 
        external
    {
        address sender = _msgSenderForMarket(bids[_bidId].marketplaceId);

        require(
            sender == bids[_bidId].lender,
            "Only bid lender may set repayment listener"
        );

        repaymentListenerForBid[_bidId] = _listener;
    }

Impact

The impact of this vulnerability is high. One could think that the only impact of this is a DoS, and as such it should be treated as medium by Sherlock (this could be accepted for the situation where setRepaymentListenerForBid can’t be called, as it does not affect any critical part in the system). However, the lender being prevented from closing a loan is actually a big issue where funds can remain stuck in the protocol and lost. This is why:

If a loan is defaulted, there’s two approaches to recovering back some value: liquidating the loan, or claiming the collateral by closing the loan.

Because Teller works with time-based loans, a situation can take place where the collateral is not attractive enough for the loan to be liquidated after its default (it would not make sense to liquidate a loan to get some collateral that is worth less than the borrow amount that the liquidator needs to pay back).

Hence, the lenderCloseLoanWithRecipient and lenderCloseLoan functions are provided as an alternative for the lender to recover some value if the loan isn’t attractive enough to be liquidated.

If, due to the vulnerability mentioned in this report, a lender can’t call lenderCloseLoanWithRecipient nor lenderCloseLoan, then he will effectively lose all of the loan’s value, as liquidating wouldn’t make sense, and closing the loan can’t be performed.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L590

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L755

Tool used

Manual Review

Recommendation

Fetch the lender using the getLoanLender function, instead of directly accessing bids[_bidId].lender:

// TellerV2.sol

function _lenderCloseLoanWithRecipient(
        uint256 _bidId,
        address _collateralRecipient
    ) internal acceptedLoan(_bidId, "lenderClaimCollateral") {
        require(isLoanDefaulted(_bidId), "Loan must be defaulted.");

        Bid storage bid = bids[_bidId];
        bid.state = BidState.CLOSED;

        address sender = _msgSenderForMarket(bid.marketplaceId);
-        require(sender == bid.lender, "only lender can close loan");
+        require(sender == getLoanLender(_bidId), "only lender can close loan");

        ...

        collateralManager.lenderClaimCollateral(_bidId);

        emit LoanClosed(_bidId);
    }

// TellerV2.sol

function setRepaymentListenerForBid(uint256 _bidId, address _listener) 
        external
    {
        address sender = _msgSenderForMarket(bids[_bidId].marketplaceId);

        require(
-            sender == bids[_bidId].lender,
+            sender == getLoanLender(_bidId),
            "Only bid lender may set repayment listener"
        );

        repaymentListenerForBid[_bidId] = _listener;
    }

Duplicate of #11