LTDingZhen - Trade fee is miscalculated in `LimitOrder`

[sherlock-admin2]

LTDingZhen

high

Trade fee is miscalculated in LimitOrder

Summary

When the user chooses to close the position using announceLeverageClose, trade fee is calculated from the price at which the user closes the position. But in 'announceLimitOrder', trade fee is calculated from the price at which the user set the limit order, which

Trade fee in LimitOrder should be determined by the position size at the take-profit and stop-loss prices, but it is actually miscalculated by user's position size when he calls announceLimitOrder.

Vulnerability Detail

In DelayedOrder, trade fee is calculated based on the the size of the position of the user's announce operation:

//LeverageModule.sol-L427
function getTradeFee(uint256 _size) external view returns (uint256 _tradeFee) {
    return levTradingFee._multiplyDecimal(_size);
}

//LimitOrder.sol-L58
function announceLimitOrder(uint256 tokenId, uint256 priceLowerThreshold, uint256 priceUpperThreshold) external {
    uint64 executableAtTime = _prepareAnnouncementOrder();
    address positionOwner = _checkPositionOwner(tokenId);
    _checkThresholds(priceLowerThreshold, priceUpperThreshold);
    //@Audit tradeFee is miscalculated
    uint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(
        vault.getPosition(tokenId).additionalSize
    );

    _limitOrderClose[tokenId] = FlatcoinStructs.Order({
        orderType: FlatcoinStructs.OrderType.LimitClose,
        orderData: abi.encode(
            FlatcoinStructs.LimitClose(tokenId, priceLowerThreshold, priceUpperThreshold, tradeFee)
        ),
        keeperFee: 0, // Not applicable for limit orders. Keeper fee will be determined at execution time.
        executableAtTime: executableAtTime
    });

    // Lock the NFT belonging to this position so that it can't be transferred to someone else.
    ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).lock(tokenId);

    emit FlatcoinEvents.OrderAnnounced({
        account: positionOwner,
        orderType: FlatcoinStructs.OrderType.LimitClose,
        keeperFee: 0
    });
}

But, as price changes affect users' position size, when a Take Profit order is triggered, less trade fee is charged and when a Stop Loss order is triggered, more trade fee is charged. (compare with announceLeverageClose)

Impact

The trade fee of every limit order is miscalculated, putting the protocol or all users at a significant loss in a one-sided market.

Code Snippet

Trade fee is correctly calculated in announceLeverageClose: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L331

Trade fee is miscalculated in announceLimitOrder: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L62-L64

Tool used

Manual Review

Recommendation

Trade fee in LimitOrder should be determined by the position size at the take-profit and stop-loss prices.

Duplicate of #212

[sherlock-admin]

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

takarez commented:

valid: the fee should correspond to the size; high(11)

[sherlock-admin]

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