search

sherlock-audit / 2023-06-symmetrical-judging

5 stars 4 forks source link

simon135 - `liquidatePendingPositionsPartyA` dosnt give fee back when PartyA has positions in pending and instead liquidates them which should'nt happen #338

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

simon135

high

liquidatePendingPositionsPartyA dosnt give fee back when PartyA has positions in pending and instead liquidates them which should'nt happen

Summary

so if PartyA has a position with 0 address or with positions that are in the pending state then they get liquidated there is no way to get trading fee/pending funds back even though the positions are pending/ in the spec and not accounted in liquidations

Vulnerability Detail

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L99 Steps: naturally through PartyA's own occurrences (like taking limit orders that haven't been filled yet for some reason like high price/low price) shouldn't be liquidated least PartyA should get the trading fees back or pendingLockedBalances Or if a big price movement happens This can cause unexpected loss of funds 

            if (
                (quote.quoteStatus == QuoteStatus.LOCKED ||
                    quote.quoteStatus == QuoteStatus.CANCEL_PENDING) &&
                quoteLayout.partyBPendingQuotes[quote.partyB][partyA].length > 0
            ) {
                delete quoteLayout.partyBPendingQuotes[quote.partyB][partyA]; 
                AccountStorage
                .layout()
                .partyBPendingLockedBalances[quote.partyB][partyA].makeZero(); 
            }
            quote.quoteStatus = QuoteStatus.LIQUIDATED; // @audit   M-14 see here that a position that is not locked or cancel_pending gets liquidated and there is nothing PartyA can call to fix this or to cancel the position
            quote.modifyTimestamp = block.timestamp;
        }
        AccountStorage.layout().pendingLockedBalances[partyA].makeZero();
        delete quoteLayout.partyAPendingQuotes[partyA];

Impact

Loss of funds for PartyA even if small for TradingFee but also makes pendingBalnace zero which is a loss that should not be accounted for. The reasons I would justify it being an issue is because even though a user can call requestToCancelQuote to cancel the position there is no documentation on this type of scenario and the user will not be able to call in 2 steps in liquidation so are we relying on PartyA to front run the liquidations? A: My basis probably for the sake of users this can cause loss for their pending Positions(with big price movement this can be more of a risk)

Code Snippet

     QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
        require(
            MAStorage.layout().liquidationStatus[partyA],
            "LiquidationFacet: PartyA is solvent"
        );
        for (uint256 index = 0; index < quoteLayout.partyAPendingQuotes[partyA].length; index++) {
            Quote storage quote = quoteLayout.quotes[
                quoteLayout.partyAPendingQuotes[partyA][index]
            ];
            if (
                (quote.quoteStatus == QuoteStatus.LOCKED ||
                    quote.quoteStatus == QuoteStatus.CANCEL_PENDING) &&
                quoteLayout.partyBPendingQuotes[quote.partyB][partyA].length > 0
            ) {
                delete quoteLayout.partyBPendingQuotes[quote.partyB][partyA];
                AccountStorage
                .layout()
                .partyBPendingLockedBalances[quote.partyB][partyA].makeZero();
            }
            quote.quoteStatus = QuoteStatus.LIQUIDATED;
            quote.modifyTimestamp = block.timestamp;
        }
        AccountStorage.layout().pendingLockedBalances[partyA].makeZero();
        delete quoteLayout.partyAPendingQuotes[partyA];
    }

Tool used

Manual Review

Recommendation

Add for those edge cases and give back at least the fee for the PartyA but I also think give back PendingBalance

Duplicate of #71