skewFractionMax can be significantly exceeded, putting LPs at risk

Summary

To ensure that the balance between leverage positions and stable collateral is healthy for the protocol and guarantees the security of LPs, the checkSkewMax() function from the FlatcoinVault module is called before any operation related to changing this ratio. It checks whether the max long skew exceeds skewFractionMax, and if it does, it reverts, thus preventing the execution of the given operation. The problem is that this check is not performed correctly, and skewFractionMax can be significantly exceeded.

Vulnerability Detail

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/FlatcoinVault.sol#L296-L307

When calculating the current skew, two variables from _globalPositions are used: sizeOpenedTotal and stableCollateralTotal. sizeOpenedTotal is the total leverage amount for all positions, and stableCollateralTotal is the total amount of rETH deposited by LPs. We know that LPs are counter-parties to the traders and they play short against the long positions of the traders. So, when there is a change in the price of rETH, it leads to a change in stableCollateralTotal and marginDepositedTotal (which is the total amount of collateral deposited by traders). How this change is calculated can be seen in the updateGlobalPositionData function in FlatcoinVault. The problem is that this is not taken into account when calculating longSkewFraction, meaning that longSkewFraction is calculated based on the old balance. A user can take advantage of this and add new leverage when it should not be possible, thus violating a core invariant and putting LPs at risk. I will provide an example of a function that incorrectly executes checkSkewMax(), but the same is done in many other places in the code.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/LeverageModule.sol#L147-L186

We can see that checkSkewMax() is executed first, and only then are the global variables updated. So the total profit and loss are not taken into consideration. Finally, I am attaching a proof of concept (POC) that demonstrates how this occurs in practice with a 10% increase in the price of rETH. You can place it in OpenPosition.t.sol.

    function log_state() public {
        FlatcoinStructs.VaultSummary memory vs = vaultProxy.getVaultSummary();
        vs = vaultProxy.getVaultSummary();
        console2.log("stableCollateralTotal: %d", vs.stableCollateralTotal);
        console2.log("sizeOpenedTotal: %d", vs.globalPositions.sizeOpenedTotal);

        uint256 longSkewFraction;
        if(vs.stableCollateralTotal > 0)
        {
            longSkewFraction = ((vs.globalPositions.sizeOpenedTotal + 0) * 1e18) / vs.stableCollateralTotal;
        }

        console2.log("longSkewFraction: %d \n", longSkewFraction);
    }

    function test_bypass_skewFractionMax() public {
        log_state();

        announceAndExecuteDeposit({
            traderAccount: alice,
            keeperAccount: keeper,
            depositAmount: uint256(100e18),
            oraclePrice: 1000e8,
            keeperFeeAmount: 0
        });

        console2.log("Alice deposited 100 weth");

        uint256 tokenId = announceAndExecuteLeverageOpen({
            traderAccount: bob,
            keeperAccount: keeper,
            margin: 100e18,
            additionalSize: 100e18,
            oraclePrice: 1000e8,
            keeperFeeAmount: 0
        });

        console2.log("Bob opened leverage position with 100 weth margin and 100 weth additional size");

        log_state();

        setWethPrice(1100e8);

        uint256 tokenId2 = announceAndExecuteLeverageOpen({
            traderAccount: carol,
            keeperAccount: keeper,
            margin: 20e18,
            additionalSize: 20e18,
            oraclePrice: 1100e8,
            keeperFeeAmount: 0
        });

        console2.log("Carl opened leverage position with 20 weth margin and 20 weth additional size");

        log_state();

        vm.expectRevert();
        vaultProxy.checkSkewMax(0);
    }

  stableCollateralTotal: 0
  sizeOpenedTotal: 0
  longSkewFraction: 0 

  Alice deposited 100 weth
  Bob opened leverage position with 100 weth margin and 100 weth additional size
  stableCollateralTotal: 100000000000000000000
  sizeOpenedTotal: 100000000000000000000
  longSkewFraction: 1000000000000000000 

  Carl opened leverage position with 20 weth margin and 20 weth additional size
  stableCollateralTotal: 90909090909090909091
  sizeOpenedTotal: 120000000000000000000
  longSkewFraction: 1319999999999999999

You can see that the skew become 1.31 while the max allowed is 1.2.

Impact

Violation of a core invariant, the possibility to take more leverage than allowed. This leads to a significant increase in the risk for LPs and consequently the loss of funds.

Code Snippet

Above

Tool used

Manual Review

Recommendation

Update the global position with profit and loss before execute checkSkewMax().

Duplicate of #143

sherlock-admin commented 7 months ago

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

takarez commented:

valid: medium(6)