Return values of transfers/approvals are not checked `LenderCommitmentGroup_Smart.sol`

Summary

In various functions such as burnSharesToWithdrawEarnings, addPrincipalToCommitmentGroup, and liquidateDefaultedLoanWithIncentive, silent transfer failures may occur, resulting in different scenarios: potential loss of user funds, loss of protocol funds, and flawed variable values, ultimately affecting collateral distribution.

A silent failure could also occur in acceptFundsForAcceptBid() for the approval

Vulnerability Detail

First note that this has been stated in the readme: https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/README.md#L12-L16

---
### Q: If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of <a href="https://github.com/d-xo/weird-erc20" target="_blank" rel="noopener noreferrer">weird tokens</a> you want to integrate?

We are 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 .
---

Now in multiple instance in scope, the integrated ERC20 token is attempted to be approved/transferred/transferFrommed, however all instances of this in the LenderCommitmentGroup_Smart.sol contract calls the un-safe approve/transfer/transferFrom() defined in openzeppelin's ERC20 and the does not check the bool.

Now going to openzeppelin's ERC20's approvetransfer/transferFrom() implementation we can see them specified like this https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L41-L78 with the bool value to be returned.

    function transfer(address to, uint256 value) external returns (bool);
    (...snip)
    function approve(address spender, uint256 value) external returns (bool);
    (...snip)
    function transferFrom(address from, address to, uint256 value) external returns (bool);

This is done cause most standard ERC20 tokens do not revert on failures but rather return a bool.

Now in the current scope in the LenderCommitmentGroup_Smart.sol these unsafe methods of transfers are used in liquidateDefaultedLoanWithIncentive, burnSharesToWithdrawEarnings & addPrincipalToCommitmentGroup.

Since these attempts to transfer could silently fail, this then leads to multiple scenarios like:

For burnSharesToWithdrawEarnings, this could lead to user's loss of funds as the attempt to transfer would silently fail, but the user's shares are already burnt and effectively meaning that they receive 0 principal token for any _amountPoolSharesTokens they burn.
For addPrincipalToCommitmentGroup , this means loss of protocol funds as the attempt at transferring could fail but users still get minted the shares with the assumption that the transfer didn't fail and they can later on withdraw the shares via burnSharesToWithdrawEarnings as earnings.
Finally, for liquidateDefaultedLoanWithIncentive , here it's being used in two instances, both depending on whether the _tokenAmountDifference is -ve or +ve, but all in all this still ends up with the liquidator having to send these tokens to cover the liquidation attempt, which could in this case silently fail, and then the variables tokenDifferenceFromLiquidations & totalPrincipalTokensRepaid would both be flawed, exarcebatingly this then sends the collateral to the user via ITellerV2(TELLER_V2).lenderCloseLoanWithRecipient(_bidId, msg.sender); where as they didn'd successfulyy provide the assets for this liquidation.

Impact

As stated under ## Vulnerability Detail, current implementations in burnSharesToWithdrawEarnings, addPrincipalToCommitmentGroup, & liquidateDefaultedLoanWithIncentive are flawed, since silent transfer failures may occur, resulting in different scenarios: potential loss of user funds, loss of protocol funds, and flawed variable values, ultimately affecting collateral distribution.

As for the case of the approval's return value not being checked in acceptFundsForAcceptBid, this might break the implementation of the call to TellerV2.sol as they include some transfer logics that would need the approvals to succeed

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#L307-L322

And all other instances of unsafe approve/transfer/transferFrom whose return values are not checked.

Tool used

Manual Review

Recommendation

Consider checking the return values and ensuring that these transfers/approvals were successful and revert when they are not successful.

Alternatively, just use openzeppelin's SafeERC20 instead just as is done in TellerV2.sol https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L58

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
//(...snip)
    using SafeERC20 for IERC20;

Duplicate of #50

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

Bauchibred - Return values of transfers/approvals are not checked `LenderCommitmentGroup_Smart.sol` #194

Return values of transfers/approvals are not checked `LenderCommitmentGroup_Smart.sol`

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

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

Bauchibred - Return values of transfers/approvals are not checked `LenderCommitmentGroup_Smart.sol` #194

Return values of transfers/approvals are not checked LenderCommitmentGroup_Smart.sol

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Return values of transfers/approvals are not checked `LenderCommitmentGroup_Smart.sol`