search

sherlock-audit / 2024-08-midas-minter-redeemer-judging

1 stars 1 forks source link

eeyore - Outdated `tokenOutRate` used during admin fulfillment of a standard redemption request. #89

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

eeyore

High

Outdated tokenOutRate used during admin fulfillment of a standard redemption request.

Summary

During the fulfillment of a standard redemption request, two separate exchange rates are used to calculate the withdrawal amount.

However, only one of them (mTokenRate) is updated by the admin when performing the fulfillment of a standard redemption request.

Failing to update the tokenOutRate can lead to a loss of funds for the user or the protocol, depending on the price change of the redemption token that can occur between the time of when redemption request was made and its fulfillment.

Vulnerability Details

In contrast to the DepositVault, where only one exchange rate is needed during the deposit request fulfillment, the RedemptionVault uses two exchange rates to calculate the withdrawal amount.

This can be seen in the following piece of code:

File: RedemptionVault.sol
331:             uint256 amountTokenOutWithoutFee = _truncate(
332:                 (request.amountMToken * newMTokenRate) / request.tokenOutRate, // <== outdated tokenOutRate used
333:                 tokenDecimals
334:             );

As shown, only the mTokenRate was updated, as newMTokenRate is used for calculations.

The missing update of the tokenOutRate can lead to a loss of funds due to under- or over-redemption of the output token.

Consider the following scenario:

  1. Alice requests a standard redemption of $100,000 worth of mTokens (mTBILL or mBASIS) into WBTC.
  2. There is a delay of a few hours between the redemption request and the fulfillment of the request.
  3. During that time, the MTokenRate does not change, but the price of WBTC increases by 10%.
  4. During the fulfillment, the admin provides the newMTokenRate, he can even use safeApproveRequest() since the redemption is within the variation tolerance of the mToken exchange rate. However, due to the use of the outdated tokenOutRate, the withdrawal amount is now 10% greater than the current valuation of the mToken/WBTC.

As demonstrated, the protocol loses funds due to not updating the tokenOutRate during fulfillment.

A similar loss can also occur for the user if the price of the output token drops between the time of the request and the redemption.

Impact

Using an incorrect exchange rate during withdrawal amount calculation leads to fund losses for the protocol or the user.

Code Snippet

https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts/contracts/RedemptionVault.sol#L331-L334

Tools used

Manual review.

Recommendations

In addition to providing the newMTokenRate as a parameter for _approveRequest(), an additional newTokenOutRate parameter should be introduced, and similarly to the safeApproveRequest() and approveRequest() functions.

In the case of safeApproveRequest(), the newTokenOutRate should also be validated for variation tolerance.

0xRandluck commented 3 months ago

tokenOutRate at the time of request creation should be used for redemptions.

0xklapouchy commented 2 months ago

Escalate.

This is a valid issue.

The issue is only within the fulfillment of a standard redemption request and is about the usage of an outdated tokenOutRate.

The Vulnerability Details explicitly explains the root cause and should be evaluated by HoJ.

In addition, the duplicate section is incorrect:

sherlock-admin3 commented 2 months ago

Escalate.

This is a valid issue.

The issue is only within the fulfillment of a standard redemption request and is about the usage of an outdated tokenOutRate.

The Vulnerability Details explicitly explains the root cause and should be evaluated by HoJ.

In addition, the duplicate section is incorrect:

  • 35 is a proper duplicate of this issue.

  • 150 looks like a duplicate of this issue but should be reevaluated for correctness, as it is a mix of valid and invalid statements about the code (the invalid part is about deposits).

  • 160 is not a duplicate of this issue. It is straightforwardly an invalid issue itself.

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.

pandasec1 commented 2 months ago

Escalate.

This is a valid issue.

The issue is only within the fulfillment of a standard redemption request and is about the usage of an outdated tokenOutRate.

The Vulnerability Details explicitly explains the root cause and should be evaluated by HoJ.

In addition, the duplicate section is incorrect:

150 doesn't make an incorrect statement because in deposits request.usdAmountWithoutFees is used, which is calculated based on the old tokenInRate

WangSecurity commented 2 months ago

This is actually the teams intention to use the exchange rate at the time of creating the request. Hence, the code here works as expected. Planning to reject the escalation and leave the issue as it is.

0xklapouchy commented 2 months ago

@WangSecurity

I must disagree.

First, I need to ask, where in any documentation is there mention of such a team intention?

If we are talking about this: Fees and exchange rates: the fees/exchange rates that are evolving between the standard request, and the processing are not a bug.

Or this: Once the funds are in this redemption wallet, he will specify an exchange rate. In the vast majority of cases, the exchange rate used will not match the exchange rate from the time of the subscription request, as the user still earns yield during the time we process the redemption.

Or even this: In non-standard scenarios, the admin may use an exchange rate that goes beyond the variation tolerance. In that scenario, he would use a different function from the “safe” one. The only difference with the safe function is that there is no variation check.

It all refers to the use of the newMTokenRate, which indeed is a design decision and allows the admin to use the updated MTokenRate, and is desired. This is necessary as the MTokenRate constantly increases by accumulating yield from BUILD bonds.

However, the issue described in this submission is quite the opposite. The problem is that tokenOutRate is not updated when fulfilling a standard redemption request, but it should be. tokenOutRate update is even more critical than the newMTokenRate and can directly lead to protocol losses.

In contrast to the deposit function, where the tokenOutRate should not be updated (and it isn't, as the value is stored as the depositedUsdAmount). Where user transfers their tokens into the DepositVault at the time of request creation, and from that moment, the tokens are under the protocol’s management. Therefore, using the exchange rate from the time of request creation is beneficial for both the protocol and the user.

However, in the context of redemption functions, there are two factors the protocol needs to account for:

  1. The standard increase in MTokenRate.
  2. The volatility of the outTokenRate.

Only the first is considered when fulfilling a standard redemption request. More importantly, the increase in MTokenRate always benefits the user, while volatility in the outTokenRate can directly lead to protocol losses. This loss to the protocol is described in the scenario within the Vulnerability Details section.

The need to update the outTokenRate arises because, during the time between request creation and fulfillment, the price of the out token can change, and that change cannot be predicted at the time of request creation. Therefore, to fulfill the request, the protocol needs to acquire the out token at an exchange rate that may differ from the one at the time of the request creation.

The statement that the fees/exchange rates that are evolving between the standard request, and the processing are not a bug. is unrelated to this issue. We actually want the outTokenRate to evolve, especially when it benefits the protocol. In cases where the evolving rate is unfavorable to the protocol, it can simply use the original outTokenRate from the request.

WangSecurity commented 2 months ago

Thank you for such an extensive explanation, really appreciate it. But, not all intentions and design decisions should be documented, cause, otherwise, the protocols would have to write out what each step of the code does, which they're not obligated to.

Still, the request.tokenOutRate is intentionally used in the calculation as well as newMTokenRate. Therefore, this report is a recommendation to use a different formula. I see your arguments that the current one is far not the best, but still it's intentionally used. Hence, the decision remains, reject the escalation and leave the issue as it is.

0xklapouchy commented 2 months ago

I'm highly confident in the validity of this issue.

I'll add more context to this case:

Before this contest, Midas' redemption flow only suspected that redemption could be done with the USDC token (refer to the Midas documentation here). In such a case, tokenOutRate, even if USDC slightly depegs, would remain at 1e18, as stated in the known issue: The system currently assumes that the price of one stablecoin is equal to one dollar.

However, as stated in the Readme: "The protocol only accepts whitelisted tokens (e.g. USDC, WBTC, mTBILL, mBASIS)" and in the spec here: Note: from a technical standpoint, it’s ok to say that a token added to the minter/redeemer smartcontract can be used for both, as we can set caps that can prevent a route from being used.

The protocol is expected to use tokens other than stablecoins for deposits and redemptions.

I can say with 99.99% confidence, based on my knowledge of the Midas protocol, that for mTBILL, redemption will only be to USDC.

But the same can't be said about mBASIS redemption. mBASIS is a more complicated asset than mTBILL, which is (or should be) a 1-to-1 representation of iShares $ BlackRock Treasury Bond. While mTBILL is passive, mBASIS is actively managed, with real Asset Managers executing strategies on deposited assets.

These assets can be manually extracted from the mBASIS vault to manually execute trades in different on-chain or off-chain systems.

Such an approach impacts the delay in processing the redemption request, which can take a few days ("Redemptions are also processed daily, with USDC returned within 30 days. Redemptions will be progressively processed faster."). In some cases, large redemption requests require strategies to be unwound by Asset Managers.

Here lies the problem: if mBASIS allows redemption in a token other than a stablecoin, the protocol must ensure that the tokenOutRate is updated. The use of WBTC as an output token is not excluded anywhere in the documentation.

If we consider the volatility of WBTC in the context of days, we can clearly see the problem. At that point, the protocol could process a standard redemption request with an outdated exchange rate for the output token (without realizing it is outdated) or cancel the redemption request.

WangSecurity commented 2 months ago

I see, thank you very much for re-iterating on this, but, still, it's the intention to consider both prices during the request approval, both the new one the price from the request creation. Hence, here the protocol works as it's expected and this has to remain invalid. Planning to reject the escalation and leave the issue as it is.

0xklapouchy commented 2 months ago

Can we still get an opinion on this matter from the Midas protocol? @kostyamospan @gymnasy55

The problem is that the request can be processed in two ways:

  1. Accept and use the outdated tokenOutRate, which can lead to a direct loss of funds for the protocol.
  2. Cancel the request, which can result in users' funds being locked for more than a week.

Here is why the Cancellation option is problematic:

Even in cases where the user is not at fault, this issue can cause their funds to be locked for over a week.

WangSecurity commented 2 months ago

@0xklapouchy excuse me for such a late reply, that's what the team says about this issue:

Yes) If smth happens with WBTC admin could resolve it by changing newMTokenRate or (in case of depeg) admin could just reject the request

(for context, the question was whether this is intended or not)

The use of both prices is actually intended by the protocol design. Additionally, I don't understand how I missed that before, but there's also the following line in the contest README:

Fees and exchange rates: the fees/exchange rates that are evolving between the standard request, and the processing are not a bug.

So here's the issue is exactly about the price change between the standard request and up until it's processed (i.e. that's why the tokenOutRate is outdated).

Therefore, this report has to remain invalid, planning to reject the escalation.

WangSecurity commented 2 months ago

Result: Invalid Has duplicates

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: