Brisk Sapphire Chipmunk - ERC20.approve Used Instead of Safe Approvals, Causing Pool Failures with Some ERC20s

[sherlock-admin3]

Brisk Sapphire Chipmunk

Medium

ERC20.approve Used Instead of Safe Approvals, Causing Pool Failures with Some ERC20s

Summary

The function acceptFundsForAcceptBid handles accepting a loan bid, transferring funds to the borrower, and securing collateral. It approves the TELLER_V2 contract to transfer the loan's principal amount using "ERC20.approve".

However, some ERC20s on some chains don't return a value.
The most popular example is USDT and USDC on the main net, and as the docs mention it should be compatible on any EVM chain and will support USDT:

Q: On what chains are the smart contracts going to be deployed?
Ethereum Mainnet, Polygon PoS, Arbitrum One, Base

Q: If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of [weird tokens](https://github.com/d-xo/weird-erc20) you want to integrate?
We absolutely do want to fully support all mainstream tokens like USDC, USDT, WETH, MOG, PEPE, etc. and we already took special consideration to make sure USDT works with our contracts.

Despite this claim, the current implementation of the approve function does not account for tokens like USDT, contradicting the protocol's intentions.
Therefore acceptFundsForAcceptBid will never work on the EVM Chain or other related chain and tokens.

Root Cause

In LenderCommitmentGroup_Smart.soL: 556, approve function is used instead of safeApprove. USDT on the main net doesn't return a value, https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code. This includes USDC which should work in the protocol.
This behavior causes the approve function to revert when interacting with these tokens.

 function acceptFundsForAcceptBid(
        address _borrower,
        uint256 _bidId,
        uint256 _principalAmount,
        uint256 _collateralAmount,
        address _collateralTokenAddress,
        uint256 _collateralTokenId, 
        uint32 _loanDuration,
        uint16 _interestRate
    ) external onlySmartCommitmentForwarder whenForwarderNotPaused whenNotPaused {
        
        require(
            _collateralTokenAddress == address(collateralToken),
            "Mismatching collateral token"
        );
        //the interest rate must be at least as high has the commitment demands. The borrower can use a higher interest rate although that would not be beneficial to the borrower.
        require(_interestRate >= getMinInterestRate(_principalAmount), "Invalid interest rate");
        //the loan duration must be less than the commitment max loan duration. The lender who made the commitment expects the money to be returned before this window.
        require(_loanDuration <= maxLoanDuration, "Invalid loan max duration");

        require(
            getPrincipalAmountAvailableToBorrow() >= _principalAmount,
            "Invalid loan max principal"
        );
 
 
        uint256 requiredCollateral = calculateCollateralRequiredToBorrowPrincipal(
            _principalAmount
        );



        require(    
             _collateralAmount   >=
                requiredCollateral,
            "Insufficient Borrower Collateral"
        );
 
        principalToken.approve(address(TELLER_V2), _principalAmount);
//do not have to override msg.sender as this contract is the lender !
        _acceptBidWithRepaymentListener(_bidId);

        totalPrincipalTokensLended += _principalAmount;

        activeBids[_bidId] = true; //bool for now
        

        emit BorrowerAcceptedFunds(  
            _borrower,
            _bidId,
            _principalAmount,
            _collateralAmount, 
            _loanDuration,
            _interestRate 
         );
    }

The specific part of the function is highlighted her;

        principalToken.approve(address(TELLER_V2), _principalAmount);

        //do not have to override msg.sender as this contract is the lender !

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L556

Internal pre-conditions

1. The protocol uses approve calls in functions like acceptFundsForAcceptBid.
2. There is no consideration or error handling for approve calls when interacting with USDT, USDC.

External pre-conditions

1. The protocol operates on EVM-compatible chains where these tokens are prevalent.

Attack Path

1. Users or lenders supply USDT or USDC as the principalToken.
2. Transactions that include approve calls revert, causing the protocol to fail in providing the intended functionality.

Impact

1. The protocol becomes unusable with ERC20 tokens like USDT AND USDC, a widely used stablecoin.
2. Breaks the intended functionality of the protocol.
3. This breaks a criticial function in the protocol and causes the pool to fail.

PoC

No response

Mitigation

Use safeApprove instead of approve

require(    
             _collateralAmount   >=
                requiredCollateral,
            "Insufficient Borrower Collateral"
        );
principalToken.safeApprove(address(TELLER_V2), _principalAmount);

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
teller-protocol/teller-protocol-v2-audit-2024#78