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

10 stars 9 forks source link

KupiaSec - A sandwich attack can potentially take most of the interest earned within the `LenderCommitmentGroup_Smart` contract #217

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

KupiaSec

high

A sandwich attack can potentially take most of the interest earned within the LenderCommitmentGroup_Smart contract

Summary

When borrowers make repayments, it increases the price of the associated shares at once. A malicious user could exploit this by depositing a large amount of the underlying assets before the repayment, and then withdrawing them after the repayment has been made. This would allow the malicious user to capture a significant portion of the interest earned. A miner could potentially maximize the impact of this attack.

Vulnerability Detail

When a repayment are made, it increases the price of the shares.

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#L700-L709


    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;
    }

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


    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;
    }

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#L262-275


    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();
    }

A malicious user could exploit this by depositing a large amount of the underlying assets before the repayment, and then withdrawing them after the repayment has been made. This would allow the malicious user to capture a significant portion of the interest earned.

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


    function addPrincipalToCommitmentGroup(
        uint256 _amount,
        address _sharesRecipient
    ) external returns (uint256 sharesAmount_) {
        //transfers the primary principal token from msg.sender into this contract escrow

        principalToken.transferFrom(msg.sender, address(this), _amount);

@>      sharesAmount_ = _valueOfUnderlying(_amount, sharesExchangeRate());

        totalPrincipalTokensCommitted += _amount;
        //principalTokensCommittedByLender[msg.sender] += _amount;

        //mint shares equal to _amount and give them to the shares recipient !!!
        poolSharesToken.mint(_sharesRecipient, sharesAmount_);
    }

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#L396-L415


    function burnSharesToWithdrawEarnings(
        uint256 _amountPoolSharesTokens,
        address _recipient
    ) external returns (uint256) {

        poolSharesToken.burn(msg.sender, _amountPoolSharesTokens);

        uint256 principalTokenValueToWithdraw = _valueOfUnderlying(
            _amountPoolSharesTokens,
@>          sharesExchangeRateInverse()
        );

        totalPrincipalTokensWithdrawn += principalTokenValueToWithdraw;

        principalToken.transfer(_recipient, principalTokenValueToWithdraw);

        return principalTokenValueToWithdraw;
    }

Consider the following scenario:

  1. Add a large amount of principle tokens into the pool.
  2. Some repayments are made.(A miner can capture all repayment transactions from the transaction pool and put all of them in the block.)
  3. Burn all shares.

A miner could potentially maximize the impact of this attack. If he has enoughprinciple tokens, he can take most of the interest earned.

Impact

A sandwich attack can potentially take most of the interest earned within the LenderCommitmentGroup_Smart contract.

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

Tool used

Manual Review

Recommendation

Reward distribution mechanism should be improved to prevent sandwich attacks. Additionally, I recommend that withdrawl should be disabled in the same block to prevent flashloan.

Duplicate of #110