pkqs90 - `LenderCommitmentGroup_Smart` is susceptible to donation attack

[sherlock-admin4]

pkqs90

medium

LenderCommitmentGroup_Smart is susceptible to donation attack

Summary

LenderCommitmentGroup_Smart implements a ERC4626 like share/asset concept. However, it is also susceptible to the famous ERC4626 donation (inflation) attack: attackers can donate a large amount of assets (principal tokens) quickly after initialization to inflate the asset/share ratio and steal tokens from initial depositers.

Vulnerability Detail

Please read https://docs.openzeppelin.com/contracts/5.x/erc4626 to first understand the donation attack.

To make it simple, an attack vector is given:

1. Attacker deposits 100 wei principal tokens, and receives 100 wei shares
2. Attacker "donates" 1e20 prinipal tokens.
3. Another user deposits 1e16 principal tokens, but since the asset/share is already 1e18, the share for this user is round down to 0.
4. Attacker can burn all his shares and retrieve all principal tokens, including the 1e16 tokens in step 3.

Steps 1-2 are done in 1 transaction to frontrun the victim in step 3.

Now, we show how to perform the "donation" part in LenderCommitmentGroup_Smart contract. Since the "asset amount" is calculated by function getPoolTotalEstimatedValue(), a simple token transfer wouldn't work here. We can do the following:

1. Attacker creates a bid and loan in TellerV2
2. Attacker calls TellerV2#claimLoanNFT and transfers the NFT to LenderCommitmentGroup_Smart as owner. Now, the LenderCommitmentGroup_Smart contract is able to call TellerV2#lenderCloseLoanWithRecipient, which means liquidateDefaultedLoanWithIncentive is callable.
3. Attacker wait until the loan is default.
4. Attacker calls liquidateDefaultedLoanWithIncentive(uint256 _bidId, int256 _tokenAmountDifference) with a very large _tokenAmountDifference. This will greatly increase tokenDifferenceFromLiquidations, which is used in getPoolTotalEstimatedValue().

Steps 1-3 are preparation steps, and step 4 can be used as the "donation" step in the attack.

Now, we have shown the attacker can find a way to perform "donation" in 1 transaction, and the donation attack is available.

Also, the asset/share ratio can also be increased by this attack, meaning that only large amount of asset deposits will result to non-zero shares, and retail users may not be able to participate depositing LP.

    function getPoolTotalEstimatedValue()
        public
        view
        returns (uint256 poolTotalEstimatedValue_)
    {

         int256 poolTotalEstimatedValueSigned = int256(totalPrincipalTokensCommitted) 
         + int256(totalInterestCollected)  + int256(tokenDifferenceFromLiquidations) 
         - int256(totalPrincipalTokensWithdrawn);

        //if the poolTotalEstimatedValue_ is less than 0, we treat it as 0.  
        poolTotalEstimatedValue_ = poolTotalEstimatedValueSigned > int256(0)
            ? uint256(poolTotalEstimatedValueSigned)
            : 0;
    }

    function liquidateDefaultedLoanWithIncentive(
        uint256 _bidId,
        int256 _tokenAmountDifference
    ) public bidIsActiveForGroup(_bidId) {
        uint256 amountDue = getAmountOwedForBid(_bidId, false);

        uint256 loanDefaultedTimeStamp = ITellerV2(TELLER_V2)
            .getLoanDefaultTimestamp(_bidId);

        int256 minAmountDifference = getMinimumAmountDifferenceToCloseDefaultedLoan(
                amountDue,
                loanDefaultedTimeStamp
            );

        require(
            _tokenAmountDifference >= minAmountDifference,
            "Insufficient tokenAmountDifference"
        );

        if (_tokenAmountDifference > 0) {
            //this is used when the collateral value is higher than the principal (rare)
            //the loan will be completely made whole and our contract gets extra funds too
            uint256 tokensToTakeFromSender = abs(_tokenAmountDifference);

            IERC20(principalToken).transferFrom(
                msg.sender,
                address(this),
                amountDue + tokensToTakeFromSender
            );

            tokenDifferenceFromLiquidations += int256(tokensToTakeFromSender);

            totalPrincipalTokensRepaid += amountDue;
        } else {

            uint256 tokensToGiveToSender = abs(_tokenAmountDifference);

            IERC20(principalToken).transferFrom(
                msg.sender,
                address(this),
                amountDue - tokensToGiveToSender
            );

            tokenDifferenceFromLiquidations -= int256(tokensToGiveToSender);

            totalPrincipalTokensRepaid += amountDue;
        }

        //this will give collateral to the caller
        ITellerV2(TELLER_V2).lenderCloseLoanWithRecipient(_bidId, msg.sender);
    }

Impact

1. First depositors may be frontrun and lose funds
2. Attackers may "inflate" the asset/share ratio, causing retail users with small amount of asset not be able to participate in depositing LP.

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#L307-L322
• 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#L288-L302

Tool used

Manual review

Recommendation

Like what https://docs.openzeppelin.com/contracts/5.x/erc4626 did, it is best to use introduce a virtual share concept.

Duplicate of #64