pkqs90 - MBasisRedemptionVaultWithSwapper does not update mBasis daily limit or allowance when conducting mBasis->mTBill swap.

[sherlock-admin4]

pkqs90

Medium

MBasisRedemptionVaultWithSwapper does not update mBasis daily limit or allowance when conducting mBasis->mTBill swap.

Summary

MBasisRedemptionVaultWithSwapper does not update mBasis daily limit or allowance when conducting mBasis->mTBill swap.

Vulnerability Detail

When users are redeeming from MBasisRedemptionVaultWithSwapper, if there is not enough tokenOut balance, it will first conduct a mBasis->mTBill swap and try to redeem from the mTBill redemption vault.

The issue here is, when this happens, the daily limit and token allowance is only deducted from the mTBill redemption vault, and not the original MBasisRedemptionVaultWithSwapper contract.

    function redeemInstant(
        address tokenOut,
        uint256 amountMTokenIn,
        uint256 minReceiveAmount
    )
        external
        override(IRedemptionVault, RedemptionVault)
        whenFnNotPaused(this.redeemInstant.selector)
        onlyGreenlisted(msg.sender)
        onlyNotBlacklisted(msg.sender)
        onlyNotSanctioned(msg.sender)
    {
        ...
        uint256 contractTokenOutBalance = IERC20(tokenOutCopy).balanceOf(
            address(this)
        );

        if (
            contractTokenOutBalance >=
            amountTokenOutWithoutFee.convertFromBase18(tokenDecimals)
        ) {
@>          _requireAndUpdateLimit(amountMTokenInCopy);
            _requireAndUpdateAllowance(tokenOutCopy, amountTokenOut);

            mToken.burn(user, amountMTokenWithoutFee);
        } else {
            uint256 mTbillAmount = _swapMBasisToMToken(amountMTokenWithoutFee);

            IERC20(mTbillRedemptionVault.mToken()).safeIncreaseAllowance(
                address(mTbillRedemptionVault),
                mTbillAmount
            );

            mTbillRedemptionVault.redeemInstant(
                tokenOutCopy,
                mTbillAmount,
                minReceiveAmountCopy
            );

            uint256 contractTokenOutBalanceAfterRedeem = IERC20(tokenOutCopy)
                .balanceOf(address(this));
            amountTokenOutWithoutFee = (contractTokenOutBalanceAfterRedeem -
                contractTokenOutBalance).convertToBase18(tokenDecimals);
        }

        _tokenTransferToUser(
            tokenOutCopy,
            user,
            amountTokenOutWithoutFee,
            tokenDecimals
        );

        emit RedeemInstant(
            user,
            tokenOutCopy,
            amountMTokenInCopy,
            feeAmount,
            amountTokenOutWithoutFee
        );
    }

Impact

mBasis daily limit and token allowance does not deduct as expected.

Code Snippet

• https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts/contracts/mBasis/MBasisRedemptionVaultWithSwapper.sol#L146-L164

Tool used

Manual Review

Recommendation

Also update mBasis daily limit and allowance when conducting mBasis->mTBill swap for MBasisRedemptionVaultWithSwapper.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

merlinboii commented:

Incorrect assumption and Intended design. For the swapper flow: the mToken limit is tracked in the vault that PROVIDE THE REAL REDEMPTION so for this case their is no token operate with the main vault but it operate redeem inside the mTBILL vault. The issue suggestion will prevent the user from redeemInstant with enough tokenOut as the limit is over updated by the amount of token that does not operate inside them.

[piken]

Escalate
Invalid issue.

instantDailyLimit is used to limit the maximum amount of mToken that can be redeemed per day, specifically for MBasisRedemptionVaultWithSwapper, It means the maximum amount of mBASIS that can be burned in exchange for tokenOut within a single day.
When MBasisRedemptionVaultWithSwapper doesn't have enough tokenOut for mBASIS redeeming, all mBASIS will be swapped for mBILL, then the received mBILL will be burned instantly for tokenOut.

As we can see, in the above case no mBASIS is burned and all tokenOut comes from mTbillRedemptionVault. Therefore it doesn't make sense to update instantDailyLimit and token allowance of MBasisRedemptionVaultWithSwapper.

[sherlock-admin3]

Escalate
Invalid issue.

instantDailyLimit is used to limit the maximum amount of mToken that can be redeemed per day, specifically for MBasisRedemptionVaultWithSwapper, It means the maximum amount of mBASIS that can be burned in exchange for tokenOut within a single day.
When MBasisRedemptionVaultWithSwapper doesn't have enough tokenOut for mBASIS redeeming, all mBASIS will be swapped for mBILL, then the received mBILL will be burned instantly for tokenOut.

As we can see, in the above case no mBASIS is burned and all tokenOut comes from mTbillRedemptionVault. Therefore it doesn't make sense to update instantDailyLimit and token allowance of MBasisRedemptionVaultWithSwapper.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
RedDuck-Software/midas-contracts#65

[WangSecurity]

@pkqs90 could I get your input on this?

[pkqs90]

@WangSecurity My response to the escalation:

1. The use case: User redeems X MBasis by MBasisRedemptionVaultWithSwapper.redeemInstant, and receives tokenOut in return. It doesn't matter how the swap is done, X should be deducted from instantDailyLimit. instantDailyLimit is used to limit the amount of MBasis user can redeem. Why only reduce the daily limit for the enough tokenOut branch? It doesn't make any sense.
2. The sponsor already applied the fix (I know this does not prove issue validity, but this proves the escalation does not correctly understand how instantDailyLimit should be used)

[WangSecurity]

Thank you for the response, I agree that indeed instantDailyLimit should be decreased in that case as well, even though the tokens come from mTBILL redemption vault.

Hence, planning to reject the escalation and leave the issue as it is.

[IronsideSec]

@WangSecurity While, the instantDailyLimit should be decreased in both cases. Correct. But this doesn't equate to make it a valid medium. Seems a low severity to me. There is no loss of funds, no DOS to anyone (user/protocol).

1. Check How to identify a medium issue : This issue doesn't cause any loss of funds. Also doesn't break the core functionality. The fix is a design improvement to the protcol. Hence a low severity.

2. Check List of Issue categories that are not considered valid : Point 8 says User experience and design improvement issues: Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid

Hence, isuue is of low severity.

[WangSecurity]

@IronsideSec thank you very much. While I see how it breaks the functionality of the protocol, i.e. the daily limit is not decreased as it should be, there is indeed to loss of funds. But, to be medium severity issue it shouldn't lead to a loss of funds necessarily:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds

And I think @pkqs90 could help us how updating the daily limit on instant redeems is important for the protocol?

[IronsideSec]

@WangSecurity @pkqs90

But, to be medium severity issue it shouldn't lead to a loss of funds necessarily:

True, but below statement is wrong

While I see how it breaks the functionality of the protocol, i.e. the daily limit is not decreased as it should be

breaks the functionality of the protocol means something like, protcol unable to mint anymore, or unable to update the fees/some address kind of things that can be categorized as DOS (breaking protocol function).

But here daily limit is to limit redemption amounts for users.Here's how protcol can still limit the redemption in mBasisTo_mToken route
To swap mBasis into mToken, it sends mBasis to liquidityProvider address and liquidityProvider sends back mToken and use those mTokens to redeem instant on mTbillRedemptionVault.redeemInstant call.

So, liquidityProvider can limit the amount of redemption by having only x amount of mToken balance per day, if balance is over, then next availibiity is only on next day. And they can even reduce its balance, if some daily limit from direct mBasis is redeemption is consumed. So, daily redemption is limited by this way. The fix to current issue is just an easier way to manage the daily limit, but admins cans till do it as I explained above.

So, not updating the daily limit on mBasis to mToken swap action doesn't break any core functionality of the protocol.

[pkqs90]

@IronsideSec thank you very much. While I see how it breaks the functionality of the protocol, i.e. the daily limit is not decreased as it should be, there is indeed to loss of funds. But, to be medium severity issue it shouldn't lead to a loss of funds necessarily:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds

And I think @pkqs90 could help us how updating the daily limit on instant redeems is important for the protocol?

Users can instant redeem more mBasis than the daily limit config set by admin. This is a direct violation of the specs. Same mechanism has been implemented for all redemption vaults except MBasisRedemptionVaultWithSwapper.

Limits / Caps

• TOKEN daily instant redeem caps

Similarly to the instant minting cap, this is defined in TOKEN

As for why the limit is required, I suggest asking the protocol team. But IIRC, violating the specs is enough for a valid medium.

Also my response to @IronsideSec on the "bypass mechanism" to feed liquidityProvider with X amount of mToken per day: Say the MBasisRedemptionVaultWithSwapper has a daily limit config of Y, and liquidityProvider has X amount of mToken (consider the simple case where we have only one tokenOut, and mToken:mBasis = 1:1). There are two cases:

1. MBasisRedemptionVaultWithSwapper has enough tokenOut. This is good, since the daily limit Y would be respected.
2. MBasisRedemptionVaultWithSwapper has some tokenOut, but not enough to fulfill all Y redemption (say Y1). The limit would be Y1+X.

For the second case, it already violates the spec, not to mention the complicated scenario where we have multiple tokenOuts, and that mToken:mBasis is a fluctuating rate. I don't think the bypass mechanism is realistic.

[IronsideSec]

But IIRC, violating the specs is enough for a valid medium.

For the second case, it already violates the spec, not to mention the complicated scenario where we have multiple tokenOuts, and that mToken:mBasis is a fluctuating rate. I don't think the bypass mechanism is realistic.

Conclusion from me :

• Saying not updating limit on swap action violates the spec is a wrong statement @pkqs90 . Read the title Admin can adjust Global Parameters. It says admin should be able to adjust the redemption limit. The admin has the power to adjust in all the cases you mentioned. Admin can set mbasis direct redemption without swap, and if mbasis to mTbill swap is done, then admin can limit with setting limit on mTbill redemption limit. And there's another way explained in previous comment mechanism. So saying violation of spec, so low severity should be a medium is wrong. There's no violation at all as explained above.

I don't think the bypass mechanism is realistic.

its easy to have a backend bot to change mTbill balance of liquidity provider to adjust limits accordingly. Its realistic.
The issue just recommends to update the mbasis limit also not only mToken's, when mBasis to mTbill swap redemption.
Still your explaination doesn't say how it breaks the core functionality of the protocol.

[0xklapouchy]

@WangSecurity

It is worth mentioning that Midas protocol asked in the Readme to report all the issues that break the spec.

Here:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes. The expected functionality and invariants are explained in the following doc: https://ludicrous-rate-748.notion.site/8060186191934380800b669406f4d83c?v=35634cda6b084e2191b83f295433efdf

And here:

Additional audit information.

Midas strongly encourages auditors to read the documents listed below to understand the design and behavior expectations (from the specification).

Product Requirements (Specifications). Please note that discrepancies between the spec and the code can be reported as issues
https://ludicrous-rate-748.notion.site/8060186191934380800b669406f4d83c?v=35634cda6b084e2191b83f295433efdf

[0xklapouchy]

@IronsideSec

Saying not updating limit on swap action violates the spec is a wrong statement @pkqs90 . Read the title Admin can adjust Global Parameters. It says admin should be able to adjust the redemption limit. The admin has the power to adjust in all the cases you mentioned. Admin can set mbasis direct redemption without swap, and if mbasis to mTbill swap is done, then admin can limit with setting limit on mTbill redemption limit. And there's another way explained in #110 (comment). So saying violation of spec, so low severity should be a medium is wrong. There's no violation at all as explained above.

This is an invalid statement; suggesting that limiting mBASIS redemption by updating the mTBILL redemption limit is more than just incorrect. The mTBILL redemption vault operates independently from mBASIS redemption.

So, liquidityProvider can limit the amount of redemption by having only x amount of mToken balance per day, if balance is over, then next availibiity is only on next day.

This is also an invalid statement; it can be seen as a global redemption limit, not a daily one, and can be easily broken.

Let's now break down both suggested ways to limit mBASIS redemption proposed by @IronsideSec:

The user has 550 mBASIS to redeem.

1. mBASIS daily redemption limit is set to 500. The contract holds 470 USDC. For the sake of example, the rate is 1:1 mBASIS/USDC.
2. The liquidity provider has only 100 mTBILLs. For this example, the rate is 1:1 mBASIS/mTBILL.
3. The mTBILL redemption vault was set to 200 mTBILLs (proposed limitation), who cares about outher users that want to redeem mTBILLS directly from the mTBILL redeemption vault.
4. The user creates two transactions and pushes them to the private mempool one by one (a sophisticated bot proposed by @IronsideSec can't catch that).
   • The first transaction is for 470 mBASIS redemption.
   • The second transaction is for 80 mBASIS redemption.

With the current code and the proposed solution, both transactions still pass, breaking the mBASIS redemption limit of 500.

And the core functionality that is broken is the breaking the daily mBASIS token redemption limit, in highly regulated RAW protocol it is more than needed.

[IronsideSec]

Replying to @0xklapouchy

Please note that discrepancies between the spec and the code can be reported

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope?

It is worth mentioning that Midas protocol asked in the Readme to report all the issues that break the spec.

I explained here why its not breaking the spec at all. Spec says admin can adjust the global parameters (i.e can change redemption limits). Admin can still change the limit. Hence, not breaking any spec. Your issue is suggesting to modify both mbasis and mToken limits and not only mToken's, when mbasis -> mTbill swap redemption is done. How is this breaking functionality ? Not violating the spec at all.

Even if as explained points in the just above comment, invalidating the mechanism bot explained here doesn't make this issue breaking core functionality

This is also an invalid statement; it can be seen as a global redemption limit, not a daily one, and can be easily broken.

Doesn't matter. Admin can stall the balance of liquidity provider to zero to completely block the mBasis to mTbill redemption for however long. So your statement that its a daily limit, not global parameter is wrong . Below statement is still valid.

So, liquidityProvider can limit the amount of redemption by having only x amount of mToken balance per day, if balance is over, then next availibiity is only on next day.

And the core functionality that is broken is the breaking the daily mBASIS token redemption limit, in highly regulated RAW protocol it is more than needed.

breaking the daily mBASIS token redemption limit is not a core functionality in this protocol. Maybe, unable redeem/mint eventhough they should be able to. That is the core functionality breakage.

Even if you claim its a core functionality, admin has lots of ways to limit the mBasis to mTbill, (mTbill balance of liquidity provider, redemption limit on mTbill itself). The fix of this issue just makes things easier. Its a low impact @WangSecurity

[pkqs90]

I fully agree with @0xklapouchy's point above.

My response to @IronsideSec's point "its easy to have a backend bot to change mTbill balance of liquidity provider to adjust limits accordingly. Its realistic.":

1. There is no backend bot mentioned anywhere in the specs or docs. I'm sure the sponsor team has no plan implementing such a complicated feature just to limit the mBasis redemption.
2. Taking a step back, even if such bot should be implemented, it is not realistic at all. @0xklapouchy already gave an scenario where limiting LiquidityProvider balance won't work. Also, the bot would need to maintain the mTBILL balance of LiquidityProvider very carefully to limit total daily mBasis redemption. It also needs to consider the fluctuation of mBasis:USDC and mBasis:mTBILL rate.

My response to @IronsideSec's point "Even if you claim its a core functionality, admin has lots of ways to limit the mBasis to mTbill, (mTbill balance of liquidity provider, redemption limit on mTbill itself)":

1. None of the "ways" you mentioned works. Not to mention they are all mechanisms to bypass the real issue here.

[IronsideSec]

@pkqs90 @0xklapouchy You guys are invalidating the bot mechanism and the other methods to limit swap redemption

But,

1. Did not still prove why not updating mBasis daily limit on mBasis->mTBill swap a core funtionality breaking. Except a plain below statement. token redemption limit is not a core functionality. Users being unable to deposit, or temporary locking of system., these are core functionality breaking. Breaching the mBasis redemption limit via mBasis to mTbill redemption is not a core functionality. Users can still redeem/deposit with no DOS without this issue's fix.

And the core functionality that is broken is the breaking the daily mBASIS token redemption limit, in highly regulated RAW protocol it is more than needed.

2. Another claim that it was said violating the spec by both of you here and here, which is false as explained here. But it wasn't countered, and kept on replying to that bot management..., If it violated the spec, you would've listed on issue itself like pkqs90 - Discrepancy between spec and code: Vault admin cannot update tokensReceiver. #111 . Both this issue and dup eeyore - Incorrect validation of the daily redemption limit in the MBasisRedemptionVaultWithSwapper contract. #93 doesn't mention spec/readme violation. Please read this and this again

[WangSecurity]

Firstly, to clarify, the medium doesn't say "core functionality", it says "core contract functionality", i.e. the functionality of the core contract. I get that it's a bit confusing, therefore, wanted to clarify.

Secondly, I agree with @pkqs90 that bots were not mentioned anywhere and we shouldn't assume they will be used. In this case, it's a hypothetical way to mitigate the issue, which has also been proved to be not working.

Thirdly, about breaking the spec. Let's look at the entire section about caps:

Limits / Caps

• TOKEN daily instant mint caps. Defined in TOKEN
  Example: Only 300k mTBILL can be minted instantly per day via this smartcontract minter. If the limit has been reached for a given day, users can make a standard subscription request (or wait for the day after)
• TOKEN daily instant redeem caps
  Similarly to the instant minting cap, this is defined in TOKEN

As we can see there's an example where the cap is set and reaching this limit shouldn't allow more instant mints. Similarly to it, if the limit is reached, instant redemptions shouldn't be allowed. Here, in this issue, this limit can be reached but the redemptions will continue, because it doesn't track the redemptions involving mBasis correctly. Therefore, I agree the spec is broken here.

Hence, planning to reject escalation and leave the issue as it is.

Note: mentioning the broken spec in the report is not required

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• piken: rejected