w42d3n - The function burnSharesToWithdrawEarnings() is subject to Re-entrancy Vulnerability

[sherlock-admin4]

w42d3n

medium

The function burnSharesToWithdrawEarnings() is subject to Re-entrancy Vulnerability

Summary

In the contract LenderCommitmentGroup_Smart.sol, the function burnSharesToWithdrawEarnings transfers the funds before updating the state. This order of execution exposes the function, and subsequently the contract, to re-entrancy attacks.

Vulnerability Detail

In a re-entrancy attack, a malicious contract can call back into the vulnerable contract before the latter had a chance to finish its execution.

Impact

This potentially allows the attacker to drain funds from the 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#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;
    }

Tool used

Manual Review

Recommendation

To prevent re-entrancy attack, the state should be updated before calling another contract and transferring any assets. Using the "checks-effects-interactions" pattern can mitigate this issue.
So the recommended change to the code is as follows:

function burnSharesToWithdrawEarnings(
        uint256 _amountPoolSharesTokens,
        address _recipient
    ) external returns (uint256) {
        poolSharesToken.burn(msg.sender, _amountPoolSharesTokens);

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

        totalPrincipalTokensWithdrawn += principalTokenValueToWithdraw;

        //Ensure the state (balance) is updated before transfer is made
        //This prevents re-entrancy attacks
        uint256 newBalance = totalPrincipalTokensWithdrawn;

        require(
            newBalance <= address(this).balance, 
            "Insufficient balance in contract"
        );

        principalToken.transfer(_recipient, principalTokenValueToWithdraw);

        return newBalance;
    }

Duplicate of #179