sherlock-admin3 commented 6 months ago

maushish

medium

Unhandled Chainlink revert would lock access to Oracle Price feeds.

Summary

Chainlink's latestRoundData() is used which could potentially revert and make it impossible to query any prices. This could lead to permanent denial of service.

Vulnerability Detail

Chainlink has taken oracles offline in extreme cases. For example, during the UST collapse, Chainlink paused the UST/ETH price oracle, to ensure that it wasn't providing inaccurate data to protocols.

In such a situation (or one in which the token's value falls to zero), all liquidations requests which are being off-chain will be reverted. This could happen due to any of these reasons:

the call to Chainlink's registry.latestRoundData would fail.
The following check might fail
```
require(
        // solhint-disable-next-line not-rely-on-time
        block.timestamp - updatedAt <= _HEALTHY_DIFF,
        "DF: feed is unhealthy"lthy"
    );
```
If the Oracle price lookup reverts, liquidations will be frozen, and the user will be immune to liquidation requests.

Impact

Liquidations may not be possible at a time when the protocol or user needs them most.

Code Snippet

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/DepositVault.sol#L137 https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/feeds/DataFeed.sol#L75

Tool used

Manual Review

Recommendation

Use a try-catch block mechanism to safeguard against this possibility

maushish commented 5 months ago

@pkqs90 @MxAxM There hasn't been any reason stated for my finding to be considered invalid.

[!TIP] Here are some similar findings that have been previously reported on sherlock and C4A :1, 2,

MxAxM commented 5 months ago

@pkqs90 @MxAxM There hasn't been any reason stated for my finding to be considered invalid.

Tip

Here are some similar findings that have been previously reported on sherlock and C4A :1, 2,

In this situation admin can pause the contracts to prevent interactions

maushish commented 5 months ago

@MxAxM I respectfully disagree with your reason for invalidating my issue.

Admin taking action to stop the interaction depends only on the notification of the discord server and the admin availability at that time as already mentioned in the chainlink docs section
As mentioned in the docs: My finding severity is also deemed to be completely valid as the core contract functionality will be broken.

[!NOTE] Previously obront has also reported this same finding in blueberry contest that happened on sherlock you can see it here.

And for these reasons, I believe my finding should be escalated.

MxAxM commented 5 months ago

@MxAxM I respectfully disagree with your reason for invalidating my issue.

* Admin taking action to stop the interaction depends only on the notification of the discord server and the admin availability at that time as already mentioned in the [chainlink docs section](https://docs.chain.link/data-feeds/deprecating-feeds?network=deprecated&page=1)
  ![image](https://private-user-images.githubusercontent.com/114429859/337877982-c4e029ae-5b01-4108-a13e-614950da2bb7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTc5MzA0MDUsIm5iZiI6MTcxNzkzMDEwNSwicGF0aCI6Ii8xMTQ0Mjk4NTkvMzM3ODc3OTgyLWM0ZTAyOWFlLTViMDEtNDEwOC1hMTNlLTYxNDk1MGRhMmJiNy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNjA5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDYwOVQxMDQ4MjVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT03NzFlODFiOGI5NWZkYjE3MzFjMTA2OTRmZjQwMGQzNmVhNGVjNjc2YjBlMzRiYjllMDRhZTUyMWI4M2MwMjhjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.A2cl274DzWx_lfL1CqWLCCxlUGPSlHhzTEt3rMJOi74)

* As mentioned in the docs:
  ![image](https://private-user-images.githubusercontent.com/114429859/337877739-6be86719-99f8-494f-ab60-2b95d1c21ef3.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTc5MzA0MDUsIm5iZiI6MTcxNzkzMDEwNSwicGF0aCI6Ii8xMTQ0Mjk4NTkvMzM3ODc3NzM5LTZiZTg2NzE5LTk5ZjgtNDk0Zi1hYjYwLTJiOTVkMWMyMWVmMy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNjA5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDYwOVQxMDQ4MjVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jYTBjMWJjOTMyZTI0ZTQ5MTM0MDYzNjI1Yzg1Zjk5OWE1OTNlZTY1ZTdhZGMzYjdlN2ZhMTNkY2FjNDQ4YzUwJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.bLFNwonXpC_1VaRcZ2kk9vTkQtElEXWGgSZLqGmkygI)
  My finding severity is also deemed to be completely valid as the core contract functionality will be broken.

Note

Previously obront has also reported this same finding in blueberry contest that happened on sherlock you can see it here.

And for these reasons, I believe my finding should be escalated.

It's a design choice to prevent use of stale price, admins should change aggregator in this situation

maushish commented 5 months ago

@MxAxM sir, I believe you are not reading the issue properly. The issue is Oracle price lookup reverts, due to which users won't be able to request liquidation of assets. As this is a highly centralized RWA protocol, liquidation is done off-chain. If there aren't any requests, users/protocols will be immune to liquidation until either Oracle prices lookup stops reverting or admin stops the interaction. This is happening due to the absence of a try-catch block mechanism that should have been implemented when the protocol decided to go through the current design choices.

maushish commented 5 months ago

For reference here are the same findings that have been previously reported on sherlock: 1,2,3, 4 Even after this if you think my issue is invalid, I would like to request another judge's interference here.

MxAxM commented 5 months ago

For reference here are the same findings that have been previously reported on sherlock: 1,2,3, 4 Even after this if you think my issue is invalid, I would like to request another judge's interference here.

You can escalate the issue if you disagree with decision

maushish commented 5 months ago

I would love to but sadly I haven't passed the threshold yet. My other issue also has been escalated by another judge/warden so if you can escalate please do :) you can see the other issue here

MxAxM commented 5 months ago

Escalate

Please review the conversion

sherlock-admin3 commented 5 months ago

Escalate

Please review the conversion

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.

0xMR0 commented 5 months ago

@maushish Historically such issues are judged as low severity. I highly doubt, it will be accepted as Medium considering sherlock strict rules.

maushish commented 5 months ago

Please refrain from giving any responses under an issue especially when you haven't read the whole conversation, I clearly mentioned 4 findings submitted on sherlock itself mentioning the same issue as mine, and they all have been considered valid mediums.

0xMR0 commented 5 months ago

Please refrain from giving any responses under an issue especially when you haven't read the whole conversation, I clearly mentioned 4 findings submitted on sherlock itself mentioning the same issue as mine, and they all have been considered valid mediums.

Historical decisions are not considered sources of truth. Please check sherlock rules

maushish commented 5 months ago

I am completely aware of that rule(Jan 1, 2024), even then my finding is a medium as it breaks the contract's main functionality in case of emergency i.e oracle stops providing data feeds. Secondly, a mistake by judges can't happen 4 times, if you think it can happen then sherlock judging is flawed.

WangSecurity commented 5 months ago

Firstly, as correctly said above, historical decisions are not sources of truth and saying that similar issues were judged as valid in the past is not a valid argument. Just an advice for the future, the you don't have to spend time searching for the similar valid reports, since they do not affect the final judgement.

Secondly, as I see IB01/USD price feed admin is restricted by the README, but before making the final decision, will consult with the sponsor what exactly they meant by restricted in that case.

maushish commented 5 months ago

@WangSecurity thanks for the response Apologies for my invalid argument. I posted all of the previous findings to reason my severity criteria, which you now have adviced not to do in the future, and I will be abiding by that.

Secondly, as I see IB01/USD price feed admin is restricted by the README, but before making the final decision, will consult with the sponsor what exactly they meant by restricted in that case.

I agree with your argument that it is restricted and it would be really great if the sponsor confirms what they actually meant by restricted here before we continue the discussion further.

WangSecurity commented 5 months ago

I've got the clarification and the protocol team meant that the price feed shouldn't return stale data (>3 days). Hence, I believe we shouldn't assume the case when the price feed is just offline.

Planning to reject the escalation and leave the issue as it is.

maushish commented 5 months ago

Hey @WangSecurity thanks for the response Here is my reasoning for why I think this finding is a valid medium:

As mentioned in the chainlink Docs EUR/USD is a forex type Datafeed which isn't always live

Chainlink mentions the market hours of forex type data feeds:
Now I agree with the sponsor statement that the price wouldn't be giving stale data for more than 3 days, and here lies my issue itself any high TVL chain gas token or any huge liquidity token getting collapsed in a too short time could lead to Chainlink shutting down their oracles(as happened in LUNA case), now here consider a scenario in which chainlink shut down their data feed in the market hours let's say on Thursday (which is a valid marketing hour as per the chainlink docs) but due to the emergency oracle shutdown user won't be able to request any liquidations same goes for the protocol, and as I mentioned above the only way this important information can be shared is through discord which makes it dependent on admin activity if he sees the message or not. Hence the liquidation's requests will be bricked as the oracle is down also they haven't passed the 3-day threshold yet, and they will be in this condition until admin sees the announcement and takes some action regarding it or chainlink restarts the oracles.

WangSecurity commented 5 months ago

It's a very fair point, but if Chainlink would turn off their oracle, then it's quite an emergency situation and it would be better if it was off. Can you elaborate on "liquidation" a bit. Is there a liquidation mechanic? As I understand in terms of that protocol only EUR/USD price feed is used and if it's off, the users still can deposit USDC? Anyway, please forward me the link to liquidation functionality, cause from the current perspective I don't think it's needed here, cause it's essentially a DEX for buying IB01 shares on-chain. Hence, this seems at max low severity.

maushish commented 5 months ago

As mentioned in the readme: Minting and burning aka swapping of USDC to mTBILLs will be done off-chain, what I meant from liquidations was in this emergency the user should be allowed to swap(mTBILLs -> USDC or USD) if not this will greatly impact the market value of the protocol I agree with your statement:

cause from the current perspective I don't think it's needed here, cause it's essentially a DEX for buying IB01 shares on-chain.

The above impact stands true in the dex case too. Let me give a realistic scenario:

As mentioned in the readme for first-time users the threshold is $100k which makes users "wealthy" type when it comes to funds.
In case of emergency, oracles were forced to shut down if the user wanted to swap (could be many reasons, ex: the token that is crashing, they own a huge amount in that, or it could be simply they want to invest in some other token) simply so that they don't account for any loss, they wouldn't be able to do so because the protocol hasn't taken proper measures.
As far as good practices go DEXes should always allow swapping, in this case, it becomes more necessary because the users aren't your normal ones(because of the threshold being $100k) even losing a few could lead to a huge loss of market value.

Your statement:

Hence, this seems at max low severity.

This stands true for normal dexes but in this case, even losing 10 users could potentially be a loss of 1 million dollars to the protocol in terms of market value. In my opinion, losing market value is the same as losing funds, especially when the protocol is dealing with RWAs, which are highly dependent on off-chain procedures.

WangSecurity commented 5 months ago

As I see, Chainlink's EUR/USD price feed is only used during the deposit, hence, there's no impact on the redemption side. Therefore, even in case of emergency withdrawals, users will easily withdraw their USDC and it won't revert due to chainlink's offline price feed.

The decision remains the same, reject the escalation and invalidate the report.

maushish commented 5 months ago

Hey @WangSecurity thanks for the response.

[!NOTE] This is the flow by which user can back his original asset by returning mTBILLs

As you can see in the RedemptionVault.sol

    function redeem(address tokenOut, uint256 amountTBillIn)
        external
        onlyGreenlisted(msg.sender)
        whenNotPaused
    {
        require(amountTBillIn > 0, "RV: 0 amount");

        address user = msg.sender;

        lastRequestId.increment();
        uint256 requestId = lastRequestId.current();

        _requireTokenExists(tokenOut);
        _tokenTransferFromUser(address(mTBILL), amountTBillIn);

        emit Redeem(requestId, user, tokenOut, amountTBillIn);
    }

In ManageableVault.sol

    function _tokenTransferFromUser(address token, uint256 amount) internal {
        IERC20(token).safeTransferFrom(
            msg.sender,
            tokensReceiver,
            amount.convertFromBase18(_tokenDecimals(token))
        );
    }

_tokenDecimals is being taken from the data feed. Hence your argument:

Therefore, even in case of emergency withdrawals, users will easily withdraw their USDC and it won't revert due to chainlink's offline price feed.

seems to be wrong here.

WangSecurity commented 5 months ago

As I see here in ManageableVault.sol:

    function _tokenTransferFromUser(address token, uint256 amount) internal {
        IERC20(token).safeTransferFrom(
            msg.sender,
            tokensReceiver,
            amount.convertFromBase18(_tokenDecimals(token))
        );
    }

    /**
     * @dev retreives decimals of a given `token`
     * @param token address of token
     * @return decimals decinmals value of a given `token`
     */
    function _tokenDecimals(address token) internal view returns (uint8) {
        return IERC20Metadata(token).decimals();
    }

tokenDecimals is taken from the token contract, not the price feed contract. Moreover, it's done one the mTBILL token which is transferred from the user during the redemption and is not connected to the EUR/USD price feed. But please correct if it's wrong.

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

maushish commented 5 months ago

Yup you are correct apologies for my misunderstanding.

WangSecurity commented 5 months ago

Result: Invalid Unique

sherlock-admin4 commented 5 months ago

Escalations have been resolved successfully!

Escalation status:

MxAxM: rejected

sherlock-audit / 2024-05-midas-judging

maushish - Unhandled Chainlink revert would lock access to Oracle Price feeds. #52

Unhandled Chainlink revert would lock access to Oracle Price feeds.

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation