santipu_ - Invariants will fail when funding fee sets `stableCollateralTotal` to zero

[sherlock-admin4]

santipu_

high

Invariants will fail when funding fee sets stableCollateralTotal to zero

Summary

Since the new changes to the code, now the PnL of the leveraged positions is not settled until the individual positions are closed. This will cause the variable stableCollateralTotal to be set to zero by the funding fees (even though the collateral owned by LPs is not zero), causing a discrepancy between the tracked balances and the new balances. This discrepancy will cause the invariant checks to revert, causing a DoS on all orders and liquidations.

Vulnerability Detail

Imagine the following scenario, the price of ETH goes down significantly, causing an increase in the collateral owned by the LPs. If traders don't close their positions, the losses from the positions won't be realized and the variable stableCollateralTotal won't represent the current collateral owned by the LPs. Then, is possible that some LPs withdraw liquidity, driving stableCollateralTotal to almost zero (even though the actual collateral owned by the LPs is way bigger because of the unrealized loss of the traders).

In the scenario given above, if we add that the funding fee is paying traders when the function settleFundingFees is called (always before any operation), the value of stableCollateralTotal will be negative, but is capped at zero so it will be set to zero:

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L442-L448

    function _updateStableCollateralTotal(int256 _stableCollateralAdjustment) private {
        int256 newStableCollateralTotal = int256(stableCollateralTotal) + _stableCollateralAdjustment;

        // The stable collateral shouldn't be negative as the other calculations which depend on this
        // will behave in unexpected manners.
        stableCollateralTotal = (newStableCollateralTotal > 0) ? uint256(newStableCollateralTotal) : 0;
    }

Capping stableCollateralTotal at zero will cause a desync between stableCollateralTotal and marginDepositedTotal, causing a discrepancy between the tracked funds (stableCollateralTotal + marginDepositedTotal) and the actual funds (collateral.balanceOf(vault)).

When this discrepancy is given, the invariant checks that are executed before and after any operation will fail, reverting the whole transaction. Theses checks are the following:

First, we get the collateral net before and after the operation: https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/misc/InvariantChecks.sol#L94-L102

    function _getCollateralNet(IFlatcoinVault vault) private view returns (int256 netCollateral) {
        int256 collateralBalance = int256(vault.collateral().balanceOf(address(vault)));
        int256 trackedCollateral = int256(vault.stableCollateralTotal()) +
            vault.getGlobalPositions().marginDepositedTotal;

        if (collateralBalance < trackedCollateral) revert FlatcoinErrors.InvariantViolation("collateralNet1");

        return collateralBalance - trackedCollateral;
    }

Then, we compare that the collateral net before and after the operation doesn't differ too much, only allowing a difference of 1e6 for rounding: https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/misc/InvariantChecks.sol#L109-L115

    /// @dev Collateral balance changes should match tracked collateral changes
    function _collateralNetBalanceRemainsUnchanged(int256 netBefore, int256 netAfter) private pure {
        // Note: +1e6 to account for rounding errors.
        // This means we are ok with a small margin of error such that netAfter - 1e6 <= netBefore <= netAfter.
        if (netBefore > netAfter || netAfter > netBefore + 1e6)
            revert FlatcoinErrors.InvariantViolation("collateralNet2");
    }

The key is that the first collateral net is got before the function settleFundingFees is executed. When stableCollateralTotal is near zero and the funding fee is paying traders with a value bigger than 1e6, the invariant will revert because of the discrepancy on the collateral net.

Sidenote: After some thinking, I've realized that when stableCollateralTotal is near zero, there are more causes apart from the funding fee that can bring stableCollateralTotal to a negative value, capping it at zero and causing the same DoS. The funding fee is only ONE scenario where stableCollateralTotal can be capped at zero, other scenarios could be adjusting or closing a position, realizing some profits.

Impact

When the funding fee (or other cause) sets the variable stableCollateralTotal to a negative value, capping it at zero, the invariant checks will fail, causing a DoS on the executeOrder and liquidate functions. The highest impact will be the DoS on the liquidate function, because it will prevent unhealthy positions from being liquidated, causing bad debt on the market.

Code Snippet

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L447

Tool used

Manual Review

Recommendation

It's recommended to always settle the funding fees before the execution of the invariant checks.

    modifier orderInvariantChecks(IFlatcoinVault vault) {
        IStableModule stableModule = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY));
+       vault.settleFundingFees();

        // ...
    }

    modifier liquidationInvariantChecks(IFlatcoinVault vault, uint256 tokenId) {
        IStableModule stableModule = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY));
+       vault.settleFundingFees();

        // ...
    }

Also, I'd recommend allowing the variable stableCollateralTotal to be set to a negative value to deal with these edge cases. That is because there may be more actions during an operation that may bring stableCollateralTotal to zero, causing the invariant checks to revert.

Duplicate of #8

[sherlock-admin2]

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

santipu_ commented:

High

[santipu03]

Escalate

This issue shouldn't be duplicated with #8 and should be valid.

Issue #8 was marked as invalid because it's a known issue that UNIT goes to zero. However, my report correctly describes a scenario where stableCollateralTotal can be set to zero without the need for UNIT to go to zero.

The only constraint to the scenario I describe in my report is that when LPs try to withdraw liquidity, the transaction will fail due to checkMaxSkew. But this limitation is indeed a bug (described here #9), that if fixed, will allow LPs to withdraw when traders have unrealized losses, triggering the failure of invariants on liquidations.

[sherlock-admin2]

Escalate

This issue shouldn't be duplicated with #8 and should be valid.

Issue #8 was marked as invalid because it's a known issue that UNIT goes to zero. However, my report correctly describes a scenario where stableCollateralTotal can be set to zero without the need for UNIT to go to zero.

The only constraint to the scenario I describe in my report is that when LPs try to withdraw liquidity, the transaction will fail due to checkMaxSkew. But this limitation is indeed a bug (described here #9), that if fixed, will allow LPs to withdraw when traders have unrealized losses, triggering the failure of invariants on liquidations.

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.

[Bauchibred]

Escalate

This issue shouldn't be duplicated with #8 and should be valid.

Issue #8 was marked as invalid because it's a known issue that UNIT goes to zero. However, my report correctly describes a scenario where stableCollateralTotal can be set to zero without the need for UNIT to go to zero.

The only constraint to the scenario I describe in my report is that when LPs try to withdraw liquidity, the transaction will fail due to checkMaxSkew. But this limitation is indeed a bug (described here #9), that if fixed, will allow LPs to withdraw when traders have unrealized losses, triggering the failure of invariants on liquidations.

I don't see how this should be deduplicated from #8 though, both issues imo should be valid.

Would be key to also note that these "edge cases" were directly requested in this section of the readMe where protocol clearly requested we submit bug ideas that break the invariant checks, to quote them:

The protocol has hardcoded invariant checks in InvariantChecks.sol. If these can be made to revert, then we should know about it.

Additionally Protocol's comment on the idea of this issue being submitted in the previous contest, does not invalidate this on sherlock... all will fix issues from a previous contest that wasn't correctly fixed in an update contest is considered in scope.

[rashtrakoff]

Issue https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest-judging/issues/8 was marked as invalid because it's a known issue that UNIT goes to zero. However, my report correctly describes a scenario where stableCollateralTotal can be set to zero without the need for UNIT to go to zero.

stableCollateralTotal can be 0 or -ve in the following cases:

• Collateral price has increased rapidly and the market was over-leveraged.
• Funding fees depleted the LPs.
• A combination of both of the above.

In all these cases, the UNIT goes to 0. UNIT price is determined by the PnL of the LPs AND the funding fees they earned.

Coming to invariant breaking, we were aware of this before the start of the audit and we didn't explicitly mention this particular case because it occurs when UNIT is valued to be 0 (which has already been mentioned). However, we agree that it would make better sense to revert explicitly when stableCollateralTotal (or the newStableCollateralTotal) is 0 or -ve instead of capping to 0. We want to be clear though that even if we didn't explicitly revert, the invariants would have prevented any order from going through and hence this change isn't necessary. Since we are going to make a change, we will leave the decision for validity of the issue upon the lead judge.

[cvetanovv]

I agree with the escalation that we can separate #23 and #8.

8 - I agree that according to Sherlock rules, if from a previous contest of the same protocol, the issue has a will fix label, and the issue is not fixed, it should be recognized as valid. However, it also has it written in the readme as a "known issue", so 8 should remain invalid.

23 - I think it's borderline valid/invalid. Yes, they wrote that they want to know about invariant breaking and didn't mention this exact case, but this problem can happen as the sponsor wrote, only when UNIT goes to 0, which is a "known issue".

[santipu03]

In my report, I described a scenario where the stableCollateralTotal could be zero without UNIT being zero. However, this scenario was only possible if issue #9 was fixed. Given that the protocol team doesn't seem to fix issue #9 and considers it the "intended design", then this issue is not possible, making it invalid.

[WangSecurity]

Since this report is only possible if #9 is fixed, but the protocol is not going to fix it, this report should remain invalid. Planning to reject the escalation and leave the issue as it is.

[Evert0x]

For context, it's not possible to report an issue based on a fix in another issue.

With that logic you can

• Report issue A, with a fix.
• Report issue B which happens because of the fix in issue A. And also included a propsed fix.
• Report issue C...
• Report ...

[Evert0x]

Result: Invalid Duplicate of #8

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• santipu03: rejected