Wise Lending - Jorgect's results

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/TransferHelper.sol#L20

Vulnerability details

Some implementations of transfer and transferFrom functions could return ‘false’ on failure instead of reverting. It is safe to wrap such calls into require() statements to these failures.

There is also another important check for low level external calls, if the address doesn't not have bytecode the call success.

This is been doing for the contract in the _callOptionalReturn function:

file:/contracts/TransferHub/TransferHelper.sol

function _callOptionalReturn(
        address token,
        bytes memory data
    )
        internal
        returns (bool call)
    {
        (
            bool success,
            bytes memory returndata
        ) = token.call(
            data
        );

        bool results = returndata.length == 0 || abi.decode(
            returndata,
            (bool)
        );

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

        call = success
            && results
            && token.code.length > 0;
    }

[Link]

The problem is that the result variable is not been checked if it was false.

Impact

The safeTransfer function is not checking if the transfer and transferFrom return ‘false’ on failure instead of reverting. so if a token return false the executing of the logic is continuing but the transfer was never made it.

Proof of Concept

contract TransferHelper is CallOptionalReturn {
    /**
     * @dev
     * Allows to execute safe transfer for a token
     */
    function _safeTransfer(address _token, address _to, uint256 _value) internal {
        _callOptionalReturn(_token, abi.encodeWithSelector(IERC20.transfer.selector, _to, _value));
    }

    /**
     * @dev
     * Allows to execute safe transferFrom for a token
     */
    function _safeTransferFrom(address _token, address _from, address _to, uint256 _value) internal {
        _callOptionalReturn( 
        _token, abi.encodeWithSelector(IERC20.transferFrom.selector, _from, _to, _value));
    }
}

As you can see the safeTransfer function is not checking whether the return value of the _callOptionalReturn is true or false, in consequence is not reverting on false

Tools Used

Manual review

Recommended Mitigation Steps

Consider implement well known and tested librarys for safe erc20 calling, The most used are SafeERC20.sol or SafeTransferLib.sol.

Oher solution is check for false and rever in this cases.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurityHelper.sol#L895

Vulnerability details

If the total borrowed for a borrower is more than the total collateral plus some threshold they can get liquidated, that is common in all lending platforms.

When a position is ready to be liquidate, there is a important check in the wiseSecurity contract:

 function liquidatePartiallyFromTokens(
        uint256 _nftId,
        uint256 _nftIdLiquidator,
        address _paybackToken,
        address _receiveToken,
        uint256 _shareAmountToPay
    )
        external
        syncPool(_paybackToken)
        syncPool(_receiveToken)
        returns (uint256)
    {
        CoreLiquidationStruct memory data;

        data.nftId = _nftId;
        data.nftIdLiquidator = _nftIdLiquidator;

        data.caller = msg.sender; 

        data.tokenToPayback = _paybackToken;
        data.tokenToRecieve = _receiveToken;
        data.shareAmountToPay = _shareAmountToPay;

        data.maxFeeETH = WISE_SECURITY.maxFeeETH();
        data.baseRewardLiquidation = WISE_SECURITY.baseRewardLiquidation();

        (
            data.lendTokens,
            data.borrowTokens
        ) = _prepareAssociatedTokens(
            _nftId,
            _receiveToken,
            _paybackToken
        );

        data.paybackAmount = paybackAmount(
            _paybackToken,
            _shareAmountToPay
        );

        _checkPositionLocked(
            _nftId,
            msg.sender
        ); 

        _checkLiquidatorNft(
            _nftId,
            _nftIdLiquidator 
        );

        WISE_SECURITY.checksLiquidation(  <------
            _nftId,
            _paybackToken,
            _shareAmountToPay
        );

        return _coreLiquidation(
            data
        );
    }

[Link]

function checksLiquidation(
        uint256 _nftIdLiquidate,
        address _tokenToPayback,
        uint256 _shareAmountToPay
    )
        external
        view
    {
        (
            uint256 weightedCollateralETH, 
            uint256 unweightedCollateralETH

        ) = overallETHCollateralsBoth(
            _nftIdLiquidate
        );

        uint256 borrowETHTotal = overallETHBorrowHeartbeat(
            _nftIdLiquidate
        );

        canLiquidate(
            borrowETHTotal,
            weightedCollateralETH
        ); 

        checkMaxShares(   <------
            _nftIdLiquidate,
            _tokenToPayback,
            borrowETHTotal,
            unweightedCollateralETH,
            _shareAmountToPay
        );
    }

[Link]

function checkMaxShares(
        uint256 _nftId,
        address _tokenToPayback,
        uint256 _borrowETHTotal,
        uint256 _unweightedCollateralETH,
        uint256 _shareAmountToPay
    )
        public
        view
    {
        uint256 totalSharesUser = WISE_LENDING.getPositionBorrowShares(
            _nftId,
            _tokenToPayback
        );

        uint256 maxShares = checkBadDebtThreshold(_borrowETHTotal, _unweightedCollateralETH)
            ? totalSharesUser
            : totalSharesUser * MAX_LIQUIDATION_50 / PRECISION_FACTOR_E18;

        if (_shareAmountToPay <= maxShares) {  <-------
            return;
        }

        revert TooManyShares();
    }

[Link]

The problem is that if a liquidator try to paid the whole position, the borrower can just payback 1 share just before the transaction of the liquidation making the transaction revert because _shareAmountToPay will be major than maxShares:

if (_shareAmountToPay <= maxShares) {  <-------
            return;
        }

        revert TooManyShares();

[Link]

Impact

User can avoid get liquidate if liquidator try to liquidate the whole amount, this can represent serious problems in the protocol due is breaking a core functionality, in the worse case can generate bad debt to the protocol.

Proof of Concept

 function paybackExactShares(
        uint256 _nftId,
        address _poolToken,
        uint256 _shares
    )
        external
        syncPool(_poolToken)
        returns (uint256)
    {
        uint256 repaymentAmount = paybackAmount( 
            _poolToken,
            _shares
        );

        _validateNonZero(
            repaymentAmount
        );

        _handlePayback(
            msg.sender,
            _nftId,
            _poolToken,
            repaymentAmount,
            _shares
        );

        _safeTransferFrom(
            _poolToken,
            msg.sender,
            address(this),
            repaymentAmount
        );

        return repaymentAmount;
    }

[Link]

As you can see there is not check for health or bad deb on the payback functions, which means that borrowers can pay whatever amount that he want. So borrowers can pay 1 share to make revert this transaction if the liquidator try to liquidate the whole amount:

if (_shareAmountToPay <= maxShares) {  <-------
            return;
        }

        revert TooManyShares();

[Link]

Tools Used

Manual review

Recommended Mitigation Steps

instead of revert if _shareAmountToPay > maxShares refund the different between _shareAmountToPay - maxShares .

Assessed type

Other