Unchecked return value bug on `TransferHelper::_safeTransferFrom()`

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L21 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L26-L29 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L35-L37 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/TransferHelper.sol#L42

Vulnerability details

Impact

Currently, the Wise Lending protocol supports several ERC-20 tokens and will also support more tokens in the future:

ERC20 in scope: WETH, WBTC, LINK, DAI, WstETH, sDAI, USDC, USDT, WISE and may others in the future. (Also corresponding Aave tokens if existing)

On some ERC-20 tokens, the transferFrom() will return false on failure instead of reverting a transaction. I noticed that the TransferHelper::_safeTransferFrom(), which is used throughout the protocol, is vulnerable to detecting the token transfer failure if the transferFrom() returns false due to the unchecked return value bug.

The following lists the functions directly invoking the vulnerable _safeTransferFrom():

1. WiseCore::_coreLiquidation()
2. WiseLending::depositExactAmount()
3. WiseLending::solelyDeposit()
4. WiseLending::paybackExactAmount()
5. WiseLending::paybackExactShares()
6. FeeManager::paybackBadDebtForToken()
7. FeeManager::paybackBadDebtNoReward()
8. PendlePowerFarm::_manuallyPaybackShares()
9. PendlePowerManager::enterFarm()
10. PendlePowerFarmController::exchangeRewardsForCompoundingWithIncentive()
11. PendlePowerFarmController::exchangeLpFeesForPendleWithIncentive()
12. PendlePowerFarmController::lockPendle()
13. PendlePowerFarmToken::addCompoundRewards()
14. PendlePowerFarmToken::depositExactAmount()
15. AaveHub::depositExactAmount()
16. AaveHub::paybackExactAmount()
17. AaveHub::paybackExactShares()

Due to the unchecked return value bug, users or attackers can exploit these protocol's functions without supplying a token (please refer to the Proof of Concept section for more details).

Proof of Concept

On some ERC-20 tokens, the transferFrom() will return false on failure instead of reverting a transaction.

The Wise Lending protocol implements the TransferHub::_callOptionalReturn() as a helper function for executing low-level calls. In case the transferFrom() returns false on failure, the _callOptionalReturn() will receive the returned parameters: success == true and returndata != bytes(0).

Then, the _callOptionalReturn() will decode the received returndata for the success status. If the transferFrom() returns false, the results will become false.

With the results == false, finally, the _callOptionalReturn() will return false to its function caller.

On the TransferHelper::_safeTransferFrom(), I noticed that the function does not evaluate the return value (i.e., unchecked return value bug) of the _callOptionalReturn(). Subsequently, the _safeTransferFrom() cannot detect the token transfer failure if the transferFrom() returns false.

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol
    function _callOptionalReturn(
        address token,
        bytes memory data
    )
        internal
        returns (bool call)
    {
        (
            bool success,
@1          bytes memory returndata //@audit -- On some tokens, the transferFrom() will return false instead of reverting a transaction
        ) = token.call(
            data
        );

@2      bool results = returndata.length == 0 || abi.decode(
@2          returndata, //@audit -- If the transferFrom() returns false, the results == false
@2          (bool)
@2      );

        if (success == false) {
            revert();
        }

@3      call = success
@3          && results // @audit -- If the results == false, the _callOptionalReturn() will return false
@3          && token.code.length > 0;
    }

    ...

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/TransferHelper.sol
    function _safeTransferFrom(
        address _token,
        address _from,
        address _to,
        uint256 _value
    )
        internal
    {
@4      _callOptionalReturn( //@audit -- The _safeTransferFrom() cannot detect the token transfer failure if the transferFrom() returns false instead of reverting a transaction due to the unchecked return value bug
            _token,
            abi.encodeWithSelector(
                IERC20.transferFrom.selector,
                _from,
                _to,
                _value
            )
        );
    }

• @1 -- On some tokens, the transferFrom() will return false instead of reverting a transaction: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L21
• @2 -- If the transferFrom() returns false, the results == false: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L26-L29
• @3 -- If the results == false, the _callOptionalReturn() will return false: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L35-L37
• @4 -- The _safeTransferFrom() cannot detect the token transfer failure if the transferFrom() returns false instead of reverting a transaction due to the unchecked return value bug: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/TransferHelper.sol#L42

Note: Please refer to the Impact section for a complete list of the functions directly invoking the vulnerable _safeTransferFrom().

The following briefly analyzes 2 of the 17 functions that would affect the exploitation as examples.

• Due to the unchecked return value bug, the WiseCore::_coreLiquidation() cannot be aware of the token transfer failure. Thus, a rogue liquidator can steal collateral from the target liquidable position without supplying any debt token (tokenToPayback).

• On the WiseLending::depositExactAmount(), a rogue depositor can increase their collateral without spending any token.

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol
    function _coreLiquidation(
        CoreLiquidationStruct memory _data
    )
        internal
        returns (uint256 receiveAmount)
    {
        ...

@5      _safeTransferFrom( //@audit -- Liquidator can steal collateral (_receiveToken) from the target liquidable position
@5          _data.tokenToPayback,
@5          _data.caller,
@5          address(this),
@5          _data.paybackAmount
@5      );

        ...
    }

    ...

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol
    function depositExactAmount(
        uint256 _nftId,
        address _poolToken,
        uint256 _amount
    )
        public
        syncPool(_poolToken)
        returns (uint256)
    {
        uint256 shareAmount = _handleDeposit(
            msg.sender,
            _nftId,
            _poolToken,
            _amount
        );

@6      _safeTransferFrom( //@audit -- Depositor can increase their collateral without supplying any token
@6          _poolToken,
@6          msg.sender,
@6          address(this),
@6          _amount
@6      );

        return shareAmount;
    }

• @5 -- Liquidator can steal collateral (_receiveToken) from the target liquidable position: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L674-L679
• @6 -- Depositor can increase their collateral without supplying any token: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol#L476-L481

Tools Used

Manual Review

Recommended Mitigation Steps

Improve the _safeTransferFrom() by checking the return value from the _callOptionalReturn() and reverting a transaction if it is false.

    function _safeTransferFrom(
        address _token,
        address _from,
        address _to,
        uint256 _value
    )
        internal
    {
-       _callOptionalReturn(
+       bool success = _callOptionalReturn(
            _token,
            abi.encodeWithSelector(
                IERC20.transferFrom.selector,
                _from,
                _to,
                _value
            )
        );

+       require(success, "Token transfer failed");
    }

Assessed type

Invalid Validation

[c4-pre-sort]

GalloDaSballo marked the issue as duplicate of #212

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[vm06007]

This then leads to some tokens unusable (like USDT for example) and this topic was already discussed severely during hats competition where I can send links to findings, so should be scraped in my opinion. Also sufficient checks are already done in _callOptionalReturn directly.

[thebrittfactor]

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.

[vm06007]

Additionally: WiseLending is not meant to be used with corrupted tokens that have unsupported transfer/transferFrom implementations.