Malicious lenders can set the lender commitment contract as the repayment listener for their regular loans, leading to several issues

Summary

It is possible to perform a donation attack due to lenders being able to set the LenderCommitmentGroup_Smart contract as the repaymentListenerForBid for their loan, even if they don’t participate as liquidity providers in the LenderCommitmentGroup pool.

Vulnerability Detail

Currently, lenders can participate in Teller in two ways:

By directly lending their assets in a peer-to-peer manner in TellerV2
By depositing assets as liquidity providers in theLenderCommitmentGroup_Smart contract

When a loan is fulfilled via the theLenderCommitmentGroup_Smart contract, the following steps will take place:

The acceptFundsForAcceptBid function will be called from the forwarder. This function will perform some checks and call the internal _acceptBidWithRepaymentListener function, which will actualy lend the assets
_acceptBidWithRepaymentListener will be called. This function is really important, as it is the function that allows assets to be lent using the pool mechanism. Checking _acceptBidWithRepaymentListener, we can see that not only the loan is accepted by calling TellerV2’s lenderAcceptBid, but also the setRepaymentListenerForBid function is called:
```
// LenderCommitmentGroup_Smart.sol
 function _acceptBidWithRepaymentListener(uint256 _bidId) internal {
        ITellerV2(TELLER_V2).lenderAcceptBid(_bidId); //this gives out the funds to the borrower

        ILoanRepaymentCallbacks(TELLER_V2).setRepaymentListenerForBid(
            _bidId,
            address(this)
        );
    }
```
This means that every time a loan is accepted the LenderCommitmentGroup_Smart will be set as the repayment listener for that specific bid in the TellerV2 contract.

The LenderCommitmentGroup_Smart must be set as the repayment listener because every time a lender commitment loan is repaid, the repayLoanCallback will be called from TellerV2, which allows LenderCommitmentGroup_Smart to be aware of how many principal and interest tokens have been repaid:

// LenderCommitmentGroup_Smart.sol
function repayLoanCallback( 
        uint256 _bidId,
        address repayer,
        uint256 principalAmount,
        uint256 interestAmount
    ) external onlyTellerV2 { 
        //can use principal amt to increment amt paid back!! nice for math .
        totalPrincipalTokensRepaid += principalAmount;
        totalInterestCollected += interestAmount;
    }

The problem is that the repayLoanCallback function in the lender commitment contract does not perform any extra check besides executing the onlyTellerV2 modifier. This makes it possible for lenders to set the lender commitment as their repayment listener even if they aren’t liquidity providers in the commitment contract.

The consequence of this vulnerability is that anybody can artificially inflate the totalPrincipalTokensRepaid and totalInterestCollected variables without actually being part of a lender commitment loan. These two variables are extremely important as they’re used to perform relevant computations in LenderCommitmentGroup_Smart:

totalInterestCollected is used to determine the pool’s total estimated value via the getPoolTotalEstimatedValue function. Because of the fact that anybody can inflate this variable, it is possible to also inflate the pool’s estimated value. The pool’s estimated value is of extreme importance as well, as it is used in the sharesExchangeRate function to determine the exchange rate of the assets:
```
// LenderCommitmentGroup_Smart.sol
function sharesExchangeRate() public view virtual returns (uint256 rate_) {

        uint256 poolTotalEstimatedValue = getPoolTotalEstimatedValue();

        if (poolSharesToken.totalSupply() == 0) {
            return EXCHANGE_RATE_EXPANSION_FACTOR; // 1 to 1 for first swap
        }

        rate_ =
            (poolTotalEstimatedValue  *
                EXCHANGE_RATE_EXPANSION_FACTOR) /
            poolSharesToken.totalSupply();
    }
```
Inflating the pool’s estimated value is essentially the same attack as the well-known [ERC4626 inflation attack](https://medium.com/@shresthasubik/demystifying-the-inflation-attack-in-erc4626-c06301f7d4a4), where the amount of assets held in the pool is artificially inflated to make the share’s value drastically increase.
totalPrincipalTokensRepaid is used in the getTotalPrincipalTokensOutstandingInActiveLoans function, where it is substracted from the totalPrincipalTokensLended variable:
```
// LenderCommitmentGroup_Smart.sol
function getTotalPrincipalTokensOutstandingInActiveLoans()
        public
        view
        returns (uint256)
    {
        return totalPrincipalTokensLended - totalPrincipalTokensRepaid;
    }
```
Inflating totalPrincipalTokensRepaid will make the getTotalPrincipalTokensOutstandingInActiveLoans be uncallable, as totalPrincipalTokensRepaid will be greater than totalPrincipalTokensLended and an underflow will occur. Because getTotalPrincipalTokensOutstandingInActiveLoans will be completely DoS’ed, calling the getPrincipalAmountAvailableToBorrow and getPoolUtilizationRatio will also always revert (both of them use getTotalPrincipalTokensOutstandingInActiveLoans internally). All of these functions are used in the flow that allows users to accept bids in the lender commitment contract (the acceptFundsForAcceptBid function), so inflating totalPrincipalTokensRepaid will completely DoS the lender commitment’s lending functionality (which is its core mechanic).

Proof of concept

The following scenario demonstrates how an attacker can leverage inflating totalInterestCollected and totalPrincipalTokensRepaid to steal user’s assets.

An attacker deposits 1 wei of principal into the lender commitment. In exchange, the attacker receives 1 wei of shares.
The attacker then creates a loan to itself in TellerV2. He also sets the lender commitment contract as the repayment listener for his bid, and then repays the loan back to himself. The principal and interest that the attacker repays to himself will be added to the totalPrincipalTokensRepaid and totalInterestCollected variables in the lender commitment contract, respectively. Considering that the attacker repaid an interest amount of 20000 USDC, the lender commitment’s totalSupply is 1, while the pool’s estimated value is 20000e6 + 1 (the inflated assets + the initially deposited wei)
A regular user then provides 20000 USDC of liquidity. The share computation for the victim will be 20000e6 / 20000e6 + 1. This will make the victim obtain zero shares, while depositing the 20000 USDC, effectively losing all of the money to the pool.

It is important to note that this is one of the possible attacks. Because the donation is artificial and assets are actually never transferred from the attacker to the lender commitment contract, inflating these variables and hence inflating the share ratio will have many negative outcomes for the liquidity providers, such as assets remaining locked forever, while the cost of the attack always being close to zero.

Impact

High. As shown in the bug description, this issue leads to two outcomes:

Inflation of totalInterestCollected. As demonstrated, this leads to a complete DoS of the acceptFundsForAcceptBid function, which is a core contract mechanic. This has great impact on the protocol, although as per Sherlock’s rules the DoS should only be considered as medium impact.
Inflation of totalPrincipalTokensRepaid. As demonstrated, inflating this variable can actually lead to a scenario where the shares’ exchange rate is extremely inflated. This allows the attacker to perform an attack similar to the [ERC4626 inflation attack](https://medium.com/@shresthasubik/demystifying-the-inflation-attack-in-erc4626-c06301f7d4a4).

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Check that the repayLoanCallback function can only be triggered for loans created in the lender commitment. The following check can be added:

// LenderCommitmentGroup_Smart

function repayLoanCallback( 
        uint256 _bidId,
        address repayer,
        uint256 principalAmount,
        uint256 interestAmount
    ) external onlyTellerV2 { 
+       require(activeBids[_bidId], "Only lender commitment active bids are allowed");
        //can use principal amt to increment amt paid back!! nice for math .
        totalPrincipalTokensRepaid += principalAmount;
        totalInterestCollected += interestAmount;
    }

Duplicate of #42

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

0xadrii - Malicious lenders can set the lender commitment contract as the repayment listener for their regular loans, leading to several issues #225