calling transferToken Function Disrupts Repay Functionality

Summary

This report identifies a critical issue stemming from the improper implementation of the transferToken function. This flaw has the potential to disrupt the entire functionality of the repay function, rendering it impossible for users to repay their loans or for lenders to access the repay function during emergency scenarios.

Vulnerability Detail

The repay function relies on the transferToken function to transfer assets and fees to different users, including borrowers, lenders, and liquidators. However, this implementation falls short of expectations. Consider the following snippet from the repay function

function repay(
        RepayParams calldata params,
        uint256 deadline
    ) external nonReentrant checkDeadline(deadline) {
       ...
        // Check if it's an emergency repayment
        if (params.isEmergency) {
            (collateralBalance >= 0).revertError(ErrLib.ErrorCode.FORBIDDEN);
            (
                uint256 removedAmt,
                uint256 feesAmt,
                bool completeRepayment
            ) = _calculateEmergencyLoanClosure(
                    zeroForSaleToken,
                    params.borrowingKey,
                    borrowing.feesOwed,
                    borrowing.borrowedAmount
                );
            (removedAmt == 0).revertError(ErrLib.ErrorCode.LIQUIDITY_IS_ZERO);
            // prevent overspent
            // Subtract the removed amount and fees from borrowedAmount and feesOwed
            borrowing.borrowedAmount -= removedAmt;
            borrowing.feesOwed -= feesAmt;
            feesAmt /= Constants.COLLATERAL_BALANCE_PRECISION;
            // Deduct the removed amount from totalBorrowed
            holdTokenRateInfo.totalBorrowed -= removedAmt;
            // If loansInfoLength is 0, remove the borrowing key from storage and get the liquidation bonus
            if (completeRepayment) {
                LoanInfo[] memory empty;
                _removeKeysAndClearStorage(borrowing.borrower, params.borrowingKey, empty);
                feesAmt += liquidationBonus;
            } else {
                BorrowingInfo storage borrowingStorage = borrowingsInfo[params.borrowingKey];
                borrowingStorage.dailyRateCollateralBalance = 0;
                borrowingStorage.feesOwed = borrowing.feesOwed;
                borrowingStorage.borrowedAmount = borrowing.borrowedAmount;
                // Calculate the updated accLoanRatePerSeconds
                borrowingStorage.accLoanRatePerSeconds =
                    holdTokenRateInfo.accLoanRatePerSeconds -
                    FullMath.mulDiv(
                        uint256(-collateralBalance),
                        Constants.BP,
                        borrowing.borrowedAmount // new amount
                    );
            }
            // Transfer removedAmt + feesAmt to msg.sender and emit EmergencyLoanClosure event
            //@audit the transfer here will revert because of the onlyowner modifier
            Vault(VAULT_ADDRESS).transferToken(
                borrowing.holdToken,
                msg.sender,
                removedAmt + feesAmt
            );
            emit EmergencyLoanClosure(borrowing.borrower, msg.sender, params.borrowingKey);
        } else {
            // Deduct borrowedAmount from totalBorrowed
            holdTokenRateInfo.totalBorrowed -= borrowing.borrowedAmount;

            // Transfer the borrowed amount and liquidation bonus from the VAULT to this contract
            //@audit
            Vault(VAULT_ADDRESS).transferToken(
                borrowing.holdToken,
                address(this),
                borrowing.borrowedAmount + liquidationBonus
            );
            // Restore liquidity using the borrowed amount and pay a daily rate fee
            LoanInfo[] memory loans = loansInfo[params.borrowingKey];
            _maxApproveIfNecessary(
                borrowing.holdToken,
                address(underlyingPositionManager),
                type(uint128).max
            );
            _maxApproveIfNecessary(
                borrowing.saleToken,
                address(underlyingPositionManager),
                type(uint128).max
            );

           ...
        }
    }

The code shows that the transferToken function is called twice. However, the entire repay process is bound to fail because the transferToken function is only callable by the owner, enforced by the onlyOwner modifier:

 function transferToken(address _token, address _to, uint256 _amount) external onlyOwner {
        if (_amount > 0) {
            IERC20(_token).safeTransfer(_to, _amount);
        }
    }

Due to this constraint, all calls to the repay function will fail, effectively preventing anyone from repaying their loans or liquidating their positions.

Impact

The use of the onlyOwner modifier in the transferToken function has a severe impact as it disrupts the repay functionality. This can lead to a breakdown in the protocol's core operations, affecting both borrowers and lenders.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L621-L624 https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L632-L636

Tool used

Manual Review

Recommendation

recommend removing the onlyOwner modifier from the transferToken function. Instead, create a custom modifier that allows calls from the protocol contracts and the owner only, maintaining security without disrupting the repay functionality

sherlock-audit / 2023-10-real-wagmi-judging

0xkazim - calling transferToken Function Disrupts Repay Functionality #171