pkqs90 - RedemptionVaultWIthBUIDL.sol#redeemInstant will always DoS due to incorrect contract call.

[sherlock-admin2]

pkqs90

High

RedemptionVaultWIthBUIDL.sol#redeemInstant will always DoS due to incorrect contract call.

Summary

RedemptionVaultWIthBUIDL.sol#redeemInstant will always DoS due to incorrect contract call.

Vulnerability Detail

In RedemptionVaultWIthBUIDL contract, it uses the BUIDL contracts to redeem BUIDL tokens to USDC tokens, and users are expected to always receive USDC tokens.

On Ethereum, the buidlRedemption is this contract https://etherscan.io/address/0x31D3F59Ad4aAC0eeE2247c65EBE8Bf6E9E470a53#readContract. The buidlRedemption.liquidity() is this contract https://etherscan.io/address/0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48, which is USDC.

The issue here is, the code wrongly assumes buidlRedemption.liquidity() is the liquiditySource, when it should be the USDC token. This will always lead to DoS for the buidlLiquiditySource.token() call in redeemInstant since USDC does not support .token() call.

    function initialize(
        address _ac,
        MTokenInitParams calldata _mTokenInitParams,
        ReceiversInitParams calldata _receiversInitParams,
        InstantInitParams calldata _instantInitParams,
        address _sanctionsList,
        uint256 _variationTolerance,
        uint256 _minAmount,
        FiatRedeptionInitParams calldata _fiatRedemptionInitParams,
        address _requestRedeemer,
        address _buidlRedemption
    ) external initializer {
        __RedemptionVault_init(
            _ac,
            _mTokenInitParams,
            _receiversInitParams,
            _instantInitParams,
            _sanctionsList,
            _variationTolerance,
            _minAmount,
            _fiatRedemptionInitParams,
            _requestRedeemer
        );
        _validateAddress(_buidlRedemption, false);
@>      buidlRedemption = IRedemption(_buidlRedemption);
        buidlSettlement = ISettlement(buidlRedemption.settlement());
@>      buidlLiquiditySource = ILiquiditySource(buidlRedemption.liquidity());
        buidl = IERC20(buidlRedemption.asset());
    }


    function redeemInstant(
        address tokenOut,
        uint256 amountMTokenIn,
        uint256 minReceiveAmount
    )
        external
        override
        whenFnNotPaused(this.redeemInstant.selector)
        onlyGreenlisted(msg.sender)
        onlyNotBlacklisted(msg.sender)
        onlyNotSanctioned(msg.sender)
    {
        // BUG: This will always fail.
@>      tokenOut = buidlLiquiditySource.token();
        ...
    }

The correct implementation would be to use buidlSettlement.liquiditySource() to get the liquidity source, since there is an API for that in the buidlSettlement contract https://etherscan.io/address/0x57Dd4E92712b0fBC8d3f3e3645EebCf2600aCef0#readContract.

Impact

RedemptionVaultWIthBUIDL.sol#redeemInstant will always DoS.

Code Snippet

• https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts/contracts/RedemptionVaultWithBUIDL.sol#L100

Tool used

Manual Review

Recommendation

Use buidlSettlement.liquiditySource() to get the liquidity source. Also add the liquiditySource API to ISettlement.

[sherlock-admin3]

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

merlinboii commented:

Incorrect assumption and OOS.

[0xadrii]

Escalate

This should be considered as Medium, not High. The issue clearly details how the bug will lead to RedemptionVaultWIthBUIDL.sol#redeemInstant always DoS'ing. This means that users can't interact with this function.

From Sherlock docs, Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue?, DoS issues can only be considered as high if both following apply:

• The issue causes locking of funds for users for more than a week.
• The issue impacts the availability of time-sensitive functions

Because redeemInstant always reverts, it is not possible for user's funds to be locked in the contract, as any interaction with it will revert and DoS. Because of this, the issue should be considered as Medium.

[sherlock-admin3]

Escalate

This should be considered as Medium, not High. The issue clearly details how the bug will lead to RedemptionVaultWIthBUIDL.sol#redeemInstant always DoS'ing. This means that users can't interact with this function.

From Sherlock docs, Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue?, DoS issues can only be considered as high if both following apply:

• The issue causes locking of funds for users for more than a week.
• The issue impacts the availability of time-sensitive functions

Because redeemInstant always reverts, it is not possible for user's funds to be locked in the contract, as any interaction with it will revert and DoS. Because of this, the issue should be considered as Medium.

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.

[rickkk137]

u have to use mock version instead of deployed version and if u do that,this isuue cannot happen ,hence inavalid
this issue happen because old redemption version has been used


SettlementTest
LiquiditySourceTest
RedemptionTest

    ERC20Mock buidl;
    SettlementTest settlementTest;
    LiquiditySourceTest liquiditySourceTest;
    RedemptionTest redemptionTest;

    buidl = new ERC20Mock(8);
        settlementTest = new SettlementTest(address(this));
        liquiditySourceTest = new LiquiditySourceTest(USDC);

	

    //deploy redemption test
    redemptionTest = new RedemptionTest(
        address(buidl),
        address(liquiditySourceTest),
        address(settlementTest));

[Kirkeelee]

Since the issue is escalated, I think some of the duplications need to be evaluated. There is no question of issue validity since there is no function token() in the USDC contract.

[IronsideSec]

duplicates of this issue := #75, #90, #94, #114
not a duplicate of this issue := #41, #46, #138

[4gontuk]

duplicates of this issue := #75, #90, #94, #114 not a duplicate of this issue := #41, #46, #138

#46 should be valid, root cause is the same

[0xNirix]

#138 and #41 are exact duplicates of each other. If we are not clubbing together these two in this issue, they should be considered valid separately. cc: @merlinboii

[merlinboii]

#138 and #41 are exact duplicates of each other. If we are not clubbing together these two in this issue, they should be considered valid separately. cc: @merlinboii

Sorry for the late reply. During the judging contest, I mistakenly marked the issue as invalid due to referencing a Mock (my bad). The issue is valid, and I grouped it separately from #138 and #41 as I thought they mentions different root cause.

Moreover, I believe this comment is effective, and for #46 , I think the issue highlights the following root cause:

In RedemptionVaultWithBUIDL.sol: redeemInstant, the tokenOut is set to the token from buidlLiquiditySource, but the actual redemption uses the buidl token.

while the root cause of the issue is a DoS when attempting to access buidlLiquiditySource.token() due to an incorrect setting of buidlLiquiditySource in the initializer(). so, it might differ.

[sherlock-admin2]

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

[WangSecurity]

1. I agree it's a valid issue.
2. I agree it's medium rather than High. Hence, will downgrade the severity.
3. I believe valid dupes are 0xpiken - RedemptionVaultWithBUIDL#redeemInstant() does not work due to an incorrect function call #75, eeyore - Incorrect BUILD integration in the RedemptionVaultWIthBUIDL contract. #90, Ironsidesec - RedemptionVaultWithBUIDL.redeemInstant will always revert #94 and Kirkeelee - Incorrect pointer set in the initialize() function leads to broken functionality of RedemptionVaultWithBUIDL.sol #114.
4. I believe incorrect duplicates are:

• 4gontuk - Inconsistent Token Usage in Redemption Process will cause Financial Loss for Users #46 -- doesn't identify the root cause and in fact I believe is invalid based on this comment.
• merlinboii - The protocol will cause user fund losses due to misalignment between expected and actual token redemption processes #41 -- Firstly, the line "BUIDL variant of the redemption vault can be used as an mTBILL redemption vault" doesn't mean it's a carbon-copy of the mTBILL vault and has to be exactly the same. Secondly, I believe the Buidl vault is intended to process only USDC redemptions and not WBTC redemptions (but my understanding can be incorrect here). Moreover, the admin can withdraw the tokens from the contract. Hence, I believe it's not a duplicate and a different issue, but an invalid one.
• 0xNirix - Using RedemptionVaultWIthBUIDL for mTBILL with MBasisRedemptionVaultWithSwapper will cause a complete loss of redeemed tokens for users #138 -- the same as above

Planning to apply the changes above and accept the escalation.

[0xNirix]

I do not think argument on #41 and #138 being invalid is correct:

1. The mBASIS redemption vault has no provision to redirect redemptions for a certain token (e.g., USDC) to a specific mTBILL redemption vault in the swap case. All swap redemptions must go to the same vault; e.g., if the BUIDL version of mTBILL is configured as the swap redemption vault, then every redemption, including WBTC redemptions, should hit it.
2. Moreover, in the document, there is no mention that the BUIDL variant of mTBILL redemption will be used only for USDC; the paragraph in fact talks about any token:

Instant redemption - MBasisRedemptionVaultWithSwapper takes mBasis from the user, validates conditions, and takes fees. If the contract doesn’t have enough token_out for complete redemption mBASIS will be swapped to mTBILL according to exchange rates through the specified Liquidity Provider, then redemption will be processed on mTBILL redemption vault and token_out tokens from mTBILL redemption transfer to the user. Also, BUIDL variant of the redemption vault can be used as an mTBILL redemption vault

3. It is certainly possible for an admin to manually withdraw the stuck USDC to the user. However, the user never wanted USDC and may suffer a loss due to additional conversions required. Moreover, the feature that was supposed to be instant is no longer instant.
4. And there is one more bigger issue: while doing redemptions, the BUIDL version would use an incorrect slippage check (as the minAmountReceived would be in the token_out that the user wanted, e.g., WBTC, but is actually checked for USDC). This means this could cause permanent loss of funds for the user. The issue was highlighted in 0xNirix - RedemptionVaultWIthBUIDL will potentially cause loss of funds for users redeeming mBasis due to incorrect slippage considerations #130, which was marked as duplicate to 0xNirix - Using RedemptionVaultWIthBUIDL for mTBILL with MBasisRedemptionVaultWithSwapper will cause a complete loss of redeemed tokens for users #138.

cc: @WangSecurity

[WangSecurity]

The mBASIS redemption vault has no provision to redirect redemptions for a certain token (e.g., USDC) to a specific mTBILL redemption vault in the swap case. All swap redemptions must go to the same vault; e.g., if the BUIDL version of mTBILL is configured as the swap redemption vault, then every redemption, including WBTC redemptions, should hit it.

Still, it's said the BUIDL redemption vault can be used as an mTBILL redemption vault, it doesn't say it will be used in the same way as a regular redemption vault and should handle WBTC redemptions.

Moreover, in the document, there is no mention that the BUIDL variant of mTBILL redemption will be used only for USDC; the paragraph in fact talks about any token

I didn't say my understanding was based only that one paragraph. My understanding is based on code comments inside the BUIDL redemption vault that explicitly mention USDC and BUIDL only. Hence, I believe this vault is only for using the USDC and BUIDL vaults.

It is certainly possible for an admin to manually withdraw the stuck USDC to the user. However, the user never wanted USDC and may suffer a loss due to additional conversions required. Moreover, the feature that was supposed to be instant is no longer instant
And there is one more bigger issue: while doing redemptions, the BUIDL version would use an incorrect slippage check (as the minAmountReceived would be in the token_out that the user wanted, e.g., WBTC, but is actually checked for USDC). This means this could cause permanent loss of funds for the user

But, the vault is only meant for using USDC, so submitting WBTC in that case is a user mistake I believe.

Hence, I'm still planning to accept the escalation and apply changes expressed in my previous comment.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xadrii: accepted