sherlock-admin commented 1 year ago

rvierdiiev

medium

In case if symbol is not valid it should be not possible to open position

Summary

Vulnerability Detail

When user creates a quote, then there is a check [that symbol is valid](In case if symbol is not active it should be not possible to open position). Otherwise, you can't create quote.

It's possible that after some time of trading, symbol will be switched off.

When this happened, then all trades that use old symbol should be closed in some time. And new trades should not be started. All pending qoutes should be canceled adn locked to be unlocked. However, there is no check if symbol is valid in PartyBFacetImpl.openPosition function. As result partyB still can open position for not valid symbol.

It's possible that later, oracle will stop provide signatures with prices for that symbol, which means that position can be stucked.

Impact

Possible to open position for invalid symbol.

Code Snippet

Tool used

Manual Review

Recommendation

Do not allow to open position for invalid symbol.

mstpr commented 1 year ago

Escalate

Symbol manager is trusted. Symbol manager should not do this action when there are any symbols actively being traded or queued. In addition to trust factor, funds will not be stucked. The pnl will be calculated as usual. Please check the Muon code here

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/muon/crypto_v3.js

sherlock-admin2 commented 1 year ago

Escalate

Symbol manager is trusted. Symbol manager should not do this action when there are any symbols actively being traded or queued. In addition to trust factor, funds will not be stucked. The pnl will be calculated as usual. Please check the Muon code here

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/muon/crypto_v3.js

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.

ctf-sec commented 1 year ago

Agree with escalation

circlelooper commented 1 year ago

There should be no doubt this one is valid.

It's impractical to assume symbol manager would not invalidate a symbol when there are any symbols actively being traded or queued, symbol manager could be DOSed otherwise (even if there is no trading, when symbol manager tries to invalidate a symbol, malicious user can front-run and create a quote with that symbol);
Opening a quote with invalid symbol breaks the invariant that no invalid symbol should be used in a quote, clearly code does not work as intended, also leading to various issues and potentially losses for the users

mstpr commented 1 year ago

@circlelooper

1- Symbol manager can easily avoid front-running: Pausing the partyA, partyB actions and then does the symbol update 2- When a symbol is invalid, no quotes can be opened anymore by any partyA https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L44 3- Even there are some trades that going on with an invalid symbol, there is still no problem because the Muon app will be getting the correct pnl. MuonApp checks the quotes symbolId, calculates the price and returns the pnl. Nothing can go wrong here as long as the MuonApp works as intended.

circlelooper commented 1 year ago

@mstpr

Malicious user can still front-run pausing to send out quote with invalid symbol, there is not much difference;
A honest user may send quotes with long expiration dates, it's irrational for admin to wait for time to expire then invalidate the symbol;
MuonApp won't work as intended with invalid symbolId, as mentioned in the report:

It's possible that later, oracle will stop provide signatures with prices for that symbol, which means that position can be stucked.

This can be seen from verify method:

The verify method takes several inputs, including the signature, reqId, nonceAddress, start, size, v3Contract, partyA, nonce, uPnl, loss, symbolIds, prices, timestamp, and chainId. It verifies the signature by comparing it with a generated hash of the provided parameters. If the signature is successfully verified, the method returns a subset of the input data, including the v3contract, partyA, nonce, uPnl, loss, symbolIds within a specified range, prices corresponding to those symbolIds, timestamp, and chainId. If the signature verification fails, an error is thrown.

            case 'verify': {
                let { signature, reqId, nonceAddress, start, size, v3Contract, partyA, nonce, uPnl, loss, symbolIds, prices, timestamp, chainId } = params
                const seedRequest = { ...request, method: 'partyA_overview', reqId }
                start = parseInt(start)
                size = parseInt(size)
                symbolIds = JSON.parse(symbolIds)
                prices = JSON.parse(prices)
                const seedSignParams = [
                    { type: 'address', value: v3Contract },
                    { type: 'address', value: partyA },
                    { type: 'uint256', value: nonce },
                    { type: 'int256', value: uPnl },
                    { type: 'int256', value: loss },
                    { type: 'uint256[]', value: symbolIds },
                    { type: 'uint256[]', value: prices },
                    { type: 'uint256', value: timestamp },
                    { type: 'uint256', value: chainId },
                ]
                const hash = this.hashAppSignParams(seedRequest, seedSignParams)
                if (!await this.verify(hash, signature, nonceAddress))
                    throw `signature not verified`

                return {
                    v3Contract,
                    partyA,
                    nonce,
                    uPnl,
                    loss,
                    symbolIds: symbolIds.slice(start, start + size),
                    prices: prices.slice(start, start + size),
                    timestamp,
                    chainId
                }

            }

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/muon/crypto_v3.js#L412-L446

Symmetrical gets prices from Binance, Kucoin and Mexc, things will go wrong if use an symbol not supported by those platforms

getPrices: async function (symbols) {
    const promises = [
        this.getBinancePrices(),
        this.getKucoinPrices(),
        this.getMexcPrices(),
    ]

    const result = await Promise.all(promises)
    const markPrices = {
        'binance': result[0],
        'kucoin': result[1],
        'mexc': result[2],
    }

    if (!this.checkPrices(symbols, markPrices)) throw { message: `Corrupted Price` }

    return { pricesMap: markPrices['binance'], markPrices }
},

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/muon/crypto_v3.js#L125-L142

Opening a quote with invalid symbol is not an intended behavior, can lead to unexpected results.

hrishibhat commented 1 year ago

@mstpr Do you have additonal comments?

mstpr commented 1 year ago

@mstpr

Malicious user can still front-run pausing to send out quote with invalid symbol, there is not much difference;

A honest user may send quotes with long expiration dates, it's irrational for admin to wait for time to expire then invalidate the symbol;

MuonApp won't work as intended with invalid symbolId, as mentioned in the report:

It's possible that later, oracle will stop provide signatures with prices for that symbol, which means that position can be stucked.

This can be seen from verify method:

The verify method takes several inputs, including the signature, reqId, nonceAddress, start, size, v3Contract, partyA, nonce, uPnl, loss, symbolIds, prices, timestamp, and chainId. It verifies the signature by comparing it with a generated hash of the provided parameters. If the signature is successfully verified, the method returns a subset of the input data, including the v3contract, partyA, nonce, uPnl, loss, symbolIds within a specified range, prices corresponding to those symbolIds, timestamp, and chainId. If the signature verification fails, an error is thrown.
            case 'verify': {
                let { signature, reqId, nonceAddress, start, size, v3Contract, partyA, nonce, uPnl, loss, symbolIds, prices, timestamp, chainId } = params
                const seedRequest = { ...request, method: 'partyA_overview', reqId }
                start = parseInt(start)
                size = parseInt(size)
                symbolIds = JSON.parse(symbolIds)
                prices = JSON.parse(prices)
                const seedSignParams = [
                    { type: 'address', value: v3Contract },
                    { type: 'address', value: partyA },
                    { type: 'uint256', value: nonce },
                    { type: 'int256', value: uPnl },
                    { type: 'int256', value: loss },
                    { type: 'uint256[]', value: symbolIds },
                    { type: 'uint256[]', value: prices },
                    { type: 'uint256', value: timestamp },
                    { type: 'uint256', value: chainId },
                ]
                const hash = this.hashAppSignParams(seedRequest, seedSignParams)
                if (!await this.verify(hash, signature, nonceAddress))
                    throw `signature not verified`

                return {
                    v3Contract,
                    partyA,
                    nonce,
                    uPnl,
                    loss,
                    symbolIds: symbolIds.slice(start, start + size),
                    prices: prices.slice(start, start + size),
                    timestamp,
                    chainId
                }

            }
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/muon/crypto_v3.js#L412-L446

Symmetrical gets prices from Binance, Kucoin and Mexc, things will go wrong if use an symbol not supported by those platforms
    getPrices: async function (symbols) {
        const promises = [
            this.getBinancePrices(),
            this.getKucoinPrices(),
            this.getMexcPrices(),
        ]

        const result = await Promise.all(promises)
        const markPrices = {
            'binance': result[0],
            'kucoin': result[1],
            'mexc': result[2],
        }

        if (!this.checkPrices(symbols, markPrices)) throw { message: `Corrupted Price` }

        return { pricesMap: markPrices['binance'], markPrices }
    },
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/muon/crypto_v3.js#L125-L142

Opening a quote with invalid symbol is not an intended behavior, can lead to unexpected results.

You can't frontrun if you pause first, if you try to frontrun pause Symbol manager will check and will not update the symbol.

I agree that symbols shouldn't be opened if they are invalid however, I don't think this is a medium finding considering symbol manager is trusted, also as long as the Muon oracle gives price there is no real harm.

Remember, symbol manager can update a symbol while a quote is actually opened aswell, so it all comes down to Symbol managers actions

panprog commented 1 year ago

I'd like to add to the discussion: if there are positions opened with the symbol, which is switched off, there is no way to forcedly close all those positions, which is a much bigger problem than opening quotes and somewhat similar as well: positions already opened (and quotes already sent) will remain valid even after symbol is switched off. I'd say it's no big deal if quotes can be opened for invalid symbol as it's not much different from already opened quotes with invalid symbol. Yes, it's better if it's not allowed, but it's not much different from already opened quotes, so I'd say it's low impact. I've also seen this problem but didn't report it exactly because I thought that already opened positions with invalid symbol are what really matters and since those can't be closed, there is no real harm in letting open position with invalid symbol.

circlelooper commented 1 year ago

You can't frontrun if you pause first, if you try to frontrun pause Symbol manager will check and will not update the symbol.

Sounds like a DOS attack.

I agree that symbols shouldn't be opened if they are invalid however, I don't think this is a medium finding considering symbol manager is trusted, also as long as the Muon oracle gives price there is no real harm. Remember, symbol manager can update a symbol while a quote is actually opened aswell, so it all comes down to Symbol managers actions

Symbol manager is trusted doesn't mean he/she can do everything at perfect timing, symbol manager will have to update a symbol even if there are pending requests, because some requests may have no expiration date.

I'd like to add to the discussion: if there are positions opened with the symbol, which is switched off, there is no way to forcedly close all those positions, which is a much bigger problem than opening quotes and somewhat similar as well: positions already opened (and quotes already sent) will remain valid even after symbol is switched off. I'd say it's no big deal if quotes can be opened for invalid symbol as it's not much different from already opened quotes with invalid symbol. Yes, it's better if it's not allowed, but it's not much different from already opened quotes, so I'd say it's low impact. I've also seen this problem but didn't report it exactly because I thought that already opened positions with invalid symbol are what really matters and since those can't be closed, there is no real harm in letting open position with invalid symbol.

Symbol can be invalidated for various reasons:

If Muon gives no price data for the invalid symbol and user's position is opened, user's position cannot be closed and funds will be locked;
If Muon gives incorrect price data for the invalid symbol and user's position is opened, user's position can be closed but user will suffer a huge loss.

The already opened positions with invalid symbol will no doubt bring damages, that's exactly why we should not allow new positions to be opended with invalid symbol, only by doing that we can mitigate futher damages to the protocol.

panprog commented 1 year ago

The already opened positions with invalid symbol will no doubt bring damages, that's exactly why we should not allow new positions to be opended with invalid symbol, only by doing that we can mitigate futher damages to the protocol.

My point is that there is no way to remove already opened positions with invalid symbol. So if there are opened positions, it doesn't matter if we also let the pending quotes with this symbol be opened - there are the other opened positions anyway. So in this sense pending quotes are no worse than opened position, and since opened positions can not be mitigated anyway, pending quotes can't cause any harm not already present.

Now thinking of it, I guess the real bug is that there is no symbol "settlement" functionality - like if the price feed stops working for the symbol, or is about to be stopped, all positions with this symbol should be settled (closed) at the market (or some average) price. Maybe this should happen when symbol is set invalid. So the problem is not in letting quotes open, the problem is with not settling already opened positions. But this is not mentioned anywhere.

circlelooper commented 1 year ago

So the problem is not in letting quotes open, the problem is with not settling already opened positions. But this is not mentioned anywhere.

Actually this is metioned in the report:

It's possible that later, oracle will stop provide signatures with prices for that symbol, which means that position can be stucked.

I think this is very obvious: positions opened with invalid symbol -> opened positions with invalid symbol -> positions cannot be settled

The rootcause is the same, so the issue is valid.

panprog commented 1 year ago

The rootcause is the same, so the issue is valid.

Yes, I agree that issue is valid, but I think that impact is low, because fixing this issue won't improve the situation significantly: if pending -> open transition is prohibited for invalid symbol, there are still open positions with this invalid symbol, meaning nothing really changed.

The only scenario when fixing this issue will help is if there are no open positions, but there are pending quotes (for example: an incorrect symbol was accidently allowed, some users already started sending quotes, but the admin noticed this and invalidated the symbol). In such scenario, yes, fixing this issue will help. But it can be classified as admin mistake which should be invalid then. Or it can happen on a mature market, where all users have closed positions but there are still pending quotes (which is extremely unlikely but possible). I'm not sure if such scenario is enough for the medium impact.

circlelooper commented 1 year ago

if pending -> open transition is prohibited for invalid symbol, there are still open positions with this invalid symbol, meaning nothing really changed

They are essentially the same: If there are no opening positions, this issue leads to invalid symbol used in opening positions; If there are opening positions, this issue makes things worse and more funds are locked (oracle provides no price data) or lost (oracle provides compromised price data).

But it can be classified as admin mistake which should be invalid then.

It's not admin error, as I mentioned above: "symbol manager will have to update a symbol even if there are pending requests, because some requests may have no expiration date".

Opening quotes with invalid symbol leads to user's funds being locked or lost, it's enough for a medium.

MoonKnightDev commented 1 year ago

Fixed code PR link: https://github.com/SYMM-IO/symmio-core/pull/6

panprog commented 1 year ago

this issue makes things worse and more funds are locked (oracle provides no price data) or lost (oracle provides compromised price data).

Good point, agree. However, this can be fixed off-chain and I don't think funds will be stuck/lost due to this issue, because the protocol doesn't use the oracle feeds directly, and off-chain muon app can simply return and sign the same (settled) price for malfunctioning feed as a "settlement" measure for invalid symbols with invalid oracle feed.

However, @hrishibhat says that historically Sherlock rules consider issues that can be fixed off-chain to be valid medium, because they must be fixed on-chain. This issue can be fixed both off-chain and on-chain, so according to this rule it should probably be a valid medium.

In addition, I'd like to add that this issue is only part of the story. I think that the whole process of making a symbol invalid is just not well thought out by developers. They should think what to do with open positions. And the best fix should most probably be on-chain, something like:

Instead of valid/invalid they should make a symbol status like: invalid, active, closeonly, settled, paused
For certain statuses like "settled" admin should also be able to set the settlement price
If status is not active, new quotes should not be opened and pending/locked quotes should only be able to be cancelled
Open positions should be closed using fixed settlement price in settled status, be denied to be closed in paused and invalid status.

This issue only tackles point 3, but is still valid.

hrishibhat commented 1 year ago

Result: Medium Has duplicates After considering all the comments above and further internal discussion, there seems to be a lot of factors to be considered here and this issue can be considered on the borderline regarding the impact of the issue and that it can be handled on the muon app end. However, given some of the comments above, Sponsor's opinion, and the fix applied, this can be considered a valid issue as this can cause issues if positions are opened with some of these quotes as the check is currently only for send quotes.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

mstpr: accepted

sherlock-audit / 2023-06-symmetrical-judging

rvierdiiev - In case if symbol is not valid it should be not possible to open position #122

In case if symbol is not valid it should be not possible to open position

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation