YFX-V4

Delegate into a loop can allow double spending attack. The `msg.value` is not reset after each delegate call. So if the loop is big enough, the `msg.value` can drain the contract balance.

In commit 49b6ebb9d89ddde763c04e91b7ea6b0fe561290f, the implementation has divided `Router.sol` into `Router.sol` and `Executor.sol`, introducing a potential for reentrancy attacks. For instance, an attacker could reentrantly call `Executor.executePoolOrder()` during a `Router.cancelPoolOrder()` operation, before `Order.updatePoolOrder()` effectively cancels the order. This could allow execution of an order that should have been cancelled, potentially leading to unauthorized withdrawal or manipulation of funds.

The Chainlink API (latestAnswer) used in the `MarketPriceFeed` contract is deprecated: This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs [https://web.archive.org/web/20210304160150/https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference](https://web.archive.org/web/20210304160150/https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference). This function does not error if no answer has been reached but returns 0. Besides, the latestAnswer is reported with 18 decimals for crypto quotes but 8 decimals for FX quotes (See Chainlink FAQ for more details). A best practice is to get the decimals from the oracles instead of hard-coding them in the contract.

The function ` _shouldExecute` contains a conditional check where `block.timestamp` is expected to be greater than `_createTs.add(IManager(manager).communityExecuteOrderDelay())`. This logic is intended to ensure that a certain amount of time has passed before an action can be executed, but the implementation use incorrect operator.

In the `trade()` function of the `MarketLogic` contract, there's a line that checks the validity of `iParams.price`. This line incorrectly uses `||` (logical OR) to link two conditions that should both hold true, potentially allowing invalid transactions to be processed. ```solidity=124 require((order.takerOpenPriceMin > 0 ? iParams.price >= order.takerOpenPriceMin : true) || (order.takerOpenPriceMax > 0 ? iParams.price <= order.takerOpenPriceMax : true), "MT2"); ```

The function `IWrappedCoin(WETH).withdraw()` unwraps WETH and transfers ETH to the current contract. So please ensure that the function `receive()` is declared in the `Order` contract to successfully receive the ETHs, also ensure the function `_fallback()` is not called in the function `receive()` to avoid the risk of gas inefficiency. ```solidity receive () external payable virtual { // no _fallback(); } ```

The project allows any users to create pool orders with a fee. The `params.maker` parameter is taken from user input, allowing any user to initiate a liquidity removal order that affects another user's liquidity position. This vulnerability could be exploited by a malicious actor to remove liquidity on behalf of another user without their consent, leading to unintended loss of positions.

The `Router` contract's functions `setTPSLPrice()`, `switchPositionMode()`, and `setMakerTPSLPrice()` lack the `nonReentrant` modifier, a standard security practice to prevent reentrancy attacks.

The function `takerOpen` allows user to place an open-position order. The comment should be `@notice place an open-position order, margined by both erc20 and eth`, instead of `margined by erc20 tokens`.

**Router.sol** ```solidity=506 function setMakerTPSLPrice(address _pool, uint256 _positionId, uint256 _profitPrice, uint256 _stopLossPrice) external payable validatePool(_pool) { IPool(_pool).setTPSLPrice(msg.sender, _positionId, _profitPrice, _stopLossPrice); emit SetMakerTPSLPrice(_pool, _positionId, _profitPrice, _stopLossPrice); } ``` **Pool.sol** ```solidity=404 function setTPSLPrice(address maker, uint256 positionId, uint256 tp, uint256 sl) external _onlyRouter { IPool.Position storage position = makerPositions[positionId]; require(!clearAll && position.maker == maker && position.liquidity > 0, "PS0"); position.makerStopLossPrice = sl; position.makerProfitPrice = tp; } ``` In the given contract setup, the `setMakerTPSLPrice()` function in `Router.sol` allows setting TP and SL prices for a maker's position in the `Pool.sol` contract. This setup is crucial for enabling the `PriceProvider` to remove the maker's liquidity based on specified price conditions. However, there is a critical oversight: the absence of validation checks for the TP and SL prices set by the maker. Without these checks, it's possible to set a TP price that is equal to or lower than the SL price. In such cases, given the liquidity removal conditions (`g.sharePrice >= position.makerProfitPrice || g.sharePrice <= position.makerStopLossPrice`), the `PriceProvider` can trigger liquidity removal at any time, potentially leading to unintended liquidity removals. **Router.sol** ```solidity=453 function removeLiquidityBySystem(address _pool, address _maker, bool isReceiveETH) external payable onlyPriceProvider { _removeLiquidity(0, _pool, _maker, type(uint256).max, true, isReceiveETH, true); } ``` **Pool.sol** ```solidity=295 if (isSystem) { vars.isTP = position.makerProfitPrice > 0 ? g.sharePrice >= position.makerProfitPrice : false; vars.aType = vars.isTP ? 1 : 2; require((g.sharePrice <= position.makerStopLossPrice) || vars.isTP, "PRL4"); } ```