KupiaSec - The `LenderCommitmentGroup_Smart` contract cannot use USDT as its principal token, because `USDT.transfer()` does not return a boolean value.

[sherlock-admin2]

KupiaSec

medium

The LenderCommitmentGroup_Smart contract cannot use USDT as its principal token, because USDT.transfer() does not return a boolean value.

Summary

The transferFrom(), transfer(), and approve() functions from the IERC20 standard return a boolean value, but the ones of USDT does not. This makes it impossible for USDT to be used as the principal token of the LenderCommitmentGroup_Smart contract.

Vulnerability Detail

The document of this protocol said, "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."

USDT is one of the most widely-used tokens on the Uniswap V3 decentralized exchange.

And, the USDT.transfer() function does not return a boolean value, which is different from other tokens that inherit the IERC20 standard.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol

    interface IERC20 {
        // ...
        function transfer(address to, uint256 value) external returns (bool);

        function approve(address spender, uint256 value) external returns (bool);

        function transferFrom(address from, address to, uint256 value) external returns (bool);
        // ...
    }

In the LenderCommitmentGroup_Smart.sol, however, the use of transferFrom(), transfer(), and approve() functions from the IERC20 standard makes it impossible for USDT to be used as the principal token, because any call to these functions would revert.

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

contract LenderCommitmentGroup_Smart is
    // ...
85      IERC20 public principalToken;
    // ...
313     principalToken.transferFrom(msg.sender, address(this), _amount);
    // ...
373     principalToken.transfer(_recipient, principalTokenValueToWithdraw);
    // ...
412     principalToken.approve(address(TELLER_V2), _principalAmount);
    // ...

Impact

The LenderCommitmentGroup_Smart contract cannot use USDT as its principal token.

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#L412

Tool used

Manual Review

Recommendation

SafeERC20 library should used for IERC20.

Duplicate of #50

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/30

[nevillehuang]

Highlights only transfer/transferFrom boolean reverts

• 92
• 142
• 194
• 211

Highlights only approval boolean reverts

• 9
• 26
• 37
• 49

Highlights both transfer/transferfrom and approval boolean reverts

• 128
• 239
• 266

Highlights only unchecked boolean return values for certain tokens (e.g. ZRX, BNB)

• 4
• 17
• 50
• 52
• 75
• 118
• 150
• 181
• 182
• 194
• 199
• 213
• 221
• 249
• 256
• 265
• 288

Highlights both transfer/transferfrom reverts and unchecked boolean return values

• 5
• 6
• 27
• 227

There are 2 impacts, although same fix is employed. Likely duplicates given same root cause of not using safe ERC20 transfers/approve

• Reverts from incorrect function signature or transfer/transferFrom/approve (e.g. USDT) --> Medium severity
• Unchecked return value, false without transferring and does not revert (e.g. ZRX, BNB) --> High severity, can repay/add principal tokens without transferring

[High] finding-token-unchecked-return-value

• 4
• 17
• 50 - best
• 52
• 75
• 118
• 150
• 181
• 182
• 194
• 199
• 213
• 221
• 249
• 256
• 265
• 288

[Medium] finding-token-revert-boolean

• 5
• 6
• 9
• 26
• 27
• 37
• 49
• 92
• 100
• 128
• 142
• 147
• 198
• 211
• 227
• 239 - best
• 266

[nevillehuang]

After discussions with @cvetanovv, planning to duplicate duplicates of #239 and #50 given same root cause of not using safe ERC20 transfers/approve. Given issue #239 has shown a potential high impact, all of its duplicates will be high severity as well based on sherlock duplications rules

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.