merlinboii - The protocol will cause user fund losses due to misalignment between expected and actual token redemption processes

[sherlock-admin4]

merlinboii

Medium

The protocol will cause user fund losses due to misalignment between expected and actual token redemption processes

Summary

The misalignment between the expected token redemption process and its actual implementation may cause temporary loss of funds for users. According to the High-level summary: main flows short description - mBASIS redemption vault with swapper documentation:

MBasisRedemptionVaultWithSwapper is a contract that is responsible for the redemption of the
mBasis. In regular MBasisRedemptionVaultWithSwapper it can be achieved in 3 ways the same
as RedemptionVault, but with one difference in the instant redemption function:

• 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

The mBASIS redemption vault with swapper should allow the BUIDL variant to perform similarly to the mTBILL redemption vault. However, the current code does not support all cases when using the BUIDL variant as the mTBILL redemption vault, leading to potential discrepancies and user losses.

Furthermore, the README: Q: Additional audit information specifies that any discrepancies between the specification and the code are reportable issues.

Root Cause

In MBasisRedemptionVaultWithSwapper.redeemInstant(), when using RedemptionVaultWithBUIDL.redeemInstant() as an mTBILL redemption vault, the protocol is expected to handle different tokenOut options like USDC or WBTC.

MBasisRedemptionVaultWithSwapper.redeemInstant()#L134-L164

File: MBasisRedemptionVaultWithSwapper.sol
088:     function redeemInstant(
089:         address tokenOut,
090:         uint256 amountMTokenIn,
091:         uint256 minReceiveAmount
092:     )
---
134:         uint256 contractTokenOutBalance = IERC20(tokenOutCopy).balanceOf(
135:             address(this)
136:         );
137: 
138:         if (
139:             contractTokenOutBalance >=
140:             amountTokenOutWithoutFee.convertFromBase18(tokenDecimals)
141:         ) { 
---
146:         } else {
147:             uint256 mTbillAmount = _swapMBasisToMToken(amountMTokenWithoutFee); //@audit swap mBASIS for mTBILL provided from LP
148: 
149:             IERC20(mTbillRedemptionVault.mToken()).safeIncreaseAllowance(
150:                 address(mTbillRedemptionVault),
151:                 mTbillAmount
152:             );
153:             
154:             mTbillRedemptionVault.redeemInstant( 
155:@>               tokenOutCopy, //@audit tokenOut input from user
156:                 mTbillAmount,
157:                 minReceiveAmountCopy    
158:             );
159: 
160:             uint256 contractTokenOutBalanceAfterRedeem = IERC20(tokenOutCopy)
161:                 .balanceOf(address(this));
162:@>           amountTokenOutWithoutFee = (contractTokenOutBalanceAfterRedeem -
163:                 contractTokenOutBalance).convertToBase18(tokenDecimals);
164:         }
165: 
166:         _tokenTransferToUser(
167:@>           tokenOutCopy,
168:             user,
169:@>           amountTokenOutWithoutFee,
170:             tokenDecimals
171:         );

However, RedemptionVaultWithBUIDL.redeemInstant() only processes redemptions with USDC as the tokenOut. If a user requests redemption with any tokenOut other than USDC (e.g., WBTC), the USDC transferred from the RedemptionVaultWithBUIDL gets stuck in the MBasisRedemptionVaultWithSwapper contract, and the user does not receive their intended redemption.

RedemptionVaultWithBUIDL.redeemInstant()#L86-L156

File: RedemptionVaultWithBUIDL.sol
086:     function redeemInstant(
087:         address tokenOut,
088:         uint256 amountMTokenIn,
089:         uint256 minReceiveAmount
090:     )
---
098:         address user = msg.sender;
099: 
100:@>       tokenOut = buidlLiquiditySource.token(); //@audit USDC
---
111:         uint256 amountMTokenInCopy = amountMTokenIn;
112: @>      address tokenOutCopy = tokenOut;
113:         uint256 minReceiveAmountCopy = minReceiveAmount;
---
142:         _tokenTransferToUser(
143:@>           tokenOutCopy,
144:             user,
145:             amountTokenOutWithoutFeeFrom18.convertToBase18(tokenDecimals),
146:             tokenDecimals
147:         );
---
156:     }

Although the admin has control over the Liquidity Provider to transfer the mBASIS back to the user and withdraw the stuck USDC for that user, this misalignment between the code and the documentation leads to potential losses for users, as this contract is expected to support all possible cases.

Internal pre-conditions

There are no internal preconditions for this issue, as users can encounter the vulnerability when interacting with the mBASIS redemption vault with the swapper that integrates with the mTBILL redemption vault with BUIDL.

External pre-conditions

User needs to redeem using a whitelisted payment token other than USDC for this vault.

Impact

The protocol cause a temporary loss of funds for users due to discrepancies between the expected behavior described in the documentation and the actual implementation.

Mitigation

Typically, all redemption vaults are designed to support both instant and request-based redemptions and handle whitelisted tokens consistently. Therefore, restricting this vault type to accept only USDC as the whitelisted payment token will reduce the standard vault design's flexibility and prevent users from redeeming other tokens.

As BUIDL is only intended to redeem 1:1 with USDC, as stated in the High-level summary: main flows short description - Redemption vault with BUIDL, updating the RedemptionVaultWithBUIDL.redeemInstant() function to validate and handle the tokenOut input correctly rather than ignoring it would be necessary.

File: RedemptionVaultWithBUIDL.sol
086:     function redeemInstant(
087:         address tokenOut,
088:         uint256 amountMTokenIn,
089:         uint256 minReceiveAmount
090:     )
---
098:         address user = msg.sender;
099: 
-100:       tokenOut = buidlLiquiditySource.token(); //@audit USDC
+100:         require(
+101:             tokenOut == buidlLiquiditySource.token(),
+102:              "RV: token not support by the BUIDL"
+103:         );

---
156:     }

[sherlock-admin2]

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

merlinboii commented:

The issue describes discrepency and missing of the feature that be specified with in the specification doc

[WangSecurity]

Invalid based on the discussion under #99

[sherlock-admin2]

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