Missing conversion of deposited assets to borrow parts before repayment in MagnetarAssetModule

[c4-bot-9]

Lines of code

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

Vulnerability details

Description

In the depositRepayAndRemoveCollateralFromMarket function of MagnetarAssetModule contract, there is an option to deposit assets into the YieldBox before repaying user's loan in market. However, it doesn't convert the deposited asset amount to the borrow part for repayment. This results in the risk that the deposited asset amount will mismatch with the part of the borrow that will be repaid.

 // @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);
}

After depositing, this function doesn't check if data.repayAmount aligns with the deposited amount. Note that the parameter of the repay function is the borrow part of the loan which will be repaid. The ratio between the borrow amount and the borrow part in a market is not stable, so there is no way to ensure that data.repayAmount is the exact borrow part to repay using data.depositAmount asset tokens.

Therefore, data.repayAmount can be lower than the exact borrow part that can be repaid from the deposited asset amount, resulting in users losing assets because of depositing more than repaying. The excess assets will be stuck in the Magnetar contract. Otherwise, if data.repayAmount is higher than the maximum borrow part can be repaid, this function will revert due to insufficient assets for repaying.

Impact

User may lose their funds or experience DOS attack when using depositRepayAndRemoveCollateralFromMarket of Magnetar.

Tools Used

Manual review

Recommended Mitigation Steps

In depositRepayAndRemoveCollateralFromMarket function, if data.depositAmount > 0, it should recalculate repayAmount by convert from received shares to borrow part

// @dev performs a repay operation for the specified market
if (data.repayAmount > 0) {
    if (data.depositAmount > 0) {
        data.repayAmount = magnetarHelper.getBorrowPartForAmount(_market, data.depositAmount);
    }
    ...
}

Assessed type

Other

[c4-sponsor]

cryptotechmaker (sponsor) confirmed

[c4-judge]

dmvt marked the issue as primary issue

[cryptotechmaker]

https://github.com/Tapioca-DAO/tapioca-periph/pull/211/commits/bad6826e035c1c060e69b81a2967e0627c19a786

[c4-sponsor]

cryptotechmaker marked the issue as disagree with severity

[cryptotechmaker]

I don't think this is an issue @c4-judge (@GalloDaSballo saw you mentioned it in another place)

depositAmount and repayAmount are 2 independent parameters. One can deposit 10 because he has another 90 already deposited and repays 100.

The repayAmount should be converted to part before calling the method by using MagnetarHelper. This or we manually convert it to part in the method itself. What do you think?

https://github.com/Tapioca-DAO/tapioca-periph/pull/211/commits/bad6826e035c1c060e69b81a2967e0627c19a786

[c4-judge]

dmvt marked the issue as selected for report

[dmvt]

On review I agree. As @GalloDaSballo suggested, I don't think this is actually an issue. If the warden would like to provide a written test POC, I'd reinstate. As is I'll be downgrading to low.

[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