TellerV2.sol :: _repayLoan() is subject to Re-entrancy

Summary

The function _repayLoan() is subject to Re-entrancy attack, users' funds are at risk.

Vulnerability Detail

This vulnerability is present in the _repayLoan function , where during the execution of this function an external call is made to the transferFrom function which eventually allows the control to be transferred to a potentially malicious contract.

If the called contract has a fallback function, it can call back the _repayLoan function and exploit this re-entrancy vulnerability.

Impact

This re-entrancy might enable the malicious contract to repeatedly call the vulnerable function in a loop, draining the contract's funds or leading to other unexpected behavior.

The function _repayLoan() is called in the external function repayLoanMinimum(), ;Function for users to make the minimum amount due for an active loan;. I believe users can lost all their funds.

Code Snippet

The issue can be seen in the _repayLoan function: https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L887

function _repayLoan(
    uint256 _bidId,
    Payment memory _payment,
    uint256 _owedAmount,
    bool _shouldWithdrawCollateral
) internal virtual {
    ...
    _sendOrEscrowFunds(_bidId, _payment); // Problematic call
    ...
}

In the _sendOrEscrowFunds function: https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L911-L914

function _sendOrEscrowFunds(uint256 _bidId, Payment memory _payment) internal {
    ...
    bid.loanDetails.lendingToken.transferFrom{ gas: 100000 }( 
        _msgSenderForMarket(bid.marketplaceId),
        lender,
        _paymentAmount
    );
    ...
}

The transferFrom function could potentially call an external contract where the control can be transferred and can cause a re-entrancy attack.

Tool used

Manual Review

Recommendation

To prevent re-entrancy attacks, we recommend using the Checks-Effects-Interactions pattern. This means that any function that interacts with other contracts should be the last operation in the function. This prevents other contracts from seizing control flow and manipulating internal states or re-entering the function.

In this case, moving the _sendOrEscrowFunds(_bidId, _payment); line in the _repayLoan function to be the last operation before the function exits would mitigate this potential vulnerability.

The use of re-entrancy guard pattern is also a recommended solution, like OpenZeppelin's ReentrancyGuard.

Duplicate of #179

sherlock-audit / 2024-04-teller-finance-judging

w42d3n - TellerV2.sol :: _repayLoan() is subject to Re-entrancy #267