sherlock-admin4 commented 2 weeks ago

xiaoming90

High

PartyA's allocated balance could increase after `deferredLiquidatePartyA` is executed

Summary

PartyA's allocated balance could increase after deferredLiquidatePartyA is executed in an edge case, which could result in loss of assets.

Vulnerability Detail

When the deferredLiquidatePartyA function is executed, if there is any excess allocated balance in accountLayout.allocatedBalances[partyA], they will be transferred to the accountLayout.partyAReimbursement[partyA] at Line 42 below. Afterwards, the accountLayout.allocatedBalances[partyA] should not increase under any circumstance. Otherwise, an accounting error will occur.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/DeferredLiquidationFacetImpl.sol#L22

File: DeferredLiquidationFacetImpl.sol
22:     function deferredLiquidatePartyA(address partyA, DeferredLiquidationSig memory liquidationSig) internal {
23:         MAStorage.Layout storage maLayout = MAStorage.layout();
24:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
25: 
26:         LibMuon.verifyDeferredLiquidationSig(liquidationSig, partyA);
27: 
28:         int256 liquidationAvailableBalance = LibAccount.partyAAvailableBalanceForLiquidation(
29:             liquidationSig.upnl,
30:             liquidationSig.liquidationAllocatedBalance,
31:             partyA
32:         );
33:         require(liquidationAvailableBalance < 0, "LiquidationFacet: PartyA is solvent");
34: 
35:         int256 availableBalance = LibAccount.partyAAvailableBalanceForLiquidation(
36:             liquidationSig.upnl,
37:             accountLayout.allocatedBalances[partyA],
38:             partyA
39:         );
40:         if (availableBalance > 0) {
41:             accountLayout.allocatedBalances[partyA] -= uint256(availableBalance);
42:             accountLayout.partyAReimbursement[partyA] += uint256(availableBalance);
43:         }

However, there is an edge case where the accountLayout.allocatedBalances[partyA] can increase after the deferredLiquidatePartyA function is executed.

Assume Bob is a liquidator and a PartyA at the same time. As PartyA is permissionless, anyone can be a PartyA. There is no validation check in the codebase that prevents a liquidator from also being a PartyA AND no rules stated on the contest page that a liquidator cannot be a PartyA. Thus, it is fair to assume that some liquidators might use the same wallet to trade on the Symm App simultaneously (Users of Symm App = PartyA).

At T1, Bob's liquidator account took part in some liquidation process.
At T2, Bob's PartyA account gets liquidated by the deferredLiquidatePartyA function (Step 1 of liquidation process), and his accountLayout.allocatedBalances[Bob] has been set to zero at Line 41 above.
At T3. Some liquidations have been settled, and the liquidation fees are transferred to accountLayout.allocatedBalances[Bob] as per the code at Line 297-298 below. Assume that 1000 is paid out as a liquidation fee to Bob. Thus, accountLayout.allocatedBalances[Bob] = 1000 now.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L297

File: LiquidationFacetImpl.sol
295:             uint256 lf = accountLayout.liquidationDetails[partyA].liquidationFee;
296:             if (lf > 0) {
297:                 accountLayout.allocatedBalances[accountLayout.liquidators[partyA][0]] += lf / 2;
298:                 accountLayout.allocatedBalances[accountLayout.liquidators[partyA][1]] += lf / 2;
299:                 emit SharedEvents.BalanceChangePartyA(accountLayout.liquidators[partyA][0], lf / 2, SharedEvents.BalanceChangeType.LF_IN);
300:                 emit SharedEvents.BalanceChangePartyA(accountLayout.liquidators[partyA][1], lf / 2, SharedEvents.BalanceChangeType.LF_IN);
301:             }

accountLayout.allocatedBalances[Bob] = 1000 at this point. Next, the deferredSetSymbolsPrice function (Step 2 of liquidation process) will called against Bob's PartyA account.
The deferredSetSymbolsPrice is designed to always expect the availableBalance at Line 76 to be zero or negative to work properly. If availableBalance is positive, the logic and accounting will be incorrect. Because the accountLayout.allocatedBalances[Bob] = 1000, the availableBalance will become a positive value. Let's assume that availableBalance returned from LibAccount.partyAAvailableBalanceForLiquidation function at Line 76 is 800 after factoring the PnL (loss and/or deficit)
In Line 80 below, the remainingLf will be evaluated as 900. The liquidation fee of PartyA account will unexpectedly increase from 100 to 900, which is incorrect regarding the system's accounting. The liquidators should only receive up to accountLayout.lockedBalances[partyA].lf (100) of liquidation and not more than that amount. However, in this case, the liquidators received more than expected (900 instead of 100).

remainingLf = accountLayout.lockedBalances[partyA].lf - (- availableBalance);
remainingLf = 100 - (-800);
remainingLf = 900

As a result, 800 of the liquidation fee, which was supposed to belong to Bob, went to the liquidator's wallet. In this scenario, Bob lost 800.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/DeferredLiquidationFacetImpl.sol#L62

File: DeferredLiquidationFacetImpl.sol
62:     function deferredSetSymbolsPrice(address partyA, DeferredLiquidationSig memory liquidationSig) internal {
63:         MAStorage.Layout storage maLayout = MAStorage.layout();
64:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
65: 
66:         LibMuon.verifyDeferredLiquidationSig(liquidationSig, partyA);
67:         require(maLayout.liquidationStatus[partyA], "LiquidationFacet: PartyA is solvent");
68: 
69:         LiquidationDetail storage detail = accountLayout.liquidationDetails[partyA];
70:         require(keccak256(detail.liquidationId) == keccak256(liquidationSig.liquidationId), "LiquidationFacet: Invalid liquidationId");
71: 
72:         for (uint256 index = 0; index < liquidationSig.symbolIds.length; index++) {
73:             accountLayout.symbolsPrices[partyA][liquidationSig.symbolIds[index]] = Price(liquidationSig.prices[index], detail.timestamp);
74:         }
75: 
76:         int256 availableBalance = LibAccount.partyAAvailableBalanceForLiquidation(liquidationSig.upnl, accountLayout.allocatedBalances[partyA], partyA);
77: 
78:         if (detail.liquidationType == LiquidationType.NONE) {
79:             if (uint256(- availableBalance) < accountLayout.lockedBalances[partyA].lf) {
80:                 uint256 remainingLf = accountLayout.lockedBalances[partyA].lf - uint256(- availableBalance);
81:                 detail.liquidationType = LiquidationType.NORMAL;
82:                 detail.liquidationFee = remainingLf;
83:             } else if (uint256(- availableBalance) <= accountLayout.lockedBalances[partyA].lf + accountLayout.lockedBalances[partyA].cva) {
84:                 uint256 deficit = uint256(- availableBalance) - accountLayout.lockedBalances[partyA].lf;
85:                 detail.liquidationType = LiquidationType.LATE;
86:                 detail.deficit = deficit;
87:             } else {
88:                 uint256 deficit = uint256(- availableBalance) - accountLayout.lockedBalances[partyA].lf - accountLayout.lockedBalances[partyA].cva;
89:                 detail.liquidationType = LiquidationType.OVERDUE;
90:                 detail.deficit = deficit;
91:             }
92:             accountLayout.liquidators[partyA].push(msg.sender);
93:         }
94:     }

Impact

Loss of assets as shown in the above scenario.

Code Snippet

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/DeferredLiquidationFacetImpl.sol#L22

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L297

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/liquidation/DeferredLiquidationFacetImpl.sol#L62

Tool used

Manual Review

Recommendation

Implement the following additional logic to handle any edge case where PartyA's allocated balance could increase after deferredLiquidatePartyA (Step 1 of the liquidation process) is executed.

    function deferredSetSymbolsPrice(address partyA, DeferredLiquidationSig memory liquidationSig) internal {
        MAStorage.Layout storage maLayout = MAStorage.layout();
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();

        LibMuon.verifyDeferredLiquidationSig(liquidationSig, partyA);
        require(maLayout.liquidationStatus[partyA], "LiquidationFacet: PartyA is solvent");

        LiquidationDetail storage detail = accountLayout.liquidationDetails[partyA];
        require(keccak256(detail.liquidationId) == keccak256(liquidationSig.liquidationId), "LiquidationFacet: Invalid liquidationId");

        for (uint256 index = 0; index < liquidationSig.symbolIds.length; index++) {
            accountLayout.symbolsPrices[partyA][liquidationSig.symbolIds[index]] = Price(liquidationSig.prices[index], detail.timestamp);
        }

        int256 availableBalance = LibAccount.partyAAvailableBalanceForLiquidation(liquidationSig.upnl, accountLayout.allocatedBalances[partyA], partyA);

        if (detail.liquidationType == LiquidationType.NONE) {
+               if (availableBalance > accountLayout.lockedBalances[partyA].lf) {
+                uint256 remainingLf = accountLayout.lockedBalances[partyA].lf;
+                detail.liquidationType = LiquidationType.NORMAL;
+                detail.liquidationFee = remainingLf;
+               } else if (uint256(- availableBalance) < accountLayout.lockedBalances[partyA].lf) {
-           if (uint256(- availableBalance) < accountLayout.lockedBalances[partyA].lf) {
                uint256 remainingLf = accountLayout.lockedBalances[partyA].lf - uint256(- availableBalance);
                detail.liquidationType = LiquidationType.NORMAL;
                detail.liquidationFee = remainingLf;
            } else if (uint256(- availableBalance) <= accountLayout.lockedBalances[partyA].lf + accountLayout.lockedBalances[partyA].cva) {
                uint256 deficit = uint256(- availableBalance) - accountLayout.lockedBalances[partyA].lf;
                detail.liquidationType = LiquidationType.LATE;
                detail.deficit = deficit;
            } else {
                uint256 deficit = uint256(- availableBalance) - accountLayout.lockedBalances[partyA].lf - accountLayout.lockedBalances[partyA].cva;
                detail.liquidationType = LiquidationType.OVERDUE;
                detail.deficit = deficit;
            }
            accountLayout.liquidators[partyA].push(msg.sender);
        }
    }

sherlock-admin2 commented 1 week ago

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

Hash01011122 commented:

Low/Medium Vuln, Chances of occurrence of mentioned edge case is nearly zero.

sherlock-admin2 commented 1 week ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/SYMM-IO/protocol-core/pull/48

xiaoming9090 commented 1 week ago

Escalate

This issue should be at least a Medium Risk due to its significant impact (Loss of assets) if the risk is realized.

Per the protocol’s source code, there is nothing to prevent a liquidator from also being a user (PartyA) since the protocol allows anyone to trade on its system. The PartyA role is permissionless and open to anyone. Also, no rules state on the contest page that a liquidator cannot be a PartyA or trade on the system.

Thus, it is entirely a valid scenario where a user is a liquidator and a PartyA simultaneously, leading to the issue mentioned in the report.

Per Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Thus, it meets Sherlock's requirement of a Medium Risk, where it causes a loss of funds but requires certain external conditions or specific states.

sherlock-admin3 commented 1 week ago

Escalate

This issue should be at least a Medium Risk due to its significant impact (Loss of assets) if the risk is realized.

Per the protocol’s source code, there is nothing to prevent a liquidator from also being a user (PartyA) since the protocol allows anyone to trade on its system. The PartyA role is permissionless and open to anyone. Also, no rules state on the contest page that a liquidator cannot be a PartyA or trade on the system.

Thus, it is entirely a valid scenario where a user is a liquidator and a PartyA simultaneously, leading to the issue mentioned in the report.

Per Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Thus, it meets Sherlock's requirement of a Medium Risk, where it causes a loss of funds but requires certain external conditions or specific states.

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.

MxAxM commented 1 week ago

Possibility of this issue is near to zero so it should be low since it's not viable

xiaoming9090 commented 1 week ago

Possibility of this issue is near to zero so it should be low since it's not viable

Disagree. As mentioned in the report and escalation's comment, it is entirely a valid scenario where a user is a liquidator and a PartyA simultaneously, leading to the issue mentioned in the report.

In Sherlock, edge cases that have an impact that could potentially lead to a loss of assets are always judged as H/M. This view is derived with Sherlock's judging rule (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue) below:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.

WangSecurity commented 1 week ago

I think I need clarification on the attack path. I'll show how I see it and please correct me if it's wrong or not:

Bob is partyA.
Bob is liquidated by Alice and his balance is set at 0.
Bob does some liquidations himself and gets a liquidation fee of $1000 in total.
Alice calls deferredSetSymbolsPrice and after that Alice receives all the liquidation fees that Bob received above +- PnL, correct?
Bob loses these funds and Alice gets them (as I understand, doesn't even steal them necessarily).

If we simplify the attack path as much as possible, is it correct?

xiaoming9090 commented 1 week ago

I think I need clarification on the attack path. I'll show how I see it and please correct me if it's wrong or not:

Bob is partyA.

Bob is liquidated by Alice and his balance is set at 0.

Bob does some liquidations himself and gets a liquidation fee of $1000 in total.

Alice calls deferredSetSymbolsPrice and after that Alice receives all the liquidation fees that Bob received above +- PnL, correct?

Bob loses these funds and Alice gets them (as I understand, doesn't even steal them necessarily).

If we simplify the attack path as much as possible, is it correct?

@WangSecurity

Each PartyA's account has a liquidation fee (LF) component, which is reserved and locked.

Refer to explanation of Liquidation Fee (LF) at https://github.com/SYMM-IO/protocol-core?tab=readme-ov-file#sendquote
Refer to the diagram at https://github.com/SYMM-IO/protocol-core?tab=readme-ov-file#liquidate-party-a where the blue section refer to the locked Liquidation Fee of a PartyA's account.

The PartyA's locked liquidation fee (LF) is the prize that will be paid to the liquidator. In my scenario, the locked LF of Bob's PartyA account is \$100. Thus, any liquidator that liquidates Bob's PartyA is entitled to a maximum of $100, as per the protocol's specification. This liquidator fee was agreed upon by both the PartyA (maker) and PartyB (taker) when they opened the position and finalized within the protocol.

The protocol specification already clearly dictates that any liquidator who liquidates Bob's PartyA is entitled to a maximum of \$100, which is PartyA's locked liquidation fee (LF). However, in the scenario mentioned in the report, the liquidator (Alice) received \$900 instead of \$100, which deviates from the protocol specification.

The liquidator (Alice) took more than what was expected from Bob's account. When a liquidator liquidates Bob's account, they are not entitled to all the assets left in the account. They are only entitled to \$100 and not anything more than that.

WangSecurity commented 6 days ago

Thank you for that clarification, but there's still one part I might be missing. In your scenario, after Alice liquidates Bob (PartyA), and before she receives these $100 of LF, Bob also gets $1000. Is it because he liquidated the previous PartyA or other users?

xiaoming9090 commented 6 days ago

Thank you for that clarification, but there's still one part I might be missing. In your scenario, after Alice liquidates Bob (PartyA), and before she receives these $100 of LF, Bob also gets $1000. Is it because he liquidated the previous PartyA or other users?

@WangSecurity

Firstly, the term "PartyA" is the same as "User/Other Users". Users are normal people who trade on Symm platform and anyone with a blockchain wallet can do so (permissionless).

Secondly, Alice did not receive $100 in my scenario. Instead, she receives $900, which is incorrect and much more than expected. This amount exceeds the maximum allowed liquidation fee of $100 she is entitled. Effectively, $800 is "stolen" from Bob and Bob lost $800.

Following are the timeline of the events for reference:

WangSecurity commented 6 days ago

Thank you, but I'm not sure if it answers my question.

In T3, it says "Some liquidations have settled and Bob receives 1000 as a liquidation fee". It's Bob liquidating other users correct? Just need to clarify where these funds come from.

xiaoming9090 commented 6 days ago

Thank you, but I'm not sure if it answers my question.

In T3, it says "Some liquidations have settled and Bob receives 1000 as a liquidation fee". It's Bob liquidating other users correct? Just need to clarify where these funds come from.

@WangSecurity

At T1, Bob (Liquidator) kick-off the liquidation process against other users (e.g., James and Jack). James has 500 locked as LF, and Jack also has 500 locked as LF.
At T3, the liquidations have settled, and the locked LF of James and Jack, which totals 1000, will be transferred to Bob. In short, the funds come from James and Jack, which Bob gains as compensation for the service he has provided in liquidating them.

WangSecurity commented 6 days ago

Thank you very much. I agree that this scenario is viable, even though it's very unlikely to occur, it's still possible with high constraints. Planning to accept the escalation and validate the issue with medium severity.

WangSecurity commented 5 days ago

Result: Medium Unique

sherlock-admin2 commented 5 days ago

Escalations have been resolved successfully!

Escalation status:

xiaoming9090: accepted

sherlock-admin2 commented 3 days ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-06-symmetrical-update-2-judging

xiaoming90 - PartyA's allocated balance could increase after `deferredLiquidatePartyA` is executed #6

PartyA's allocated balance could increase after `deferredLiquidatePartyA` is executed

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-06-symmetrical-update-2-judging

xiaoming90 - PartyA's allocated balance could increase after `deferredLiquidatePartyA` is executed #6

PartyA's allocated balance could increase after deferredLiquidatePartyA is executed

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

PartyA's allocated balance could increase after `deferredLiquidatePartyA` is executed