A malicious user can bypass limit order trading fees via cross-function re-entrancy
A malicious user can bypass limit order trading fees via cross-function re-entrancy, since `_safeMint` makes an external call to the user before updating state.\\nIn the `LeverageModule` contract, the `_mint` function calls `_safeMint`, which makes an external call `to` the receiver of the NFT (the `to` address).\\n\\nOnly after this external call, `vault.setPosition()` is called to create the new position in the vault's storage mapping. This means that an attacker can gain control of the execution while the state of `_positions[_tokenId]` in FlatcoinVault is not up-to-date.\\n\\nThis outdated state of `_positions[_tokenId]` can be exploited by an attacker once the external call has been made. They can re-enter `LimitOrder::announceLimitOrder()` and provide the tokenId that has just been minted. In that function, the trading fee is calculated as follows:\\n```\\nuint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(\\n    vault.getPosition(tokenId).additionalSize\\n);\\n```\\n\\nHowever since the position has not been created yet (due to state being updated after an external call), this results in the `tradeFee` being 0 since `vault.getPosition(tokenId).additionalSize` returns the default value of a uint256 (0), and `tradeFee` = fee * size.\\nHence, when the limit order is executed, the trading fee (tradeFee) charged to the user will be `0`.\\nA user announces opening a leverage position, calling announceLeverageOpen() via a smart contract which implements `IERC721Receiver`.\\nOnce the keeper executes the order, the contract is called, with the function `onERC721Received(address,address,uint256,bytes)`\\nThe function calls `LimitOrder::announceLimitOrder()` to create the desired limit order to close the position. (stop loss, take profit levels)\\nThe contract then returns `msg.sig` (the function signature of the executing function) to satify the IERC721Receiver's requirement.\\nTo run this proof of concept:\\nAdd 2 files `AttackerContract.sol` and `ReentrancyPoC.t.sol` to `flatcoin-v1/test/unit` in the project's repo.\\nrun `forge test --mt test_tradingFeeBypass -vv` in the terminal\\n\\n\\n
To fix this specific issue, the following change is sufficient:\\n```\\n// Remove the line below\\n_newTokenId = _mint(_account); \\n\\nvault.setPosition( \\n    FlatcoinStructs.Position({\\n        lastPrice: entryPrice,\\n        marginDeposited: announcedOpen.margin,\\n        additionalSize: announcedOpen.additionalSize,\\n        entryCumulativeFunding: vault.cumulativeFundingRate()\\n    }),\\n// Remove the line below\\n   _newTokenId\\n// Add the line below\\n   tokenIdNext\\n);\\n// Add the line below\\n_newTokenId = _mint(_account); \\n```\\n\\nHowever there are still more state changes that would occur after the `_mint` function (potentially yielding other cross-function re-entrancy if the other contracts were changed) so the optimum solution would be to mint the NFT after all state changes have been executed, so the safest solution would be to move `_mint` all the way to the end of `LeverageModule::executeOpen()`.\\nOtherwise, if changing this order of operations is undesirable for whatever reason, one can implement the following check within `LimitOrder::announceLimitOrder()` to ensure that the `positions[_tokenId]` is not uninitialized:\\n```\\nuint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(\\n    vault.getPosition(tokenId).additionalSize\\n);\\n\\n// Add the line below\\nrequire(additionalSize > 0, "Additional Size of a position cannot be zero");\\n```\\n
A malicious user can bypass the trading fees for a limit order, via cross-function re-entrancy. These trading fees were supposed to be paid to the LPs by increasing `stableCollateralTotal`, but due to limit orders being able to bypass trading fees (albeit during the same transaction as opening the position), LPs are now less incentivised to provide their liquidity to the protocol.
```\\nuint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(\\n    vault.getPosition(tokenId).additionalSize\\n);\\n```\\n