shaka - Trade fees can be avoided in limit orders

[sherlock-admin2]

shaka

high

Trade fees can be avoided in limit orders

Summary

On limit order announcement the trade fee is calculated based on the current size of the position and its value is used on the execution of the limit order. However, it is not taken into account that the value of additionalSize in the position can have changed since the limit order was announced, so users can avoid paying trade fees for closing leveraged positions at the expense of LPs.

Vulnerability Detail

When a user announces a limit order to close a leveraged position the trade fee is calculated based on the current trade fee rate and the additionalSize of the position and stored in the _limitOrderClose mapping.

On the execution of the limit order, the value of the trade fee recorded in the _limitOrderClose mapping is used to build the AnnoundedLeverageClose struct that is sent to the LeverageModule:executeClose function. In this function the trade fee is used to pay the stable LPs and subtracted from the total amount received by the user closing the position.

However, it is not taken into account that the value of additionalSize in the position can have changed since the limit order was announced via the LeverageModule:executeAdjust function.

As a result, users that want to open a limit order can do so using the minimum additionalSize possible and then increase it after the limit order is announced, avoiding paying the trade fee for the additional size adjustment.

It is also worth mentioning that the trade fee rate can change between the announcement and the execution of the limit order, so the trade fee calculated at the announcement time can be different from the one used at the execution time. Although this scenario is much less likely to happen (requires the governance to change the trade fee rate) and its impact is much lower (the trade fee rate is not likely to change significantly).

Impact

Users can avoid paying trade fees for closing leveraged positions at the expense of UNIT LPs, that should have received the trade fee.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L62-L64

Proof of concept

Tests

The following tests show the example of a user that opens a limit order to be executed when the price doubles. The operations performed in both tests are identical, but the order of the operations is different, so we can see how a user can exploit the system to avoid paying trade fees. To reproduce it add the following code to `AdjustPositionTest` contract and run `forge test --mt test_LimitTradeFee -vv`: ```solidity function test_LimitTradeFee_ExpectedBehaviour() public { uint256 aliceCollateralBalanceBefore = WETH.balanceOf(alice); uint256 collateralPrice = 1000e8; announceAndExecuteDeposit({ traderAccount: bob, keeperAccount: keeper, depositAmount: 10000e18, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Create small leverage position uint256 initialMargin = 0.05e18; uint256 initialSize = 0.1e18; uint256 tokenId = announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: initialMargin, additionalSize: initialSize, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Increase margin and size in position uint256 adjustmentTradeFee = announceAndExecuteLeverageAdjust({ tokenId: tokenId, traderAccount: alice, keeperAccount: keeper, marginAdjustment: 100e18, additionalSizeAdjustment: 2400e18, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Anounce limit order to close position at 2x price vm.startPrank(alice); limitOrderProxy.announceLimitOrder({ tokenId: tokenId, priceLowerThreshold: 0, priceUpperThreshold: collateralPrice * 2 }); // Collateral price doubles after 1 month and the order is executed skip(4 weeks); collateralPrice = collateralPrice * 2; setWethPrice(collateralPrice); bytes[] memory priceUpdateData = getPriceUpdateData(collateralPrice); vm.startPrank(keeper); limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData); uint256 aliceCollateralBalanceAfter = WETH.balanceOf(alice); console2.log("profit:", aliceCollateralBalanceAfter - aliceCollateralBalanceBefore); } function test_LimitTradeFee_PayLessFees() public { uint256 aliceCollateralBalanceBefore = WETH.balanceOf(alice); uint256 collateralPrice = 1000e8; announceAndExecuteDeposit({ traderAccount: bob, keeperAccount: keeper, depositAmount: 10000e18, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Create small leverage position uint256 initialMargin = 0.05e18; uint256 initialSize = 0.1e18; uint256 tokenId = announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: initialMargin, additionalSize: initialSize, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Anounce limit order to close position at 2x price vm.startPrank(alice); limitOrderProxy.announceLimitOrder({ tokenId: tokenId, priceLowerThreshold: 0, priceUpperThreshold: collateralPrice * 2 }); // Increase margin and size in position uint256 adjustmentTradeFee = announceAndExecuteLeverageAdjust({ tokenId: tokenId, traderAccount: alice, keeperAccount: keeper, marginAdjustment: 100e18, additionalSizeAdjustment: 2400e18, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Collateral price doubles after 1 month and the order is executed skip(4 weeks); collateralPrice = collateralPrice * 2; setWethPrice(collateralPrice); bytes[] memory priceUpdateData = getPriceUpdateData(collateralPrice); vm.startPrank(keeper); limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData); uint256 aliceCollateralBalanceAfter = WETH.balanceOf(alice); console2.log("profit:", aliceCollateralBalanceAfter - aliceCollateralBalanceBefore); } ```

Result

```js [PASS] test_LimitTradeFee_ExpectedBehaviour() (gas: 2457623) Logs: profit: 1195246800000000000000 [PASS] test_LimitTradeFee_PayLessFees() (gas: 2441701) Logs: profit: 1197646800000000000000 Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 21.23ms ``` As we can see, in the second case the user was able to get a profit 2.4 rETH higher, corresponding to the trade fee avoided for the additional size adjustment done after creating the limit order (2,400 rETH * 0.1% fee). That higher profit has come at the expense of LPs, who should have received the trade fee.

Tool used

Manual Review

Recommendation

Calculate the trade fee at the execution time of the limit order, in the same way it is done for stable withdrawals for the withdrawal fee.

File: LimitOrder.sol

+       uint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(
+           vault.getPosition(tokenId).additionalSize
+       );
+
        order.orderData = abi.encode(
            FlatcoinStructs.AnnouncedLeverageClose({
                tokenId: tokenId,
                minFillPrice: minFillPrice,
-               tradeFee: _limitOrder.tradeFee
+               tradeFee: tradeFee
            })
        );

A maxTradeFee parameter can also be added at the announcement time to avoid the trade fee being higher than a certain value.

[sherlock-admin]

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

takarez commented:

valid:

[sherlock-admin]

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

[ydspa]

Escalate

Fee loss in certain circumstance is a Medium issue

[sherlock-admin2]

Escalate

Fee loss in certain circumstance is a Medium issue

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]

Escalate.

The impact of this issue is that the LPs will not receive the trade fee. This is similar to other issues where the protocol did not receive their entitled trade fee due to some error and should be categorized as loss of fee OR loss of earning, and thus should be a Medium.

Also, note that the LPs gain/earn when the following event happens:

• Loss of the long trader. This is because LP is the short side. The loss of a long trader is the gain of a short trader.
• Open, adjust, close long position - (0.1% fee)
• Open limit order - (0.1% fee)
• Borrow Rate Fees (aka Funding Rate)
• Liquidation Fees
• ETH Staking Yield

Statically, the bulk of the LP gain comes from the loss of the long trader in such a long-short prep protocol in the real world. The uncollected trading fee due to certain malicious users opening limit orders only makes up a very small portion of the earnings and does not materially impact the LPs or protocols. Also, this is not a bug that would drain the protocol, directly steal the assets of LPs, or lead to the protocol being insolvent. Thus, this issue should not be High. A risk rating of Medium would be more appropriate in this case.

[sherlock-admin2]

Escalate.

The impact of this issue is that the LPs will not receive the trade fee. This is similar to other issues where the protocol did not receive their entitled trade fee due to some error and should be categorized as loss of fee OR loss of earning, and thus should be a Medium.

Also, note that the LPs gain/earn when the following event happens:

• Loss of the long trader. This is because LP is the short side. The loss of a long trader is the gain of a short trader.
• Open, adjust, close long position - (0.1% fee)
• Open limit order - (0.1% fee)

Statically, the bulk of the LP gain comes from the loss of the long trader in such a long-short prep protocol in the real world. The uncollected trading fee due to certain malicious users opening limit orders only makes up a very small portion of the earnings and does not materially impact the LPs or protocols. Also, this is not a bug that would drain the protocol, directly steal the assets of LPs, or lead to the protocol being insolvent. Thus, this issue should not be High. A risk rating of Medium would be more appropriate in this case.

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.

[nevillehuang]

I think I agree with medium and @xiaoming9090 analysis, my initial thoughts were that fees make up a core part of the LPs earnings and should never be allowed to be side stepped.

I want to note though if this can be performed repeatedly by an user grieving fees for a LP, wouldn't it be meeting the following criteria? But depending on what is deemed as material loss of funds for sherlock, I will let @Czar102 decide on severity, but I can agree with medium severity.

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

[0xjuaan]

To counter @xiaoming9090 's analysis (for this issue and also for issue #75) -

Statically, the bulk of the LP gain comes from the loss of the long trader in such a long-short prep protocol in the real world.

Is the above really true? Since the protocol is designed to be delta neutral, the expected PnL from the above avenue would be zero. LPs cant affect whether traders win or lose. However, the trading fee is the only feature that guarantees some gains for the LPs, and with users bypassing this- LPs are much less incentivised to participate.

Because of the above, I think that fees still make up a core part of the LPs earnings, but you can correct me if I am wrong.

[xiaoming9090]

I have updated my original escalation with a more comprehensive list of avenues from which the LPs earn their yield, so that the judge can have a more complete picture.

[shaka0x]

Also, note that the LPs gain/earn when the following event happens:

• Loss of the long trader. This is because LP is the short side. The loss of a long trader is the gain of a short trader.
• Open, adjust, close long position - (0.1% fee)
• Open limit order - (0.1% fee)
• Borrow Rate Fees (aka Funding Rate)
• Liquidation Fees
• ETH Staking Yield

Statically, the bulk of the LP gain comes from the loss of the long trader in such a long-short prep protocol in the real world.

I respectfully disagree. The position of the UNIT holders will not always be short, and even the when they are the long trader might not necessarily have losses.

The sponsor clarified this in a Discord message, regarding the comment regarding the LSD yield as a source of yield for UNIT holders:

The LSD yield is a little more complex because UNIT holders are technically long and short rETH. 

And confirmed that the sources of UNIT yield come from funding rate, trading fees and liquidations.

UNIT is delta neutral. But yield is earned from:
1. Funding rate (which is historically usually being paid to the short positions). Ie. Leverage long traders are typically happy to pay a funding fee to shorts (but not always the case). This goes to UNIT holders or is paid by UNIT holders if funding rate is negative
2. Trading fees on each trade
3. Liquidation remaining margin when leverage traders get liquidated

Thus, the trade fees are a fundamental incentive for the UNIT holders.

[Czar102]

Not being able to gather protocol fees is a Medium severity issue as there is no loss of funds.

In this case though, I think High severity is justified, because the fee is what LPs are paid for traders to trade against, and there are value extraction strategies like multi-block #216 if the market is giving any additional information than the oracle states (always). It's like selling an option, but not receiving the premium. This is a severe loss of funds, even though the notional wasn't impacted.

Is my way of thinking about this issue accurate? If yes, planning to reject the escalations and leave the issue as is.

[Czar102]

Result: High Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: rejected
• xiaoming9090: rejected

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.