LTDingZhen - User can reenter `executeOpen` to avoid trade fee on a limit order

[sherlock-admin2]

LTDingZhen

medium

User can reenter executeOpen to avoid trade fee on a limit order

Summary

When a keeper tries to execute executeOpen to mint a new leverage NFT to a user, the data of the position is set after the mint is successful, so the user can manipulate a position without any data by setting up their own onERC721Received() function.

Even if executeOrder is protected by a re-entry lock, users can still reenter contract LimitOrder to avoid tradeFee.

Vulnerability Detail

In LeverageModule.sol:

function executeOpen(
    address _account,
    address _keeper,
    FlatcoinStructs.Order calldata _order
) external whenNotPaused onlyAuthorizedModule returns (uint256 _newTokenId) {
    ...
    {
        // The margin change is equal to funding fees accrued to longs and the margin deposited by the trader.
        vault.updateGlobalPositionData({
            price: entryPrice,
            marginDelta: int256(announcedOpen.margin),
            additionalSizeDelta: int256(announcedOpen.additionalSize)
        });

        _newTokenId = _mint(_account);
        //@Audit Reenter here

        vault.setPosition(
            FlatcoinStructs.Position({
                lastPrice: entryPrice,
                marginDeposited: announcedOpen.margin,
                additionalSize: announcedOpen.additionalSize,
                entryCumulativeFunding: vault.cumulativeFundingRate()
            }),
            _newTokenId
        );
    }
...
}

In FlatcoinVault, positions are stored as mappings from tokenId to struct userPosition. When trying to get the value of a key that does not exist in the mapping, the default value for each member of the structure is returned, so we now have a position with 0 marginDeposited, 0 additionalSize, 0 entryCumulativeFunding and 0 lastPrice.

/// @dev Holds mapping between user addresses and their leverage positions.
mapping(uint256 tokenId => FlatcoinStructs.Position userPosition) internal _positions;

/// @notice Individual leverage position
struct Position {
    uint256 lastPrice;
    uint256 marginDeposited;
    uint256 additionalSize;
    int256 entryCumulativeFunding;
}

At this point, if the user tries to call announceLimitOrder on his onERC721Received(), he can set tradeFee and receive no revert!

//@Audit this should be set in malicious user's contract:
function onERC721Received() external {
    LeverageModule.announceLimitOrder(LeverageModule.tokenIdNext(), priceLowerThreshold, priceUpperThreshold)
}

//@Audit LeverageModule.sol
function announceLimitOrder(uint256 tokenId, uint256 priceLowerThreshold, uint256 priceUpperThreshold) external {
    uint64 executableAtTime = _prepareAnnouncementOrder();
    address positionOwner = _checkPositionOwner(tokenId);
    _checkThresholds(priceLowerThreshold, priceUpperThreshold);
    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
    });
}

Impact

User can bypass trade fee on Limit Orders

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Just follow the checks-effect-interaction mode, use

        vault.setPosition(
            FlatcoinStructs.Position({
                lastPrice: entryPrice,
                marginDeposited: announcedOpen.margin,
                additionalSize: announcedOpen.additionalSize,
                entryCumulativeFunding: vault.cumulativeFundingRate()
            }),
            tokenIdNext    //@Audit tokenIdNext is tracked in this module
        );

        _mint(_account);

instead of

        _newTokenId = _mint(_account);

        vault.setPosition(
            FlatcoinStructs.Position({
                lastPrice: entryPrice,
                marginDeposited: announcedOpen.margin,
                additionalSize: announcedOpen.additionalSize,
                entryCumulativeFunding: vault.cumulativeFundingRate()
            }),
            _newTokenId
        );

Duplicate of #75

[sherlock-admin]

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

takarez commented:

valid: medium(5)