Tokens deposited will be stuck in `depositRepayAndRemoveCollateralFromMarket` function if user attempts to execute the deposit step but skip the repay step

[c4-bot-9]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarAssetModule.sol#L90-L106

Vulnerability details

Description

"In the MagnetarAssetModule contract, depositRepayAndRemoveCollateralFromMarket is used to deposit assets into YieldBox, repay on a market, and remove collateral from a market. All actions in this function are intended to be optional, allowing the user to execute only some of these steps and skip others. Here is the natspec of this function:

/// =====================
/// Public
/// =====================

/**
 * @notice helper for deposit asset to YieldBox, repay on a market, remove collateral and withdraw
 * @dev all steps are optional:
 *         - if `depositAmount` is 0, the deposit to YieldBox step is skipped
 *         - if `repayAmount` is 0, the repay step is skipped
 *         - if `collateralAmount` is 0, the add collateral step is skipped
 *
 * @param data.market the SGL/BigBang market
 * @param data.user the user to perform the action for
 * @param data.depositAmount the amount to deposit to YieldBox
 * @param data.repayAmount the amount to repay to the market
 * @param data.collateralAmount the amount to withdraw from the market
 * @param data.withdrawCollateralParams withdraw specific params
 */
function depositRepayAndRemoveCollateralFromMarket(DepositRepayAndRemoveCollateralFromMarketData memory data)

However, when a user attempts to execute the deposit step, it always deposits for this contract (the receiver address is address(this)). It means that the contract always receives the YieldBox's shares from the deposit step, whether other steps are skipped or not. Therefore, if the repay step is skipped (data.repayAmount == 0), these shares will remain unused and still be stuck in this contract.

// @dev deposit to YieldBox
if (data.depositAmount > 0) {
    data.depositAmount = _extractTokens(msg.sender, assetAddress, data.depositAmount);
    IERC20(assetAddress).approve(address(_yieldBox), 0);
    IERC20(assetAddress).approve(address(_yieldBox), data.depositAmount);
    _yieldBox.depositAsset(assetId, address(this), address(this), data.depositAmount, 0);
}

// @dev performs a repay operation for the specified market
if (data.repayAmount > 0) {
    _setApprovalForYieldBox(data.market, _yieldBox);
    (Module[] memory modules, bytes[] memory calls) = IMarketHelper(data.marketHelper).repay(
        data.depositAmount > 0 ? address(this) : data.user, data.user, false, data.repayAmount
    );
    _market.execute(modules, calls, true);
    _revertYieldBoxApproval(data.market, _yieldBox);
}

Impact

Users will lose funds if they attempt to execute the deposit step but skip the repay step in depositRepayAndRemoveCollateralFromMarket function

Tools Used

Manual review

Recommended Mitigation Steps

The receiver of depositing should be optional, as the following:

if (data.depositAmount > 0) {
    ...
    _yieldBox.depositAsset(assetId, address(this), data.repayAmount > 0 ? address(this) : data.user, data.depositAmount, 0);
}

Assessed type

Other

[c4-sponsor]

cryptotechmaker marked the issue as disagree with severity

[cryptotechmaker]

Medium. The method is not intended to be used like that. Deposit step is optional but mandatory in case of repayment. However is worth to be fixed as you said.

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

0xRektora (sponsor) confirmed

[cryptotechmaker]

Fixed by https://github.com/Tapioca-DAO/tapioca-periph/pull/207

[c4-judge]

dmvt marked the issue as selected for report

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-a

[c4-judge]

dmvt marked the issue as not selected for report