pkqs90 - In `LenderCommitmentGroup_Smart.sol#burnSharesToWithdrawEarnings()`, shares are burned before `principalTokenValueToWithdraw` is calculated, causing users to withdraw more tokens than expected.

[sherlock-admin2]

pkqs90

high

In LenderCommitmentGroup_Smart.sol#burnSharesToWithdrawEarnings(), shares are burned before principalTokenValueToWithdraw is calculated, causing users to withdraw more tokens than expected.

Summary

While burning shares, the amount of tokens for withdraw should be burnedShares * totalAssets / totalShares. However, the code burns shares before calculating the amount of withdrawn tokens, which means the totalShares is decreased. This leads to users being able to withdraw more tokens than expected.

Vulnerability Detail

burnSharesToWithdrawEarnings() is the function for users burning shares. The poolSharesToken.burn() is performed before calculating principalTokenValueToWithdraw, and we can see the poolSharesToken.totalSupply() is used while calculating the asset to share ratio.

An example of the issue is: Current pool has 10000 principalTokens and 5000 shares, a user withdraws 2500 shares. The amount of principalTokens he is suppose to withdraw is 5000, however since the 2500 shares are burned before calculation, the real value is 2500 * 10000 / (5000 - 2500) = 10000, and the user withdraws all of the principalTokens.

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

    function _valueOfUnderlying(uint256 amount, uint256 rate)
        internal
        pure
        returns (uint256 value_)
    {
        if (rate == 0) {
            return 0;
        }

        value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate;
    }

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

    function sharesExchangeRateInverse()
        public
        view
        virtual
        returns (uint256 rate_)
    {
        return
            (EXCHANGE_RATE_EXPANSION_FACTOR * EXCHANGE_RATE_EXPANSION_FACTOR) /
            sharesExchangeRate();
    }

Impact

Users withdraw more tokens than they should.

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

Tool used

Manual review

Recommendation

Burn the shares after calculating principalTokenValueToWithdraw.

Duplicate of #19