HSP - Fees are ignored when checks skew max in Stable Withdrawal / Leverage Open / Leverage Adjust

[sherlock-admin2]

HSP

medium

Fees are ignored when checks skew max in Stable Withdrawal / Leverage Open / Leverage Adjust

Summary

Fees are ignored when checks skew max in Stable Withdrawal / Leverage Open / Leverage Adjust.

Vulnerability Detail

When user withdrawal from the stable LP, vault total stable collateral is updated:

        vault.updateStableCollateralTotal(-int256(_amountOut));

Then _withdrawFee is calculated and checkSkewMax(...) function is called to ensure that the system will not be too skewed towards longs:

            // Apply the withdraw fee if it's not the final withdrawal.
            _withdrawFee = (stableWithdrawFee * _amountOut) / 1e18;

            // additionalSkew = 0 because withdrawal was already processed above.
            vault.checkSkewMax({additionalSkew: 0});

At the end of the execution, vault collateral is settled again with withdrawFee, keeper receives keeperFee and (amountOut - totalFee) amount of collaterals are transferred to the user:

        // include the fees here to check for slippage
        amountOut -= totalFee;

        if (amountOut < stableWithdraw.minAmountOut)
            revert FlatcoinErrors.HighSlippage(amountOut, stableWithdraw.minAmountOut);

        // Settle the collateral
        vault.updateStableCollateralTotal(int256(withdrawFee)); // pay the withdrawal fee to stable LPs
        vault.sendCollateral({to: msg.sender, amount: order.keeperFee}); // pay the keeper their fee
        vault.sendCollateral({to: account, amount: amountOut}); // transfer remaining amount to the trader

The totalFee is composed of keeper fee and withdrawal fee:

        uint256 totalFee = order.keeperFee + withdrawFee;

This means withdrawal fee is still in the vault, however this fee is ignored when checks skew max and protocol may revert on a safe withdrawal. Consider the following scenario:

1. skewFractionMax is 120% and stableWithdrawFee is 1%;
2. Alice deposits 100 collateral and Bob opens a leverage position with size 100;
3. At the moment, there is 100 collaterals in the Vault, skew is 0 and skew fraction is 100%;
4. Alice tries to withdraw 16.8 collaterals, withdrawFee is 0.168, after withdrawal, it is expected that there is 83.368 stable collaterals in the Vault, so skewFraction should be 119.5%, which is less than skewFractionMax;
5. However, the withdrawal will actually fail because when protocol checks skew max, withdrawFee is ignored and the skewFraction turns out to be 120.19%, which is higher than skewFractionMax.

The same issue may occur when protocol executes a leverage open and leverage adjust, in both executions, tradeFee is ignored when checks skew max.

Please see the test codes:

    function test_audit_withdraw_fee_ignored_when_checks_skew_max() public {
        // skewFractionMax is 120%
        uint256 skewFractionMax = vaultProxy.skewFractionMax();
        assertEq(skewFractionMax, 120e16);

        // withdraw fee is 1%
        vm.prank(vaultProxy.owner());
        stableModProxy.setStableWithdrawFee(1e16);

        uint256 collateralPrice = 1000e8;

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

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

        // After leverage Open, skew is 0
        int256 skewAfterLeverageOpen = vaultProxy.getCurrentSkew();
        assertEq(skewAfterLeverageOpen, 0);
        // skew fraction is 100%
        uint256 skewFractionAfterLeverageOpen = getLongSkewFraction();
        assertEq(skewFractionAfterLeverageOpen, 1e18);

        // Note: comment out `vault.checkSkewMax({additionalSkew: 0})` and below lines to see the actual skew fraction
        // Alice withdraws 16.8 collateral
        // uint256 aliceLpBalance = stableModProxy.balanceOf(alice);
        // announceAndExecuteWithdraw({
        //     traderAccount: alice, 
        //     keeperAccount: keeper, 
        //     withdrawAmount: 168e17, 
        //     oraclePrice: collateralPrice, 
        //     keeperFeeAmount: 0
        // });

        // // After withdrawal, the actual skew fraction is 119.9%, less than skewFractionMax
        // uint256 skewFactionAfterWithdrawal = getLongSkewFraction();
        // assertEq(skewFactionAfterWithdrawal, 1199501007580846367);

        // console2.log(WETH.balanceOf(address(vaultProxy)));
    }

Impact

Protocol may wrongly prevent a Stable Withdrawal / Leverage Open / Leverage Adjust even if the execution is essentially safe.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/StableModule.sol#L130 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L101 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L166

Tool used

Manual Review

Recommendation

Include withdrawal fee / trade fee when check skew max.

[sherlock-admin]

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

takarez commented:

invalid

[0xLogos]

Escalate

Low. Exceeding skewFractionMax is possible only by a fraction of a percent

[sherlock-admin2]

Escalate

Low. Exceeding skewFractionMax is possible only by a fraction of a percent

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.

[santipu03]

Agree with @0xLogos. The withdrawal fee is a tiny amount compared to the total deposited collateral, therefore the impact will be almost imperceptible. The severity should be LOW.

[0xhsp]

This issue is valid.

The fee maybe a tiny amount compared to the total deposited collateral, but the impact is significant to individual users.

Let's assume stableCollateralTotal is 1000 ether, stableWithdrawFee is 1%, skewFractionMax is 120% and current skewFraction is 60%.

If withdrawal fee is ignored in the calculation of shew fraction, withdrawAmount is calculated as:

If withdrawal fee is considered in the calculation of shew fraction, withdrawAmount is calculated as:

We can notice that 5 ether less collaterals ethers are withdrawable if fee is ignored. Just be realistic, individual user is most likely to deposit a tiny amount of collaterals, this means many users are unable to withdraw any of their collaterals even if it is safe to do so.

Similarly, since tradeFee is ignored when protocol checks a leverage open/adjustment, many traders would be wrongly prevented from opening/adjusting any positions.

[0xLogos]

In first pic after withdrawing for example 300 eth, fee for that amount will be included in the next withdrawal and so forth so eventually all 505 eth can be withdrawn. Note that it's not work around, just how things work in most cases.

[0xhsp]

It's not about how much user can withdraw but if user can withdraw whenever it is safe.

Given after withdrawing for example 300 eth, users expect to be able to withdraw 353.5 ether more but are only allowed 350 ether due to the issue.

Incorrect checking is a high risk to the protocol, it is difficult to predict how users will operate but it would eventually cause huge impact if we ignore the risk.

[santipu03]

The margin error on the calculation of checkSkewMax will be equal to the tradeFee on that operation. When the operation is using a huge amount (500 ETH), the margin error of the calculation will be of 5 ETH (assuming a 1% tradeFee). But if 10 users withdraw 50 ETH each, the margin error will only be 0.5 ETH on the last withdrawal.

To trigger this issue with a non-trivial margin error, it requires a user that withdraws collateral (or creates or adjusts a position) with a huge amount compared to the total collateral deposited.

Given the low probability of this issue happening with a non-trivial margin error and the impact being medium/low, I'd consider the overall severity of this issue to be LOW.

[0xhsp]

The probability should be medium as you cannot predict the wild market, the impact can be high since usera may suffer a loss due to price fluncation if they cannot withdraw in time. So it's fair to say the servrity is medium.

[sherlock-admin]

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

[Evert0x]

It seems to me that the issue described can negatively affect users and breaks core contract functionality in specific (but not unrealistic) scenarios https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue

Tentatively planning to reject the escalation and keep the issue state as is, but will revisit it later.

[Evert0x]

Planning to continue with my judgment as stated in the comment above.

[Czar102]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xLogos: rejected

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.