xiaoming90 - Long trader's deposited margin can be wiped out

[sherlock-admin]

xiaoming90

high

Long trader's deposited margin can be wiped out

Summary

Long Trader's deposited margin can be wiped out due to a logic error, leading to a loss of assets.

Vulnerability Detail

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L232

File: FlatcoinVault.sol
216:     function settleFundingFees() public returns (int256 _fundingFees) {
..SNIP..
226:         // Calculate the funding fees accrued to the longs.
227:         // This will be used to adjust the global margin and collateral amounts.
228:         _fundingFees = PerpMath._accruedFundingTotalByLongs(_globalPositions, unrecordedFunding);
229: 
230:         // In the worst case scenario that the last position which remained open is underwater,
231:         // we set the margin deposited total to 0. We don't want to have a negative margin deposited total.
232:         _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
233:             ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
234:             : 0;
235: 
236:         _updateStableCollateralTotal(-_fundingFees);

Issue 1

Assume that there are two long positions in the system and the _globalPositions.marginDepositedTotal is $X$.

Assume that the funding fees accrued to the long positions at Line 228 is $Y$. $Y$ is a positive value indicating the overall gain/profit that the long traders received from the LPs.

In this case, the _globalPositions.marginDepositedTotal should be set to $(X + Y)$ after taking into consideration the funding fee gain/profit accrued by the long positions.

However, in this scenario, $X < Y$​. Thus, the condition at Line 232 will be evaluated as false, and the _globalPositions.marginDepositedTotal will be set to zero. This effectively wipes out all the margin collateral deposited by the long traders in the system, and the deposited margin of the long traders is lost.

Issue 2

The second issue with the current implementation is that it does not accurately capture scenarios where the addition of _globalPositions.marginDepositedTotal and _fundingFees result in a negative number. This is because _fundingFees could be a large negative number that, when added to _globalPositions.marginDepositedTotal, results in a negative total, but the condition at Line 232 above still evaluates as true, resulting in an underflow revert.

Impact

Loss of assets for the long traders as mentioned above.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L232

Tool used

Manual Review

Recommendation

If the intention is to ensure that _globalPositions.marginDepositedTotal will never become negative, consider summing up $(X + Y)$​ first and determine if the result is less than zero. If yes, set the _globalPositions.marginDepositedTotal to zero.

The following is the pseudocode:

newMarginTotal = globalPositions.marginDepositedTota + _fundingFees;
globalPositions.marginDepositedTotal = newMarginTotal > 0 ? uint256(newMarginTotal) : 0;

[sherlock-admin]

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

ubl4nk commented:

invalid -> when marginDepositedTotal is less than Zero, all the long-traders all liquidated + some loss for LPs where that position is liquidated a bit late.

takarez commented:

valid: same fix with issue 181; high(2)

[sherlock-admin]

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

[midori-fuse]

Escalate

181 is a dupe of this issue because:

• This report mentions a general scenario where the calculation in line 232 fails to accurately account for either outcome of the ternary operation.
• "Issue 2" of this report mentions the same scenario as with 181, albeit less generalized.
• The fixes are identical.

Furthermore, report #78 is the report that accurately spells out the full problem with the current implementation (it encompasses both of #195 and #181), and thus all dupes of the given two issues should be duped under it.

[sherlock-admin2]

Escalate

181 is a dupe of this issue because:

• This report mentions a general scenario where the calculation in line 232 fails to accurately account for either outcome of the ternary operation.
• "Issue 2" of this report mentions the same scenario as with 181, albeit less generalized.
• The fixes are identical.

Furthermore, report #78 is the report that accurately spells out the full problem with the current implementation (it encompasses both of #195 and #181), and thus all dupes of the given two issues should be duped under it.

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]

Disagree with the escalation. Report 181 pointed out an issue due to an underflow bug when performing the casting. This report (195) pointed out a logical error in the math operation. Thus, they are two distinct issues.

On a side note, I have seen a few escalations regarding the sponsor's similar fixes (Same PR); thus, those issues with the same PR fix should be duplicated. However, the PR 275 (https://github.com/dhedge/flatcoin-v1/pull/275) is private. No one at this point apart from the sponsor knows the fix's implementation. One PR/commit can contain many different fixes for multiple reports. Also, we have not done the fix review yet, so one cannot conclude that the fix implemented is correct at this point. Thus, any argument regarding the same PR fix == duplicate in all the escalations in this contest is not well established.

[midori-fuse]

Thank you for your input. I edited my escalation to add some more points.

I do agree however that the same fix does not warrant the issue being a dupe.

[0xjuaan]

Disagree with the escalation. Report 181 pointed out an issue due to an underflow bug when performing the casting. This report (195) pointed out a logical error in the math operation. Thus, they are two distinct issues.

But the underflow error in report 181 is due to the same logical error in the math. Simply, the logic improperly handles the case where abs(_fundingFees) >= _globalPositions.marginDepositedTotal. While there are 2 separate impacts of this bad logic, the root cause is the exact same. It is explained in detail in issue #78

Also @nevillehuang , if these two issues (#181 and #195) end up being considered as two separate issues, shouldn't that mean that my report (#78) would come under both issues and be rewarded as such? Since I covered both impacts in detail, under the same root cause. But now if the two impacts are being paid separately, then I believe that my report should be paid for both impacts.

[nevillehuang]

@0xjuaan The root causes seems to be different though as pointed out by @xiaoming9090. But I agree that your issue points out both scenarios. Even though the same fix PR is applied, I believe the fix would involve different logic

1. Underflow due to erroneous casting
2. Logical error in math operations

[0xjuaan]

But the underflow occurs due to the logical error in math.

I believe the fix would involve different logic

The fix shown in #78 (and also in this issue's recommendation) is sufficient to fix both issues, since it mitigates the core math error (incorrect handling of abs(_fundingFees) >= _globalPositions.marginDepositedTotal) that is responsible for the 2 separate impacts.

Anyway, if you still don't agree- is it possible to add my submission to this issue?

[nevillehuang]

@rashtrakoff would it be ok to allow viewing the fix involved in this issue?

@0xjuaan I don’t believe sherlock has made this exception before. I will review the fix and come to a decision

[rashtrakoff]

@nevillehuang let me know if you need any access for fix PR.

[Czar102]

I believe these submissions present a single issue of using a formula that would prevent an underflow for unsigned integer subtraction instead of signed integer addition. Using a wrong formula to have several code fragments wrong is a single mistake.

Planning to consider #181 a duplicate of this issue and accept the escalation.

[nevillehuang]

After reviewing the fix (it is singular), I agree with @Czar102.

[Czar102]

Result: High Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: accepted

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.