Missing slippage check when depositing to YieldBox

[c4-bot-6]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarMintCommonModule.sol#L150 https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarAssetModule.sol#L95 https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarCollateralModule.sol#L83

Vulnerability details

Description

There are many functions of Magnetar's modules which attempt to deposit assets into YieldBox. For example, in the _depositYBLendSGL function of MagnetarMintCommonModule

function _depositYBLendSGL(
    IDepositData memory depositData,
    address singularityAddress,
    IYieldBox yieldBox_,
    address user,
    uint256 lendAmount
) internal returns (uint256 fraction) {
    if (singularityAddress != address(0)) {
        ...
        if (depositData.deposit) {
            depositData.amount = _extractTokens(user, sglAssetAddress, depositData.amount);

            sglAssetAddress.safeApprove(address(yieldBox_), depositData.amount);
            yieldBox_.depositAsset(sglAssetId, address(this), user, depositData.amount, 0);
        }

        // if `lendAmount` > 0:
        //      - add asset to SGL
        fraction = 0;
        if (lendAmount == 0 && depositData.deposit) {
            lendAmount = depositData.amount;
        }
        if (lendAmount > 0) {
            uint256 lendShare = yieldBox_.toShare(sglAssetId, lendAmount, false);
            fraction = ISingularity(singularityAddress).addAsset(user, user, false, lendShare);
        }

        _revertYieldBoxApproval(singularityAddress, yieldBox_);
    }
}

However, there is no slippage check for depositing assets into YieldBox, while the ratio between shares and the amount in YieldBox always fluctuates (see code snippet). It may result in the user receiving lower shares than expected from depositing.

Impact

When using Magnetar's functions, users will face risks from the fluctuation of YieldBox's shares and amounts, which may lead to unexpected losses when depositing to YieldBox.

Tools Used

Manual review

Recommended Mitigation Steps

Slippage checks should be added whenever depositing to YieldBox

Assessed type

Other

[c4-sponsor]

cryptotechmaker (sponsor) disputed

[cryptotechmaker]

I don't understand what's the issue here. When something is deposited to YB it's deposited either in amount or shares.

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt marked the issue as selected for report

[carrotsmuggler]

This issue is invalid. It shows no attack path or exploit scenario.

The report states It may result in the user receiving lower shares than expected from depositing. Receiving more or less shares does not constitute as loss of funds. If the user is unable to recover the amount of funds they had deposited by burning their received shares, THEN the user has lost funds. But this report does not describe any such scenario.

The report has no POC to show any loss, does not describe any loss of funds for the user. Even simple erc4626 vaults can have unpredictable share ratios, however that is not an issue. It is only an issue if these share ratios are manipulatable in some way, which is not demonstrated here.

There is no factual argument given here showing any loss, and the yieldbox works exactly as intended.

[dmvt]

Agreed. This is a bad click on my part which I'm going to chalk up to the tooling issues experienced in this contest.

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient proof