0xadrii - Burning shares prior to computing value to withdraw will make earnings remain locked forever in the contract

[sherlock-admin3]

0xadrii

high

Burning shares prior to computing value to withdraw will make earnings remain locked forever in the contract

Summary

The shares exchange rate is broken because shares are burnt prior to the rate being computed when withdrawing. This will make all user’s balances not work properly, and leave some funds locked forever in the contract.

Vulnerability Detail

The LenderCommitmentGroup_Smart acts as a liquidity pool where funds can be used to be lent to borrowers in Teller. At its core, LenderCommitmentGroup_Smart acts in a similar fashion to an ERC4626 vault, where liquidity providers will receive some shares when depositing, and burn them when withdrawing.

However, the current share burning flow is flawed. Considering the following code snippet of the function to burn shares:

// LenderCommitmentGroup_Smart

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

As we can see, initially poolSharesToken.burn is called and the requested _amountPoolSharesTokens is burnt. After that, the principalTokenValueToWithdraw (the amount of principal that the user should receive in exchange for their shares) is computed using the _valueOfUnderlying function, which performs the computation to determine how much principal the shares are worth.

Checking the call performed to _valueOfUnderlying, we see that the sharesExchangeRateInverse function is passed as the exchange rate to the function. The problem is that the computation of the exchange rate is extracted from the relation between the poolTotalEstimatedValue (amount of assets in the pool) and poolSharesToken.totalSupply(). Because the shares’ totalSupply has been reduced when burning the shares, the exchange rate will be wrongly calculated, given that it will consider less totalSupply than the real value:

// LenderCommitmentGroup_Smart
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

High. The shares exchange rate is broken and will lead to funds remaining stuck forever in the contract, as well as users not being able to withdraw their corresponding yield. In the worst scenario, users won’t even be able to recover any of their initial assets, given that the exchange rate will return an extremely deviated value that will allow users only to withdraw a small fraction of their assets.

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#L403-L407

Tool used

Manual Review

Recommendation

Compute the totalPrincipalTokensWithdrawn prior to actually burning the assets. This will make the exchange rate computation be properly performed:

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

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

+        poolSharesToken.burn(msg.sender, _amountPoolSharesTokens); 

        totalPrincipalTokensWithdrawn += principalTokenValueToWithdraw;

        principalToken.transfer(_recipient, principalTokenValueToWithdraw);

        return principalTokenValueToWithdraw;
    }

Duplicate of #19