Dandy Caramel Tortoise - Using original principal amount as due amount inside liquidateDefaultedLoanWithIncentive breaks contract accounting leading to lost assets/broken functionalities

[sherlock-admin2]

Dandy Caramel Tortoise

High

Using original principal amount as due amount inside liquidateDefaultedLoanWithIncentive breaks contract accounting leading to lost assets/broken functionalities

Summary

Using original principal amount as due amount inside liquidateDefaultedLoanWithIncentive breaks contract accounting leading to lost assets/broken functionalities

Root Cause

In liquidateDefaultedLoanWithIncentive, the amount due is taken as the original principal amount of the bid rather than the remaining to be repaid principal which is incorrect as part of this principal could have already been paid back

    function liquidateDefaultedLoanWithIncentive(
        uint256 _bidId,
        int256 _tokenAmountDifference
    ) external whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) nonReentrant onlyOracleApprovedAllowEOA {
        
        //use original principal amount as amountDue

        uint256 amountDue = _getAmountOwedForBid(_bidId);

    function _getAmountOwedForBid(uint256 _bidId )
        internal
        view
        virtual
        returns (uint256 amountDue)
    {
        // @audit this is the entire principal amount which is incorrect
        (,,,, amountDue, , ,  )
         = ITellerV2(TELLER_V2).getLoanSummary(_bidId);  
    }

Parts of the original principal could have been repaid and accounted via this function leading to double counting of principal in totalPrincipalTokensRepaid

    function repayLoanCallback(
        uint256 _bidId,
        address repayer,
        uint256 principalAmount,
        uint256 interestAmount
    ) external onlyTellerV2 whenForwarderNotPaused whenNotPaused bidIsActiveForGroup(_bidId) { 
        totalPrincipalTokensRepaid += principalAmount;
        totalInterestCollected += interestAmount;

This leads to several problems:

1. Underflow in totalPrincipalTokensLended - totalPrincipalTokensRepaid as totalPrincipalTokensRepaid can double count repaid tokens causing bids to revert
2. Lost assets due to tokenDifferenceFromLiquidations calculating difference from totalPrincipal without considering the repaid assets

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. A loan is created with principal amount == 100
   tokenBalanceOfContract == 0

totalPrincipalTokensCommitted == 100
totalPrincipalTokensWithdrawn == 0
totalPrincipalTokensLended == 100
totalPrincipalTokensRepaid == 0
tokenDifferenceFromLiquidations == 0

2. Repayment of 80 principal occurs before the loan gets defaulted
   tokenBalanceOfContract == 80

totalPrincipalTokensCommitted == 100
totalPrincipalTokensWithdrawn == 0
totalPrincipalTokensLended == 100
totalPrincipalTokensRepaid == 80
tokenDifferenceFromLiquidations == 0

3. Loan defaults and auction settles at price 50 (similarly problematic paths are lenders withdrawing 80 first or the auction settling at higher prices)
   tokenBalanceOfContract == 80 + 50 == 130

totalPrincipalTokensCommitted == 100
totalPrincipalTokensWithdrawn == 0
totalPrincipalTokensLended == 100
totalPrincipalTokensRepaid == 80 + 100 == 180 => incorrect
tokenDifferenceFromLiquidations == (100 - 50 == -50) => incorrect

Now:

• available amount to withdraw will be calculated as (totalPrincipalTokensCommitted + tokenDifferenceFromLiquidations == 50) while there is actually 130 amount of assets available to withdraw causing loss for lenders
• getPrincipalAmountAvailableToBorrow will underflow because (totalPrincipalTokensLended - totalPrincipalTokensRepaid == -80) and no new bids can be accepted

There are more scenarios that arise from the same root cause such as estimated value becoming 0 incorrectly, which will cause division by 0 and hence revert on withdrawals, deposits will be lost or 0 shares will be minted etc.

Impact

Lost assets for users, broken functionalities

PoC

No response

Mitigation

Instead of the totalPrincipal consider the remaining principal ie. totalPrincipal - repaidPrincipal

[sherlock-admin2]

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