Lender commitment group smart contract won't work properly with fee-on-transfer tokens

Summary

The LenderCommitmentGroup_Smart currently doesn’t support fee-on-transfer tokens, leading to accounting issues.

Vulnerability Detail

All token transfers performed in LenderCommitmentGroup_Smart assume that the token transferred does not include any fee when transferring. This can be clearly seen in the addPrincipalToCommitmentGroup function, where the _amount transferred is directly accounted in the totalPrincipalTokensCommitted storage variable:

// LenderCommitmentGroup_Smart.sol
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);

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

The totalPrincipalTokensCommitted is an important variable used to compute the shares’ exchange rate in LenderCommitmentGroup_Smart. Not considering fees will lead to situations where users won’t be able to fully retrieve their funds and burn all their shares, given that the contract will believe that there are more funds deposited in the contract than the actual real deposited amount.

Note regarding fee-on-transfer tokens and the scope of the audit: as mentioned in Teller's Sherlock contest page, the team is “allowing any standard token that would be compatible with Uniswap V3 to work with our codebase, just as was the case for the original audit of TellerV2.sol . The tokens are assumed to be able to work with Uniswap V3 .” Fee-on-transfer tokens were supported in Teller’s latest Sherlock contest, and some parts of the code also show the desire to support these kind of tokens. This is why this issue must be considered as valid.

Impact

Medium. The exchange rate of the shares won’t consider the fees applied in token transfers. This will cause accounting issues mainly in the shares exchange rate computations, making users unable to fully retrieve their funds. Given that every time tokens are transferred from and to LenderCommitmentGroup_Smart a bigger accounting error will be incurred, the exchange will break exponentially, potentially leading to a big amount of funds being stuck in 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#L313

Tool used

Manual Review

Recommendation

Consider computing the difference between balances every time a token transfer is performed. This will show the real contract’s balance, and will enable LenderCommitmentGroup_Smart to properly account for the actual funds held in the contract.

Duplicate of #122

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

0xadrii - Lender commitment group smart contract won't work properly with fee-on-transfer tokens #241