An attacker can steal users tokens when adding shares via the classic first depositor bug case

Low/Info issue submitted by Bauchibred

Summary

Malicious users can perform an inflation attack so as to steal the assets of the victim.

Vulnerability Detail

NB: Submitted as low due to the protocol stating this to be OOS in the contest readMe, would be key to not that protocol somewhat claims this is acceptable, cause bug case also exists in Uniswap, however uniswap protects this by burning a min shares amount in the first deposit which is not done here.

A malicious user can perform a donation to execute a classic similar first depositor/ERC4626 inflation Attack against LenderCommitmentGroup_Smart.sol. The general process of this attack is well-known, and a detailed explanation of this attack can be found here https://mixbytes.io/blog/overview-of-the-inflation-attack.

In short, to kick-start the attack, the malicious user will often usually mint the smallest possible amount of shares (e.g., 1 wei) and then donate significant assets to the vault to inflate the number of assets per share. Subsequently, it will cause a rounding error when other users deposit.

Now in the LenderCommitmentGroup_Smart.sol contract, whenever anyone wants to add more principal this function is called to the commitment group https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L307-L333

    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);
         //@audit
        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_);
    }

        function _valueOfUnderlying(uint256 amount, uint256 rate)
        internal
        pure
        returns (uint256 value_)
    {
        if (rate == 0) {
            return 0;
        }
        //@audit
        value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate;
    }

We can see that the value returned from sharesExchangeRate() directly has an impact on the amount of shares that gets minted to the user, now here is the implementation of sharesExchangeRate() https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L262-L275


    function sharesExchangeRate() public view virtual returns (uint256 rate_) {
        //@audit
        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();
    }

Evidently in the case where the pool is just starting up, the rate goes for 1 to 1 for the first swap as hinted by the comments, now since EXCHANGE_RATE_EXPANSION_FACTOR == 1e36 would be returned by this function, this then means that the value minted from value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate is going to be exactly the amount minted.

Now there is also a functionality to withdraw shares, i.e the burnSharesToWithdrawEarnings().

This then allows an attacker to carry out a classic first depositor attack in this case, where they deposit early, mint shares, then withdraw almost all their shares, donate or transfer assets to the vault to inflate the assets per share and inflate the balance so that when the next depositor attempts depositing they have a messed up exchange rate with the attacker doing away with their tokens.

Note that this must not be done via front running as an attacker can just do this actions early enough so they are the first depositors, alternatively they could judt frontrun a honest user's attempt at depositing with these actions to steal their tokens.

Impact

Malicous users could steal the assets of the victim.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L262-L275

Tool used

Manual Review

Recommendation

Ensure that there is always a minimum number of shares to guard against inflation attack by minting a certain amount of shares to zero address (dead address) during contract deployment (similar to what has been implemented in Uniswap V2).

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