search

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

0 stars 0 forks source link

xiaoming90 - Deferred Liquidation can get stuck at step one of the liquidation process if the nonce increment #8

Open sherlock-admin2 opened 2 weeks ago

sherlock-admin2 commented 2 weeks ago

xiaoming90

High

Deferred Liquidation can get stuck at step one of the liquidation process if the nonce increment

Summary

Deferred Liquidation can get stuck at step one of the liquidation process if the nonce increments, leading to a loss of assets.

Vulnerability Detail

Assume the following:

  1. At T10, PartyA becomes liquidatable due to price movement, resulting in a drop in PnL
  2. At T20, PartyA is solvent again. This can be due to various reasons, such as users allocating more funds or the prices recovering. Note that in the new version of the protocol, allocating funds does not lead to an increase in the account's nonce.

[!NOTE]

Per the documentation, the purpose of deferred liquidation is to allow the liquidator to liquidate the user who was liquidated at a certain block (it can be the current block or a block in the past). Even if PartyA is solvent in the current block, the account can still be liquidated if it has been eligible for liquidation in the past. The excess collateral in the accounts will returned back to the users.

  1. Still at T20. A liquidator (Bob) is aware that PartyA is liquidatable at T10. Thus, he requests the deferred liquidation signature from Muon. At T10, PartyA's nonce is equal to 555. The liquidationId, liquidationTimestamp, partyANonces of the signature will be 100, T10, and 555, respectively. He proceeds to call the deferredLiquidatePartyA function with the signature, and the liquidationId of 100 is locked in the system. PartyA's liquidation status is set to true, and the account is technically "frozen".

  2. At T21, one of the PartyBs that PartyA trades with triggers the chargeFundingRate function against the PartyA to charge a funding rate (non-malicious event), or it can also be executed intentionally with malicious intention. Note that one PartyA can trade with multiple PartyBs. The function will check if PartyA is solvent, and the solvency check will pass because PartyA is solvent at this point (T21). At the end of the function, PartyA's nonce will increment to 556.

  3. The liquidator's deferredSetSymbolsPrice transaction gets executed at T22. It is normal to have a slight delay due to many reasons, such as network congestion, low gas, or some preparation needed. Since the PartyA's nonce on the liquidation signature (555) is different from the current PartyA's nonce (556), the liquidation signature (liquidationId=100) will no longer be considered valid, and it cannot be used to proceed with the second step (set symbol price) of the liquidation process. The signature check is performed here.

  4. If the liquidator attempts to fetch the liquidation signature for T10 from Muon again, PartyA's nonce of the signature will always remain at 555 because this is PartyA's nonce value at T10.

  5. As a result, liquidation for PartyA will be struck.

Impact

Loss of assets for the counterparty as the transfer of the assets from a liquidatable account to the counterparty cannot be made. The liquidation process cannot be completed as the liquidatable account is stuck.

In addition, since the liquidatable account is stuck, the assets under the accounts are also locked.

Code Snippet

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

After step one of the deferred liquidation process, Party A's nonce should not change under any circumstance. Otherwise, the liquidation will be stuck.

Upon review of the codebase, it was found that only the chargeFundingRate function can update PartyA's nonce after step one of the deferred liquidation process. Thus, consider adding the notLiquidatedPartyA modifier to the chargeFundingRate function to prevent anyone from charging the funding rate against a PartyA account already marked as liquidated to mitigate this issue.

    function chargeFundingRate(
        address partyA,
        uint256[] memory quoteIds,
        int256[] memory rates,
        PairUpnlSig memory upnlSig
+   ) external whenNotPartyBActionsPaused notLiquidatedPartyA(partyA) {
-   ) external whenNotPartyBActionsPaused {
        FundingRateFacetImpl.chargeFundingRate(partyA, quoteIds, rates, upnlSig);
        emit ChargeFundingRate(msg.sender, partyA, quoteIds, rates);
    }
sherlock-admin4 commented 1 week ago

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

Hash01011122 commented:

Low/Med, Watson assumes Deferred liquidation can get stuck because of nonce increment via network congestion, low gas or some additional preparation which is unrealistic to occur as most of the liquidation is done in a single block still there is room for this to occur. Probability of this happening is low

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/50

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.

The report has demonstrated how it could lead to asset loss or stuck under normal conditions. The liquidation process is divided into multiple stages/steps as follows:

There is no requirement for the liquidator to complete all the stages in one go within a single block, and this requirement is also not being enforced within the smart contracts. As such, liquidators have the option to execute each of the above stages in a single or multiple blocks as they see fit.

Thus, it is a valid scenario where Stage 2 of the liquidation process is executed on a different block from Stage 1 of the liquidation process, resulting in the issue highlighted in this report to occur.

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.

Whenever the liquidator does not execute Stage 1 and Stage 2 of the liquidation process within a single block, the issue mentioned in this report can occur, causing the user’s asset to be stuck. 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.

The report has demonstrated how it could lead to asset loss or stuck under normal conditions. The liquidation process is divided into multiple stages/steps as follows:

  • Stage 1 - deferredLiquidatePartyA
  • Stage 2 - deferredSetSymbolsPrice
  • Stage 3 - liquidatePendingPositionsPartyA
  • Stage 4 - liquidatePositionsPartyA

There is no requirement for the liquidator to complete all the stages in one go within a single block, and this requirement is also not being enforced within the smart contracts. As such, liquidators have the option to execute each of the above stages in a single or multiple blocks as they see fit.

Thus, it is a valid scenario where Stage 2 of the liquidation process is executed on a different block from Stage 1 of the liquidation process, resulting in the issue highlighted in this report to occur.

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.

Whenever the liquidator does not execute Stage 1 and Stage 2 of the liquidation process within a single block, the issue mentioned in this report can occur, causing the user’s asset to be stuck. 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.

WangSecurity commented 1 week ago

I need some clarification on how the loss of funds occurs, if the PartyA was liquidatable at T10, but is not liquidatable at T20, what is the loss of funds here exactly? Maybe a numeric example would be perfect here @xiaoming9090



Additionally, are PartyB and PartyA roles or contracts and as I understand they’re considered Trusted?

xiaoming9090 commented 1 week ago

I need some clarification on how the loss of funds occurs, if the PartyA was liquidatable at T10, but is not liquidatable at T20, what is the loss of funds here exactly? Maybe a numeric example would be perfect here @xiaoming9090



Additionally, are PartyB and PartyA roles or contracts and as I understand they’re considered Trusted?

@WangSecurity

Question: how the loss of funds occurs Answer: When this issue is realized, the liquidation process will be stuck, and the account will be frozen. No one (users or liquidators) can retrieve the assets in the "frozen" account. Thus, all assets within the frozen account are effectively lost. If the "frozen" account has $10000 worth of assets, this amount will be lost.

Question: if the PartyA was liquidatable at T10, but is not liquidatable at T20 Answer: To simplify, it is a special new feature of Symm IO that allows a liquidator to "go back in time". Assume Bob's account becomes liquidatable for a moment due to volatile market movement (e.g., at T10). The price movement goes down and then up within a short period of time. Thus, when the liquidators detect it at T20, the account might be solvent again since the price has recovered. However, that does not matter for the liquidator because they are allowed to liquidate Bob's account based on old historical data at T10.

Question: PartyB and PartyA roles or contracts and as I understand they’re considered Trusted?

WangSecurity commented 6 days ago

Thank you very much! And another question on the scenario, if we change it a bit to the following:

  1. At T20, PartyB calls chargeFundingRate, changing the Nonce to 556.
  2. At 21, Bob (liquidator) will request the deferred liquidation signature from Muon. But the signature will return the nonce of 555, correct?
xiaoming9090 commented 6 days ago

Thank you very much! And another question on the scenario, if we change it a bit to the following:

  1. At T20, PartyB calls chargeFundingRate, changing the Nonce to 556.
  2. At 21, Bob (liquidator) will request the deferred liquidation signature from Muon. But the signature will return the nonce of 555, correct?

@WangSecurity

Firstly, a PartyA can interact with many PartyBs within the system. At any point, each PartyB can call the chargeFundingRate function.

Secondly, the steps in the POC should not be swapped. The timing and sequence of each step are important for the edge case to be triggered. The report presents an edge case that will happen when the chargeFundingRate function is triggered after the deferredLiquidatePartyA function, not the other way around.

On a side note, the sponsor has accepted the technical aspect of the POC, so there is no doubt about its correctness and the possibility of this edge case occurring. Otherwise, a fix would not be required if the edge case cannot occur.

The reason why this has been wrongly judged as Low in the first place is due to the following comments:

Low/Med, Watson assumes Deferred liquidation can get stuck because of nonce increment via network congestion, low gas or some additional preparation which is unrealistic to occur as most of the liquidation is done in a single block still there is room for this to occur. Probability of this happening is low

However, in Sherlock, issues are not judged based on the probability that an issue can happen.

WangSecurity commented 6 days ago

Thank you, I agree that even though this case might be highly unlikely, it's viable and should be deemed valid. Planning to accept the escalation and validate the report with medium severity, since the constraints for this scenario are quite high.

And @xiaoming9090 just for your info, the comment from a judge you quoted is not from the Lead Judge, but from another Judge from the contest. That doesn't affect anything, just making sure you're aware of that.

WangSecurity commented 5 days ago

Result: Medium Unique

sherlock-admin4 commented 5 days ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin2 commented 3 days ago

The Lead Senior Watson signed off on the fix.