LeverageModule.executeOpen doesn't apply Check-Effect-Interaction pattern

Summary

[executeOpen](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L80-L84) will internally mint NFT to the account which requests to open a new position. The problem lies in updating the position data after mint. mint uses [_safeMint](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L441) from the OZ library. This function will [callback to the receiver](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/281550b71c3df9a83e6b80ceefc700852c287570/contracts/token/ERC721/ERC721.sol#L406) (if it is a contract). This will give malicious user the opportunity to reenter the protocol.

Vulnerability Detail

A user initiates a request to open a new position via [announceLeverageOpen](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L160-L165). Afterwards, the keeper will call [executeOrder](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L378-L381) for the request, and its flow is as follows:

DelayedOrder.executeOrder
  vault.settleFundingFees();
  _executeLeverageOpen(account);
    // transfer collateral + fees to the vault
    LeverageModule.executeOpen
      //some check
      ...
L105  vault.updateGlobalPositionData({
          price: entryPrice,
          marginDelta: int256(announcedOpen.margin),
          additionalSizeDelta: int256(announcedOpen.additionalSize)
      });

L111  _newTokenId = _mint(_account);

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

L111, if account is a contract, then _mint(_account) will call back to _account internally. At this time, the position corresponding to _newTokenId has not been initialized, and the position data is all 0.

L113, [vault.setPosition](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L146-L151) initializes the position data corresponding to _newTokenId.

This is typical of not following the CEI pattern. An attack path is given below to set tradeFee of an limit order to 0.

bob deployed malicious contract C.
Call C.open() which internally call DelayedOrder.announceLeverageOpen to request for opening a position.
Afterwards, a keeper executes the LeverageOpen order. The flow is as follows:

DelayedOrder.executeOrder   //account = Contract C
  vault.settleFundingFees();
  _executeLeverageOpen(account);
    // transfer collateral + fees to the vault
    LeverageModule.executeOpen
      //some check
      ...
L105  vault.updateGlobalPositionData

L111  _mint(_account);
        _safeMint(_to, tokenIdNext)
          _safeMint(to, tokenId, "");
            _mint(to, tokenId);
            _checkOnERC721Received(address(0), to, tokenId, data)
              //@audit  to = _account = contract C
              IERC721ReceiverUpgradeable(to).onERC721Received(_msgSender(), from, tokenId, data)
                //@audit callback to contract C which is deployed by bob
->              LimitOrder.announceLimitOrder(tokenId, priceLowerThreshold, priceUpperThreshold)

L113  vault.setPosition
      ....

As shown in the above process, when C.onERC721Received is called, the position corresponding to tokenId has not yet been initialized, so position.additionalSize is 0. Calling LimitOrder.announceLimitOrder at this time will [generate a pending limit order with 0 tradeFee](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L62-L64). Since the limit order will not expire, as long as [the price of the collateral <= priceLowerThreshold or >= priceUpperThreshold](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L151-L161), the limit order can be executed.
The tradeFee of each order is paid to the short party(stable LP), which means that the short party will lose tradeFee of all such orders.

Impact

Since there is no apply CEI pattern, malicious user has the opportunity to reenter the protocol. An attack path is given above to cause the short party to lose tradeFee. There may be other attack paths to cause other impacts.

Code Snippet

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

Tool used

Manual Review

Recommendation

Two ways to fix:

move _mint(_account) to the end of the function for CEI pattern.
don't use _safeMint.

Duplicate of #75

sherlock-audit / 2023-12-flatmoney-judging

nobody2018 - LeverageModule.executeOpen doesn't apply Check-Effect-Interaction pattern #140