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

13 stars 11 forks source link

NoOne - M-3: Reentrancy Vulnerability in burnSharesToWithdrawEarnings Function #188

Closed sherlock-admin2 closed 6 months ago

sherlock-admin2 commented 6 months ago

NoOne

medium

M-3: Reentrancy Vulnerability in burnSharesToWithdrawEarnings Function

Summary

The burnSharesToWithdrawEarnings function in the smart contract allows users to convert their pool shares tokens into an equivalent value of principal tokens and withdraw them from the contract. However, the function is susceptible to reentrancy attacks, which could potentially enable attackers to manipulate token balances and withdraw more principal tokens than intended.

Vulnerability Detail

The vulnerability arises from the lack of proper mitigation against reentrancy attacks in the burnSharesToWithdrawEarnings function. By exploiting reentrancy, attackers could execute recursive calls to external contracts or functions before the function completes its execution. During these recursive calls, attackers could manipulate token balances, withdraw excessive amounts of principal tokens, or disrupt the intended distribution of earnings among token holders.

Impact

Without implementing the checks-effects-interactions pattern, the function burnSharesToWithdrawEarnings is vulnerable to reentrancy attacks. If an attacker exploits this vulnerability, they could potentially manipulate the tokens transferred as principal tokens, leading to loss of funds or unauthorized withdrawals.

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

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

Tool used

Manual Review

Recommendation

Implementing the checks-effects-interactions pattern to ensure that state changes are performed before interacting with external contracts.

Duplicate of #179

nevillehuang commented 5 months ago

Invalid, lack required PoC by sherlock for reentrancy issue. Additionally, you cannot currently reenter any existing ERC20 transfers.