search

sherlock-audit / 2023-12-flatmoney-judging

9 stars 8 forks source link

xiaoming90 - Long traders unable to withdraw their assets #196

Open sherlock-admin2 opened 5 months ago

sherlock-admin2 commented 5 months ago

xiaoming90

high

Long traders unable to withdraw their assets

Summary

Whenever the protocol reaches a state where the long trader's profit is larger than LP's stable collateral total, the protocol will be bricked. As a result, the margin deposited and gain of the long traders can no longer be withdrawn and the LPs cannot withdraw their collateral, leading to a loss of assets for the users.

Vulnerability Detail

Per Line 97 below, if the collateral balance is less than the tracked balance, the _getCollateralNet invariant check will revert.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/misc/InvariantChecks.sol#L97

File: InvariantChecks.sol
089:     /// @dev Returns the difference between actual total collateral balance in the vault vs tracked collateral
090:     ///      Tracked collateral should be updated when depositing to stable LP (stableCollateralTotal) or
091:     ///      opening leveraged positions (marginDepositedTotal).
092:     /// TODO: Account for margin of error due to rounding.
093:     function _getCollateralNet(IFlatcoinVault vault) private view returns (uint256 netCollateral) {
094:         uint256 collateralBalance = vault.collateral().balanceOf(address(vault));
095:         uint256 trackedCollateral = vault.stableCollateralTotal() + vault.getGlobalPositions().marginDepositedTotal;
096: 
097:         if (collateralBalance < trackedCollateral) revert FlatcoinErrors.InvariantViolation("collateralNet");
098: 
099:         return collateralBalance - trackedCollateral;
100:     }

Assume that:

Assume that Bob's long position gains a profit of 51 ETH.

The following actions will trigger the updateGlobalPositionData function internally: executeOpen, executeAdjust, executeClose, and liquidation.

When the FlatcoinVault.updateGlobalPositionData function is triggered to update the global position data:

profitLossTotal = 51 ETH (gain by long)

newMarginDepositedTotal = marginDepositedTotal + marginDelta + profitLossTotal
newMarginDepositedTotal = 50 ETH + 0 + 51 ETH = 101 ETH

_updateStableCollateralTotal(-51 ETH)
newStableCollateralTotal = stableCollateralTotal + _stableCollateralAdjustment
newStableCollateralTotal = 50 ETH + (-51 ETH) = -1 ETH
stableCollateralTotal = (newStableCollateralTotal > 0) ? newStableCollateralTotal : 0;
stableCollateralTotal = 0

In this case, the state becomes as follows:

Notice that the Collateral Balance and Tracked Balance are no longer in sync. As such, the revert will occur when the _getCollateralNet invariant checks are performed.

Whenever the protocol reaches a state where the long trader's profit is larger than LP's stable collateral total, this issue will occur, and the protocol will be bricked. The margin deposited and gain of the long traders can no longer be withdrawn from the protocol. The LPs also cannot withdraw their collateral.

The reason is that the _getCollateralNet invariant checks are performed in all functions of the protocol that can be accessed by users (listed below):

Impact

Loss of assets for the users. Since the protocol is bricked due to revert, the long traders are unable to withdraw their deposited margin and gain and the LPs cannot withdraw their collateral.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/misc/InvariantChecks.sol#L97

Tool used

Manual Review

Recommendation

Currently, when the loss of the LP is more than the existing stableCollateralTotal, the loss will be capped at zero, and it will not go negative. In the above example, the stableCollateralTotal is 50, and the loss is 51. Thus, the stableCollateralTotal is set to zero instead of -1.

The loss of LP and the gain of the trader should be aligned or symmetric. However, this is not the case in the current implementation. In the above example, the gain of traders is 51, while the loss of LP is 50, which results in a discrepancy here.

To fix the issue, the loss of LP and the gain of the trader should be aligned. For instance, in the above example, if the loss of LP is capped at 50, then the profit of traders must also be capped at 50.

Following is a high-level logic of the fix:

If (profitLossTotal > stableCollateralTotal): // (51 > 50) => True
    profitLossTotal = stableCollateralTotal // profitLossTotal = 50

newMarginDepositedTotal = marginDepositedTotal + marginDelta + profitLossTotal // 50 + 0 + 50 = 100

newStableCollateralTotal = stableCollateralTotal + (-profitLossTotal) // 50 + (-50) = 0
stableCollateralTotal = (newStableCollateralTotal > 0) ? newStableCollateralTotal : 0; // stableCollateralTotal = 0

The comment above verifies that the logic is working as intended.

sherlock-admin commented 5 months ago

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

takarez commented:

valid: high(6)

ydspa commented 5 months ago

Escalate

This should be a medium issue, as the likelihood is extreme low due to strict external market conditions

Flatcoin can be net short and ETH goes up 5x in a short period of time, potentially leading to UNIT going to 0. https://audits.sherlock.xyz/contests/132

Meet sherlock's rule for Medium

Causes a loss of funds but requires certain external conditions or specific states

But not meet the rule for High

Definite loss of funds without (extensive) limitations of external conditions

sherlock-admin2 commented 5 months ago

Escalate

This should be a medium issue, as the likelihood is extreme low due to strict external market conditions

Flatcoin can be net short and ETH goes up 5x in a short period of time, potentially leading to UNIT going to 0. https://audits.sherlock.xyz/contests/132

Meet sherlock's rule for Medium

Causes a loss of funds but requires certain external conditions or specific states

But not meet the rule for High

Definite loss of funds without (extensive) limitations of external conditions

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.

xiaoming9090 commented 5 months ago

Disagree with the escalation. The following point in the escalation is simply a remark by the protocol team stating that if the ETH goes up 5x, the value of UNIT token will go down to zero

Flatcoin can be net short and ETH goes up 5x in a short period of time, potentially leading to UNIT going to 0. https://audits.sherlock.xyz/contests/132

It has nothing to do with preventing the protocol from reaching a state where the long trader's profit is larger than LP's stable collateral total.

On the other hand, this point made by the protocol team actually reinforces the case I made in the report. The point by the protocol team highlighted the fact that the ETH can go up significantly over a short period of time. When this happens, the long trader's profit will be large. Thus, it will cause the protocol to reach a state where the long trader's profit is larger than LP's stable collateral total, which triggers this issue. When this happens, the protocol will be bricked, thus justified for a High.

Also, the requirement for High is "Definite loss of funds without (extensive) limitations of external conditions". This issue can occur without extensive external conditions as only one condition, which is the price of the ETH goes up significantly, is needed to trigger the bug. Thus, it meets the requirement of a High issue.

Czar102 commented 5 months ago

@xiaoming9090 aren't protocol risk parameters set not to allow the long traders' profits to be larger than LP funds?

xiaoming9090 commented 5 months ago

@xiaoming9090 aren't protocol risk parameters set not to allow the long traders' profits to be larger than LP funds?

@Czar102 The risk parameter I'm aware of is the skewFractionMax parameter, which prevents the system from having too many long positions compared to short positions. The maximum ratio of long to short position size is 1.2 (120% long: 100% long) during the audit. The purpose of limiting the number of long positions is to avoid the long side wiping out the value of the short side (LP) too quickly when the ETH price goes up.

However, I'm unaware of any restrictions constraining long traders' profits. The long traders' profits will increase along with the increase in ETH price.

Czar102 commented 5 months ago

So, the price needs to go up 5x in order to trigger this issue?

xiaoming9090 commented 5 months ago

@Czar102 Depending on the skewFractionMax parameter being configured at any single point of time. The owner can change this value via the setSkewFractionMax function. The higher the value is, the smaller the price increase needed to trigger the issue.

If the skewFractionMax is set to 20%, 5x will cause the LP's UNIT holding to drop to zero. Thus, slightly more than 5x will trigger this issue. Sidenote: The 20% is chosen in this report since it was mentioned in the contest's README

Czar102 commented 5 months ago

I think this is a fair assumption to have these values set so that other positions will practically never go into negative values – so this bug will practically never be triggered. Hence, the assumptions for this issue to be triggered are quite extensive.

Planning to accept the escalation and consider this issue a Medium severity one.

0xjuaan commented 5 months ago

So it seems that in order for this issue to be triggered, ETH has to rise 5x in a short period of time.

However in the 'known issues/acceptable risks that should not result in a valid finding' section of the contest README, it says:

theoretically, if ETH price increases by 5x in a short period of time whilst the flatcoin holders are 20% short, it's possible for flatcoin value to go to 0. This scenario is deemed to be extremely unlikely and the funding rate is able to move quickly enough to bring the flatcoin holders back to delta neutral.

Since that scenario is required to trigger this issue, this issue should not be deemed as valid.

xiaoming9090 commented 5 months ago

So it seems that in order for this issue to be triggered, ETH has to rise 5x in a short period of time.

However in the 'known issues/acceptable risks that should not result in a valid finding' section of the contest README, it says:

theoretically, if ETH price increases by 5x in a short period of time whilst the flatcoin holders are 20% short, it's possible for flatcoin value to go to 0. This scenario is deemed to be extremely unlikely and the funding rate is able to move quickly enough to bring the flatcoin holders back to delta neutral.

Since that scenario is required to trigger this issue, this issue should not be deemed as valid.

The README mentioned that the team is aware of a scenario where the price can go up significantly, leading the LP's Flatcoin value to go to 0. However, that does not mean that the team is aware of other potential consequences when this scenario happens.

0xjuaan commented 5 months ago

But surely if the sponsor deemed that the very rapid 5x price increase of ETH is extremely unlikely, that means that its an acceptable risk that they take on. So any issue which relies on that scenario is a part of that same acceptable risk, so shouldn't be valid right?

The sponsor clarified why they accept this risk and issues relating to this scenario shouldn't be reported, in this public discord message

Hello everyone. I believe some of you guys might have a doubt whether UNIT going to 0 is an issue or not. I believe it was mentioned that UNIT going to 0 is a known behaviour of the system but the reason might not be clear as to why. If the collateral price increases by let's say 5x (in case the skew limit is 120%), the entire short side (UNIT LPs) will be liquidated (though not liquidated in the same way as how leveraged longs are). The system will most likely not be able to recover as longs can simply close their positions and the take away the collateral. The hope is that this scenario doesn't play out in the future due to informed LPs and funding rate and other incentives but we know this is crypto and anything is possible here. Just wanted to clarify this so that we don't get this as a reported issue.

xiaoming9090 commented 5 months ago

The team is aware and has accepted that FlatCoin's value (short-side/LP) will go to zero when the price goes up significantly. However, that is different from this report where the long traders are unable to withdraw when the price goes up significantly.

These are two separate issues, and the root causes are different. The reason why the FlatCoin's value can go to zero is due to the design of the protocol where the short side can lose to the long side. However, this report and its duplicated reports highlighted a bug in the existing implementation that could lead to long traders being unable to withdraw.

nevillehuang commented 5 months ago

I think this is a fair assumption to have these values set so that other positions will practically never go into negative values – so this bug will practically never be triggered. Hence, the assumptions for this issue to be triggered are quite extensive.

Planning to accept the escalation and consider this issue a Medium severity one.

Agree to downgrade to medium severity based on dependency of large price increases.

Czar102 commented 5 months ago

Based on https://github.com/sherlock-audit/2023-12-flatmoney-judging/issues/196#issuecomment-1970315654, still planning to consider this a valid Medium.

Czar102 commented 4 months ago

Result: Medium Has duplicates

sherlock-admin2 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin commented 4 months ago

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/266.