search

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

1 stars 1 forks source link

KiroBrejka - The protocol uses the BTC/USD pricefeed for WBTC, which is problematic if WBTC depegs from BTC #48

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

KiroBrejka

Medium

The protocol uses the BTC/USD pricefeed for WBTC, which is problematic if WBTC depegs from BTC

Summary

WBTC uses the BTC/USD chainlink pricefeed which will be problematic if WBTC depegs from BTC

Vulnerability Detail

As we know, on Ethereum Mainnet there is no WBTC/USD price feed, which means that the protocol uses BTC/USD price feed for the Eth Mainnet. This is bad because in case of WBTC depegging from BTC, the users will eventually be minted more mToken than deserved, ultimately receiving more value of mToken for less value assets. This event is rare but possible to happen again like in 2022, especially in wild market swing or in unstable market conditions.

Impact

Users will deposit WBTC at unreal rate, and receive more mToken than deserved

Code Snippet

https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts/contracts/feeds/DataFeed.sol#L96-L115

Tool used

Manual Review

Recommendation

Check which of the networks you are going to deploy to doesn't have WBTC/USD price feed and collect the WBTC price using the WBTC/BTC feed and BTC/USD feed, as source of truth.

sherlock-admin3 commented 3 months ago

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

merlinboii commented:

Low at best as the price received from feed has min/max validation and the admin can control over the price using the manually submission via the custom aggreator

MihailNikolov04 commented 2 months ago

Escalate,

This comment from the judge is irrelevant to the whole depegging event and respectively to the issue at all. Yes, the admin can specify the min/maxExpectedAnswer but this is irrelevant to the depegging event, since as confirmed from the sponsors, the protocol gets the value directly from BTC/USD pricefeed. Second of all the

"admin can control over the price using the manually submission via the custom aggreator"

part of the comment is not true! This action is performed only for mTBILL and mBASIS tokens. For the real assets like WBTC are used chainlink feeds.

I know that a private thread with the sponsor doesn't go as proof of what I say is true but still. I had a convo with the sponsor with this exact question and thats his responce: image

sherlock-admin3 commented 2 months ago

Escalate,

This comment from the judge is irrelevant to the whole depegging event and respectively to the issue at all. Yes, the admin can specify the min/maxExpectedAnswer but this is irrelevant to the depegging event, since as confirmed from the sponsors, the protocol gets the value directly from BTC/USD pricefeed. Second of all the

"admin can control over the price using the manually submission via the custom aggreator"

part of the comment is not true! This action is performed only for mTBILL and mBASIS tokens. For the real assets like WBTC are used chainlink feeds.

I know that a private thread with the sponsor doesn't go as proof of what I say is true but still. I had a convo with the sponsor with this exact question and thats his responce: image

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

novaman33 commented 2 months ago

Escalate, on behalf of the watson read the comment above

sherlock-admin3 commented 2 months ago

Escalate, on behalf of the watson read the comment above

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.

WangSecurity commented 2 months ago

@MihailNikolov04 could you make a numerical example of how many more tokens the users would get in such a case and how large would be the losses to other users/protocol?

MihailNikolov04 commented 2 months ago

@WangSecurity Of course! In the example right here, it is shown that the WBTC/BTC ratio has dropped to a little less than 0.98. The price of one BTC at the moment of writing this comment is nearly $57,700. Considering we are talking about the same scenario, the protocol will miscalculate the USD value of the deposited WBTC by 2%, which, as of right now, is about $1,150. This is because we are taking the WBTC price from the BTC/USD Chainlink price feed, which is the root cause of the issue.

Now, let’s go over the numerical example. If we take some of the data above, assume that the feePercent on WBTC is 5% (500 bips), and deposit 1 WBTC (1e18) via the depositInstant function, with an mToken/USD ratio of 1:1, we get the following numbers:

  1. When the WBTC/BTC ratio is normal (nearly 1:1): 
    tokenAmountInUsd = 5.77e22;
tokenInRate = 57700e18;
feeTokenAmount = 1e18 * 500 / 10000 = 5e16;
amountTokenWithoutFee = 9.5e17;
feeInUsd = 5e16 * 57700e18 / 10**18 = 2.885e21;
mTokenAmount = ((5.77e22 - 2.885e21) * (10**18)) / mTokenRate = 5.4815e22;
  2. When the WBTC/BTC ratio is 0.98: 
    tokenAmountInUsd = 5.655e22;
tokenInRate = 56550e18;
feeTokenAmount = 1e18 * 500 / 10000 = 5e16;
amountTokenWithoutFee = 9.5e17;
feeInUsd = 5e16 * 56550e18 / 10**18 = 2.8275e21;
mTokenAmount = ((5.655e22 - 2.8275e21) * (10**18)) / mTokenRate = 5.37225e22;

    As seen from the examples, the 1:1 scenario mints around 1.09e21 more mTokens to the user than the second scenario, again under the conditions listed above! If we follow these conditions and see the actual value of 1.09e21 mTokens, we see that these tokens are worth around $1,040. This is for every single WBTC deposited under the conditions listed. Worst of all, after the deposit is successful and the user receives $1,000 worth of extra mTokens, they can immediately redeemInstant, even before the depegging has ended, and realize a profit, because we are still reading the price from the BTC/USD price feed. The conclusion is that in the event of a depegging, the protocol will lose about $1,000 for every WBTC deposited and then redeemed.

MihailNikolov04 commented 2 months ago

@WangSecurity Of course! In the example right here, it is shown that the WBTC/BTC ratio has dropped to a little less than 0.98. The price of one BTC at the moment of writing this comment is nearly $57,700. Considering we are talking about the same scenario, the protocol will miscalculate the USD value of the deposited WBTC by 2%, which, as of right now, is about $1,150. This is because we are taking the WBTC price from the BTC/USD Chainlink price feed, which is the root cause of the issue.

Now, let’s go over the numerical example. If we take some of the data above, assume that the feePercent on WBTC is 5% (500 bips), and deposit 1 WBTC (1e18) via the depositInstant function, with an mToken/USD ratio of 1:1, we get the following numbers:

  1. When the WBTC/BTC ratio is normal (nearly 1:1):
tokenAmountInUsd = 5.77e22;
tokenInRate = 57700e18;
feeTokenAmount = 1e18 * 500 / 10000 = 5e16;
amountTokenWithoutFee = 9.5e17;
feeInUsd = 5e16 * 57700e18 / 10**18 = 2.885e21;
mTokenAmount = ((5.77e22 - 2.885e21) * (10**18)) / mTokenRate = 5.4815e22;
  1. When the WBTC/BTC ratio is 0.98:
tokenAmountInUsd = 5.655e22;
tokenInRate = 56550e18;
feeTokenAmount = 1e18 * 500 / 10000 = 5e16;
amountTokenWithoutFee = 9.5e17;
feeInUsd = 5e16 * 56550e18 / 10**18 = 2.8275e21;
mTokenAmount = ((5.655e22 - 2.8275e21) * (10**18)) / mTokenRate = 5.37225e22;

As seen from the examples, the 1:1 scenario mints around 1.09e21 more mTokens to the user than the second scenario, again under the conditions listed above! If we follow these conditions and see the actual value of 1.09e21 mTokens, we see that these tokens are worth around $1,040. This is for every single WBTC deposited under the conditions listed. Worst of all, after the deposit is successful and the user receives $1,000 worth of extra mTokens, they can immediately redeemInstant, even before the depegging has ended, and realize a profit, because we are still reading the price from the BTC/USD price feed. The conclusion is that in the event of a depegging, the protocol will lose about $1,000 for every WBTC deposited and then redeemed.

And this can be exploited multiple times by the same user, leading to straight up stealing funds from the protocol! Actually if the depegging event happens it will be critical for the protocol, but since it's a rare event, medium severity is ok for this one

WangSecurity commented 2 months ago

I'm not sure I understand actually.

Firstly, if there's no WBTC/USD price feed and only the BTC/USD price feed (that we will use), the numbers for the scenario where WBTC price depegs to 0.98 BTC should be the same if the WBTC price is 1 WBTC. In other words, if the price feed of BTC/USD is used, then regardless of WBTC price depegging, the price in the contracts don't change and the tokenAmountInUsd and tokenInRate should be the same in both scenarios since we use the BTC/USD price. Does it make sense or I have to re-iterate?

Secondly, I don't understand the following line, could you elaborate?

The conclusion is that in the event of a depegging, the protocol will lose about $1,000 for every WBTC deposited and then redeemed

MihailNikolov04 commented 2 months ago

I'm not sure I understand actually.

Firstly, if there's no WBTC/USD price feed and only the BTC/USD price feed (that we will use), the numbers for the scenario where WBTC price depegs to 0.98 BTC should be the same if the WBTC price is 1 WBTC. In other words, if the price feed of BTC/USD is used, then regardless of WBTC price depegging, the price in the contracts don't change and the tokenAmountInUsd and tokenInRate should be the same in both scenarios since we use the BTC/USD price. Does it make sense or I have to re-iterate?

Secondly, I don't understand the following line, could you elaborate?

The conclusion is that in the event of a depegging, the protocol will lose about $1,000 for every WBTC deposited and then redeemed

@WangSecurity Actually yes you are right, but it's not quite it. The protocol will think that the WBTC price is exactly equal to BTC (1:1 ratio), which is the actual problem, because outside the protocol WBTC has a price 2% less than BTC. In the example in my last comment, regardless of the deppeging event, the scenario that happens is the number one, again because we are reading from BTC/USD price feed. This is bad because the users collateral appears more valuable in the time when the depegging event is happening. Hope you understand it with this simple example:

WBTC depegs from BTC in a 0.98 ratio and the following things happen:
BTC/USD = 57700$
WBTC/USD = 0.98 * 57700$ = 56546$

These is when it becomes a problem, because the deposited collateral is overvalued by the protocol. You are right that regardless of the depegging, the scenario is always the first one, but it should be like the second scenario when the WBTC value is properly fetched by using either WBTC/BTC ratio and then BTC/USD as source of truth or where it is possible use directly WBTC/USD feed. There is exactly when the protocol overvalues the collateral, leading to harmful calculations.

Now about the mentioned line. What I meant to say is that the protocol gets tricked with 1000$ of value, every time a WBTC is deposited during a depegging event of such size not that the protocol actually loses the money, because again if a user deposit WBTC during a depegging event, the collateral gets overvalued by the protocol and user will be minted way more mTBILL tokens than he deserves. Now I actually see that the example that I have given is not the best one for this issue at all, since if the fee is 5% the protocol won't actually lose money after instant redeem, but it will just mint more mTBILL tokens than the user deserves because again, the collateral will be overvalued. Assuming that a 5% fee is not attractive at all and little to no users will deposit at this fee rate, I will give a one, more relevant example with 1% fee:

  1. WBTC/BTC is nearly 1:1 ratio (The overvalue amount that WBTC has during the depegging event): 
    tokenAmountInUsd = 5.77e22;
tokenInRate = 57700e18;
feeTokenAmount = 1e18 * 100 / 10000 = 1e16;
amountTokenWithoutFee = 9.9e17;
feeInUsd = 1e16 * 57700e18 / 10**18 = 5,77e20;
mTokenAmount = ((5.77e22 - 5,77e20) * (10**18)) / mTokenRate = 5,7123e22;
  2. WBTC has depegged from BTC (0.98 ratio)(The actual mToken amount that 1 WBTC should have during the depegging event): 
    tokenAmountInUsd = 5.655e22;
tokenInRate = 56550e18;
feeTokenAmount = 1e18 * 100 / 10000 = 1e16;
amountTokenWithoutFee = 9.9e17;
feeInUsd = 1e16 * 56550e18 / 10**18 = 5,655e20;
mTokenAmount = ((5.655e22 - 5,655e20) * (10**18)) / mTokenRate = 5,598449999999999e22;
    5,7123e22 - 5,598449999999999e22 = 1,1385e21 = 1138,5$

    in scenarios like this the protocol actually loses money, because when the actual price of WBTC is assumed by the protocol to be equal to the price of BTC, it is 2% less. Assuming that the fee is 1%, the protocol suffers something in the range of 500$ - 600$(1138$ - 57700$ * 1 /100 = 1138$ - 577$ = 561$) of pure loss during the depegging event per one WBTC deposited. In all other times when the fee is above 2% the protocol actually doesn't suffer a loss, but just mints more than deserved tokens to the users, which on it's own is big enough problem, for this issue to be accepted as a valid medium

WangSecurity commented 2 months ago

Hm, thank you very much for such an extensive reply, but I've actually got different numbers. Let me show you my example:

Scenario of WBTC/BTC = 1: BTC == WBTC == 100k. 1% fee for 1 WBTC deposit = 1k.

Scenario of WBTC/BTC = 0.98: WBTC == 0.98 BTC == 98k 1% fee for 1 WBTC == 1k, because WBTC is still treated as 1 BTC or 100k.

Excuse me for it being this simple contrary to yours, but in that case, the protocol actually gets more fees and doesn't lose. The protocol continues to use the price of BTC, therefore, the fees are counted as there is no depeg. Hence, they don't lose, they even get more if the depeg was accounted for.

What am I missing here?

MihailNikolov04 commented 2 months ago

Hm, thank you very much for such an extensive reply, but I've actually got different numbers. Let me show you my example:

Scenario of WBTC/BTC = 1: BTC == WBTC == 100k. 1% fee for 1 WBTC deposit = 1k.

Scenario of WBTC/BTC = 0.98: WBTC == 0.98 BTC == 98k 1% fee for 1 WBTC == 1k, because WBTC is still treated as 1 BTC or 100k.

Excuse me for it being this simple contrary to yours, but in that case, the protocol actually gets more fees and doesn't lose. The protocol continues to use the price of BTC, therefore, the fees are counted as there is no depeg. Hence, they don't lose, they even get more if the depeg was accounted for.

What am I missing here?

@WangSecurity I want to get clear that the main problem here is that the deposited collateral is overvalued when depositing, during the depegging event, and as result of this it is possible for the protocol to realise losses. I think what you are missing here is to count the WBTC as BTC. Yes, you are doing it for the fee, but I mean that if depegging happen, the WBTC which is worth 98k will count as 100k, as the protocol overvalues it. Then, when the fee is subtracted from these 100k, we remain with 99k, which is 1k more than the price of WBTC outside the protocol (99k-98k = 1k). This remains as loss, as this 1k goes to the user in form of mToken. This can be seen in the last line of my examples above:

mTokenAmount = ((100k - 1k) * (10**18)) / mTokenRate = 99k;
mTokenAmount = ((98k - 980) * (10**18)) / mTokenRate = 97,02k;

The difference right here is that in the first case, the value of WBTC inside the protocol is more than the value of WBTC outside the protocol, leading to the overminting of mTokens which when calling the instant redeem function, will result in a loss for the protocol. Other possibility is for the excessive token to just being used for another purposes. In the second case is exactly how the things should look like while the depegging event is happening. There should be no overminting nor overvaluing the collateral and thats what I'm trying to show in my examples above, which may be another thing that you are missing. Just to summarize the examples again, the first one is how things will look like while a depegging event is happening as of the current state of the codebase. The second one is how things should look like while a depegging event is happening, after the recommended mitigation that I have provided is made.

Actually if what you were saying was true, and the protocol wasn't possible to lose money, it wouldn't mint more than deserved mTokens to the users, which I have proved to be true in my examples.

BTW I'm sorry for the response time! It appears that we are in different timezones from one another. Seeing that this is one of the few escalations that you haven't taken a clear decision for yet, I will try not to sleep today and help you resolve it.

MihailNikolov04 commented 2 months ago

@WangSecurity Inserting a link from an issue, which was a part of past Sherlock audit to help you understand the concept better and how it is all about the value difference inside the protocol and outside it. The issue was accepted as valid medium!

WangSecurity commented 2 months ago

Yeah, I see what I was misunderstanding when you said the protocol receives a loss, I thought you meant the protocol team in terms of the fee. Now, I see what you meant is essentially minting more tokens when the WBTC price is lower than BTC. But, I'm still not sure who receives a loss in this case, who do you mean by the "protocol"? The protocol team doesn't receive a loss in terms of fees, who exactly loses in this case?

Of course, I agree it's not good to mint more tokens in this scenario, but the other users don't receive a loss (as I understand) and it's more of an arbitrage opportunity.

About the blueberry finding, firstly, historical decisions are not sources of truth, secondly, it's a completely different protocol, and this situation has far more impact on the lending protocol than it has in this case.

MihailNikolov04 commented 2 months ago

Yeah, I see what I was misunderstanding when you said the protocol receives a loss, I thought you meant the protocol team in terms of the fee. Now, I see what you meant is essentially minting more tokens when the WBTC price is lower than BTC. But, I'm still not sure who receives a loss in this case, who do you mean by the "protocol"? The protocol team doesn't receive a loss in terms of fees, who exactly loses in this case?

Of course, I agree it's not good to mint more tokens in this scenario, but the other users don't receive a loss (as I understand) and it's more of an arbitrage opportunity.

About the blueberry finding, firstly, historical decisions are not sources of truth, secondly, it's a completely different protocol, and this situation has far more impact on the lending protocol than it has in this case.

@WangSecurity When writing "the protocol" I refer to the RedemptionVault. The loss of money for the RedemptionVault happens when a user call redeemInstant function. When the user call the function with tokenOut as USDC he will materialise his winnings as it follows:

  1. When the price of WBTC is overvalued during the deposit the user is overminted on mToken. Then at redeem the user finalises his winnings by calling redeemInstant with USDC as tokenOut 
    amountMToken = 99_000e18(As the user gets overminted in the deposit vault as shown in one of my last comments)
amountMTokenInUsd = 99_000e18
amountTokenOut = amountMTokenInUsd * 1e18 / 1e18 = 99_000e18 (as we input USDC as tokenOut)
  2. When the price of WBTC Is properly fetched 
    amountMToken = 97_020e18(As the recommended mitigation is properly applied and the user isn't overminted)
amountMTokenInUsd = 97_020e18
amountTokenOut = amountMTokenInUsd * 1e18 / 1e18 = 97_020e18 (as we input USDC as tokenOut)

    I hope that after this example you can see when the loss of funds is coming from. When a depegging event happen, the user can deposit WBTC as it is overvalued from the protocol and in result of this he is overminted on mToken. After that he can immediately call the redeemInstant function with USDC as tokenOut. This results into loss of funds for the RedemptionVault, since the user transferred to the DepositVault 1 WBTC (priced at 98k$ while the depegging event is active) and receives 99k$ worth of USDC when calling RedemptionVault::redeemInstant(1k profit for the user, respectively a loss for the protocol). Instead, if the provided recommended mitigation is applied, the user will transfer 1 WBTC to the DepositVault at proper rate and if user redeem, after the fees he will remain with 97_020$, which is the proper amount of assets that he should be getting at 1% fee, when a depegging event is happening.

IWildSniperI commented 2 months ago

Hey @WangSecurity

This is basically allows for arbitraging WBTC price difference from BTC by redeeming usdc

In short

  1. He deposits 98,000 USD worth of wbtc but gets accounted as 100k worth of BTC
  2. He redeems 100,000 usd worth of usdc
  3. He buys now from dex 100,000 usd worth of WBTC

And repeat,

WangSecurity commented 2 months ago

Yeah, I see, thanks to both of you. Indeed, the protocol gives out $2k when they only received $98k. But, when the depeg recovers and WBTC is back at $100k (i.e. equals BTC), the value of the protocol's balance also recovers and now there are no such a loss of $2k in value. Am I incorrect here?

MihailNikolov04 commented 2 months ago

@WangSecurity Yes I think you are right about that, but you are overlooking some other serious issues like draining the USDC and practically any token other than WBTC from RedemptionVault, which is an outcome of the whole overminting issue. The arbitrage opportunity as you alone said is not irrelevant in that case. Overall, as volatile as bitcoin is, this a good chance for a malicious users to materialise some winnings out of their WBTC rather than holding it, and hoping that it would go up. After reading you comment, I think that the main issue here maybe is that the RedemptionVault will be drained from any token other than WBTC, which I think can be considered as breaking core protocol functionality, since the regular users will be limited to redeeming only with WBTC, which some of them may not want to do. As stated in the contest rules:

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

meaning that if my assumption is right, this should qualify for a valid medium.

WangSecurity commented 2 months ago

Yeah, it may cause the problem that the vault is out of USDC (for simplicity picking one token for the discussion), but the protocol doesn't lose in value.

For example, The vault has 10 WBTC and 1m USDC (1 WBTC = 100k USDC). The value is $2m. Then depeg happens and 10 WBTC worth 98k USDC. The attacker deposits 10 WBTC and withdraws 1m USDC because the protocol treats 1 WBTC = 1 BTC = 100k and not 98k. In that case, if we kept track of the depeg, the protocol would have 20k USDC remaining: 10 WBTC * 98k = 980k USDC.

Now, the protocol has 20 WBTC and 0 USDC. The value is $1 960 000 (20 WBTC * $98k). But, the depeg recovers and WBTC is back at $100k. Therefore, the value is again 20 WBTC, but $2m as it was initially.

Therefore, the protocol lost USDC tokens, but they didn't lose any value and the protocol still has $2m value. I understand that the deposit and redemptions are in different vaults and it could disable the redemptions for USDC for some time, but the users can still withdraw in WBTC and there is no loss of value.

Moreover, the are limits for instant redemptions and to mitigate this issue the admin has to set the daily instant redeem limit to a value lower than there are tokens. In other words, if we have 1m USDC, the admin can set the daily instant redeem limit to 500k USDC and the USDC wouldn't be drained.

Is there anything incorrect in my assumptions?

MihailNikolov04 commented 2 months ago

@WangSecurity your assumptions honestly look correct to me. In this case there remain only the overminting and the arbitrage opportunities that go with this issue. By your last comment I understand that the RedemptionVault can be partially drained from every token used in it, except WBTC during depegging, which is something to consider too. Overminting, as you said above in one of your comments is a big enough issue, to consider a thread to the protocol. In support of what I say, I will insert the impact from one of the escalated issues, which includes overminting: image The issue is #154 , and as of you comment, you are planning to accept it as a medium.

pkqs90 commented 2 months ago

@WangSecurity In your above example, the vault should have 20 WBTC + 20k USDC, but it only has 20 WBTC (which means vault loses 20k USDC) due to users selling at a higher WBTC/USDC price at depeg. IIUC, this should be enough for a valid medium?

MihailNikolov04 commented 2 months ago

I believe that @pkqs90 is right about that one because the value in the vault should remain the same ($1_980_000 while the depegging is happening) and as of your example, it is $1_960_000. Other thing that you may miss is that if the daily limit is met in both DepositVault and RedemptionVault, the instant functionality of the vaults will be bricked during the depegging event and essentially this will bring no value to the protocol at all. Other thing is that a depegging event can last in over a week! This can be checked for the period of 13.11.2022 - 20.11.2022. As seen in the links, the least the WBTC has depegged from BTC is slightly bellow 3% for this period. This means that the instant functionality of the protocol can be bricked for 1-2 weeks before the depegging is finally over. As you said, when the depegging event recovers, the protocol should not account any losses, but this applies only if the WBTC stay in the protocol. For example, if a user redeem with WBTC, during the depegging event without knowing that there is such an event, then the protocol will account losses because when the depeg recovers, the WBTC is not going to be in the vault anymore. In this case users mistakes are actually a problem for the protocol since it will account losses because of them. Overall, I think this issue should be considered a valid medium, because the protocol team can't stop users from redeeming with WBTC for 1-2 week or so. As of the LSW's comment, I think it is true and it is something that I missed yesterday. WBTC/BTC at 13.11.2022: image

WBTC/BTC at 20.11.2022: image

WangSecurity commented 2 months ago

Yeah, you’re both correct and my example wasn’t precise. Thank you for correcting. Now, I agree it should be a valid medium. Planning to accept the escalation and validate the issue.

@MihailNikolov04 @0xRandluck are there any duplicates?

pkqs90 commented 2 months ago

I believe https://github.com/sherlock-audit/2024-08-midas-minter-redeemer-judging/issues/152 is a dup.

underdog-sec commented 2 months ago

@WangSecurity correct me if I'm wrong but I believe this issue should not be valid as per the audit scope. Oracles were not part of the scope of the audit, so we can't directly assume that the protocol would be using the BTC / USD feed because there's no WBTC / USD feed in Chainlink. Even in the screenshots provided from discord, sponsor claims they will be using Chainlink, but don't specify anything about the specific feeds that will be used for each asset.

Actually, given that oracles were outside of the scope for the audit, we should assume that they will be working correctly, so the expected behavior from Midas is not to directly use the BTC / USD feed, but instead to use the WBTC / BTC feed, and then the BTC / USD feed to prevent depegs and properly obtain the USD value.

MihailNikolov04 commented 2 months ago

The MBasis/MTBillCustomAggregatorFeed contracts are in scope. They are directly connected to the DataFeed contract. Other thing is that literally every vault calls the DataFeed contract in one way or another, so this should remain valid.

WangSecurity commented 2 months ago

@underdog-sec fair question. The protocol is going to be deployed on Ethereum, and it's going to use the chainlink price feed for WBTC. Since there's no WBTC/USD price feed, I think it's fair to assume they will use BTC/USD price feed directly. The oracles are indeed out-of-scope, but the issue is not in the oracle, the issue is in the contract which treats the price of WBTC and BTC to always be equal. That's why I think it's fair to validate this issue. The decision remains, accept the escalation and validate the issue with medium severity.

underdog-sec commented 2 months ago

@WangSecurity I really believe that it should be considered as out of scope due to the following two reasons:

  1. When Midas requests for a price from an external token, it does so with the _getTokenRate() internal function, which directly requests data from the dataFeed via the getDataInBase18() function:
function _getTokenRate(address dataFeed, bool stable)
        internal
        view
        returns (uint256)
    { 
        // @dev if dataFeed returns rate, all peg checks passed
        uint256 rate = IDataFeed(dataFeed).getDataInBase18(); 

        if (stable) return STABLECOIN_RATE; 

        return rate;
    }

If any depeg took place, it should be handled inside the Datafeed's getDataInBase18() function, the same way timeouts, invalid prices, etc are handled directly in the price feed, which is a contract out of scope. With the current logic, depegs can't actually be handled in the in-scope contracts, as they just receive a rate. The vaults just query an out-of-scope contract built by Midas that is expected to fetch the correct price or to handle adverse scenarios as per the audit guidelines.

  1. Regarding the assumptions of which feed will be used, I also don't believe that the conclusion should be that the project will use the BTC/USD price feed just because there's no WBTC/USD price feed on Ethereum. The standard way to handle these situations is to operate with two feeds (in this case WBTC -> BTC, BTC -> USD). This is a usual approach. But even if we overlooked this, I would like to reiterate that there was no explicit information about which feeds would be used in the README nor the docs, and the feeds were clearly out of scope.

Because of this reasons, I believe the issue should be considered as out of scope

MihailNikolov04 commented 2 months ago

This is the recommended mitigation I provided in the issue. By seeing the oracles it is obvious that the protocol will use BTC/USD feed for the WBTC price, since there is no possibility of implementing dual feed price fetching as of the active state of the codebase. Other thing is that in the begining of this escalation I provided a private thread with this exact question (Yes I know thats not a source of truth, but I believe that @WangSecurity has contacted the sponsors and confirmed it as well). And as he stated:

The oracles are indeed out-of-scope, but the issue is not in the oracle, the issue is in the contract which treats the price of WBTC and BTC to always be equal.

0xRandluck commented 2 months ago

wBTC is designed to always maintain a 1:1 ratio with Bitcoin (BTC). If wBTC depegs from BTC, it's an issue with wBTC itself, not the Midas protocol. Midas assumes that 1 wBTC equals 1 BTC, which is generally true, but in rare instances, wBTC can depeg due to its own issues. Since the protocol relies on wBTC's integrity and trusts wbtc, I believe this falls outside the scope of the protocol's responsibility. It's similar to an oracle providing an incorrect price due to its own issues, which isn't the protocol's fault.

MihailNikolov04 commented 2 months ago

As @WangSecurity stated:

The oracles are indeed out-of-scope, but the issue is not in the oracle, the issue is in the contract which treats the price of WBTC and BTC to always be equal.

The issue is both in wBTC and in the protocol which treats it's price as equal to BTC, event though this is not always true. Me and @pkqs90 proved that such an event can make the protocol account losses and brick the instant functionality of the protocol for the whole duration of the event, which can be around 2 weeks. Another thing is that as I said in my comment above, the DataFeed may be out of scope, it's functions are used in literally every vault, meaning that this can be treated as issue in the vault itself, because the DataFeed contract can do absolutely nothing on itself. Overall I think that the other 15 comments from me, The LSW and @IWildSniperI prove my point and how should remain a valid medium!

MihailNikolov04 commented 2 months ago

Above I provided a link for another issue, which was accepted from sherlock as valid medium. If what you say is true and this is not a problem of the protocol, why there are accepted similar issues at all? Inserting a screenshot with accepted WBTC depegging issues from solodit: image

underdog-sec commented 2 months ago

I'm not saying that this is not an issue. What I'm saying is that this issue can't be considered as a bug as per the guidelines and scope provided for this specific audit.

As I mentioned in my previous comments, saying that the protocol will be using the BTC/USD is just an assumption that has not directly been backed by any data in the README nor by the sponsor during the audit. The only way to know that the protocol would be using one feed for WBTC was to check the DataFeed contract, but as mentioned several times, it is out of scope. Of course if the protocol uses the BTC/USD feed depegs on WBTC will cause losses, but this is only an assumption as per Midas' implementation that can't be considered as valid due to falling outside of scope.

why there are accepted similar issues at all? Inserting a screenshot with accepted WBTC depegging issues from solodit:

Different audit contests have different requirements in their README and different scopes. For those contests, the oracle contracts were in scope. Because of this, such an issue could and had to be considered. However, this is not the case for the Midas contest.

another thing is that as I said in my comment above, the DataFeed may be out of scope, it's functions are used in literally every vault, meaning that this can be treated as issue in the vault itself, because the DataFeed contract can do absolutely nothing on itself.

This statement is incorrect. From Sherlock rules: "If there is a vulnerability in a contract from the contest repository but is not included in the scope then issues related to it cannot be considered valid." In this case, it is crystal clear that if such bug occured, it would belong to the DataFeed contract, which unfortunately is a contract from the contest repository but is out of scope.

MihailNikolov04 commented 2 months ago

I see your point, but I think that the following line applies in full for this issue:

In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

As I understand it, when a vulnerability arises in the ‘DataFeed’ contract, it should be considered valid since, as I said, it’s functions are used in literally every vault. Other thing is that as I said above, I believe that @WangSecurity considered the things you were saying and judged the issue as of knowing this things.

pkqs90 commented 2 months ago

Interesting observation. I just found out that DataFeed.sol is indeed out of scope. Per sherlock rules (and my past experience on scope related rules), if a in-scope contract calls a lib, the lib automatically falls in scope, but if a in-scope contract calls another contract, the contract is not in scope. Let's see how @WangSecurity thinks.

Breeje16 commented 2 months ago

Interesting Observations till now indeed.

In my view, this scenario is similar to "minimum redemption" issue seen in #98. Even though there wasn’t a 250k minimum redemption in the out of scope contract, if it had existed, the impact would have been on the in-scope contract. That was earlier considered an integration issue, and the risk was classified as valid Medium because Sherlock aims to catch such vulnerabilities, even when the root cause lies in an out-of-scope contract but the impact is on in scope contracts.

This situation is quite similar I feel as there is a clear risk of exploitation of in-scope contract due to out of scope contract.

On a side note, In my mind, I always look for issues which can affect any part of codebase in scope and I believe that's what sponsor wants as well. That was the case with #98 as well which would have been valid if 250k limit was there on "out of scope contracts". Either way, as I also submitted #152 which is dup of this, Result of this escalation will help me get clear answer on: Should I report any such issues in the future at all irrespective of how big the impact is on the in scope contracts as any integration issue can be invalidated stating it arise from out of scope contract.

My Personal Opinion on this is:

This is my personal perspective. This could have been the case with #98 being valid if there was a limit. Sherlock's Perspective can be different. Anyways an interesting conversation this.

WangSecurity commented 2 months ago

Indeed, the point about out-of-scope contract is very good. To get the price of an asset, the contracts call DataFeed contract, and specifically, the getDataInBase18 function. Then, it fetches the associated Aggregator (i.e. either chainlink oracle or the custom price feed made by Midas). After getting the price, the contract checks for price being correct (i.e. `answer > 0) and the time. Then, it converts the price to 18 decimals and returns to the contract to continue the deposit/redemption.

Therefore, all the checks and adjustments are made in the DataFeed contract. The contract is indeed out of scope. The argument can be that we can check for depeg between BTC and WBTC in the vault contract after the price is returned. But, the contract that makes all the appropriate checks is DataFeed, therefore, it's indeed the issue in the DataFeed contract which leads to not accounting for the depeg between WBTC and BTC. Hence, the following rule applies, since DataFeed is OOS in this contest:

If there is a vulnerability in a contract from the contest repository but is not included in the scope then issues related to it cannot be considered valid.

Also, I see the argument that this issue impacts the entire protocol and causes impact on the in-scope contracts. Unfortunately, based on the contract scope standards (point 7), it's not a valid issue. Thanks to the Watsons point it out.

Planning to reject the escalation and leave the issue as it is. Will apply this decision in a couple of hours.

MihailNikolov04 commented 2 months ago

@WangSecurity Yesterday you pointed out that if the issue is in the whole protocol, because the all of the vault contracts are calling the getDataInBase18 function. Back then you said that regardless the contract being out of scope, the issue is in scope, because it applies for every possible vault in the protocol. As stated in sherlock rules, when a function is called in a contract in scope, the issue is in scope too

In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

WangSecurity commented 2 months ago

Yesterday you pointed out that if the issue is in the whole protocol, because the all of the vault contracts are calling the getDataInBase18 function. Back then you said that regardless the contract being out of scope, the issue is in scope, because it applies for every possible vault in the protocol. As stated in sherlock rules, when a function is called in a contract in scope, the issue is in scope too

When I've said it I was referring to oracles being OOS as a whole, not the DataFeed contract. And based on the DataFeed contract being responsible for checking the price returned and this contract being OOS, this issue should be also OOS.

Also, the rule you quoted is about libraries, not about contracts and DataFeed is a contract, not a library. Hence, the line that I quoted in the previous comment applies.

The decision remains, reject the escalation and leave the issue as it is. Planning to apply the decision in several hours.

Breeje16 commented 2 months ago

Hi @WangSecurity.. I have doubts about this point:

The argument can be that we can check for depeg between BTC and WBTC in the vault contract after the price is returned. But, the contract that makes all the appropriate checks is DataFeed, therefore, it's indeed the issue in the DataFeed contract which leads to not accounting for the depeg between WBTC and BTC.

Isn't it subjective? I mean if a protocol has 2 contracts: 1 in scope and other OOS then future integration issues between them with HM level impacts will be judged based on where the mitigation should be straightforward? If it's straightforward to mitigate on in scope contract then issue is valid and if it's straightforward to mitigate in OOS Contract then issue is invalid? While in reality mitigating on any of them solves the problem of an Issue whose impact is in scope.

Medium Severity as per docs is:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

I don't see anything here which suggest that if the mitigation is in OOS code then the issue is not valid even if it cause loss of funds. I thought HM level in-scope impacts is what we need to find irrespective of mitigations.

I don't mind you invalidate this issue as long as sponsor fixes this one as that's what my motive was when submitting but IMHO, your comment: "I see the argument that this issue impacts the entire protocol and causes impact on the in-scope contracts." and invaliding should never happen especially for HM level impacts.

Breeje16 commented 2 months ago

Few After Thoughts I am adding:

If we consider the rule used to invalidate:

  1. 100 will be invalidated by:

buidlRedemption contract is OOS so we need to assume it will behave correctly. So when calling redeem function, it is expected to redeem even if token balance is less than 250k. If that is not the case, it's OOS.

  1. 99 will be invalidated by:

buidlRedemption contract is OOS so we need to assume it will behave correctly. So when we call buidlRedemption.liquidity(), it will return correct buidlLiquiditySource and not USDC. If that is not the case, it's OOS.

  1. Similarly in this issue:

DataFeed contract is OOS so we need to assume it will behave correctly. So when we call IDataFeed(dataFeed).getDataInBase18(), it will return correct value handling depeg events as well. If that is not the case, it's OOS.

IMHO, all these 3 issues are valid as they have HM level in scope impact. But only Difference between above 2 and this one is that straightforward Mitigation will happen in OOS contract here rather than in scope contract (which is also a subjective call).

WangSecurity commented 2 months ago

As I've said previously, the contract that checks and makes adjustments to the price (i.e. adjusting decimals). Therefore, the issue is that DataFeed doesn't check the price correctly. I'm not talking about the mitigation, I'm talking about the root cause of the issue. And the root cause is inside the OOS contract as said above in this comment and in my previous comments.

About the other 2 issues, the issue is in the in-scope contract which doesn't integrate with external contract correctly. Hence, the contracts scope standard is not applicable for these 2 issues.

Also, the contracts scope standard doesn't say that the issue is valid if it has HM impact, the standard is about issues located in the OOS contracts are not valid issues. This is very much the case here as said above.

Hence, the decision remains the same, reject the escalation and leave the issue as it is. Planning to apply that decision in a couple of hours.

Breeje16 commented 2 months ago

I'm talking about the root cause of the issue. And the root cause is inside the OOS contract

This is the subjectivity I was talking about.

Anyways, please go ahead and invalidate. I don't have any more points here.

WangSecurity commented 2 months ago

The root causes are in the in-scope contracts in these 2 issues.

100 — the root cause is inside the Midas vault which doesn’t check for the balance of BUIDL after redemptions being below a certain threshold and then withdrawing the tokens if needed. The OOS contract works as expected. Additionally, there’s a specification that the Midas vault has to have such functionality.

99 — the root cause is within the Midas in-scope contract which doesn’t get the liquidity source correctly.

In both cases, the OOS contracts work as expected and that’s the in-scope contracts that have the issue. In this report, it’s DataFeed which is responsible for the appropriate checks and adjustments on the price. The adjustment for the depeg is missing in the DataFeed contract, while the in-scope contracts work as expected.

Hence, the issue here is in the out of scope contract. The decision remains, planning to reject the escalation and leave the issue as it is. Planning to apply it in a couple of hours.

WangSecurity commented 2 months ago

Result: Invalid Unique

sherlock-admin2 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: