YFX-V3

CertiK7 Jul 2023Files named in the report

Audit

CertiK reviewed YFX - (YFX-V3) on 7 Jul 2023: 46 findings, 0 critical; 36 marked fixed, 4 acknowledged, 6 open.

No open critical findings

Open the report Open YFX -’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings46

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	4	0
Medium	0	0	7
Low	6	0	29

MediumInaccurate Max Market Borrow CalculationFixed

If the value of the `researveRate` state variable in the `Pool` contract is greater than zero, it indicates that a portion of the funds in the pool needs to be reserved and not lent out. In this scenario, when calculating the maximum lendable funds for a market, this reserved amount should be excluded; otherwise, it would result in an inflated value compared to the actual lendable funds. At line 82 of `FundingLogic.sol`, the code calls the `getMarketAmount()` function to obtain the specified market's maximum lendable funds. Based on the logic of this function, we can infer that the value of the exposure limit `openLimitByRatio` is calculated based on `balance + allMakerFreeze`, without considering the case where `reserveRate` is greater than zero. This leads to an inflated value of `openLimitByRatio` compared to the actual maximum lendable funds in the market. Consequently, if `openLimitByRatio` is smaller than `args.openLimit`, it ultimately results in an underestimated calculation of the `Funding Rate` (refer to line 95 in `FundingLogic.sol`).

MediumIncorrect Code Execution PathFixed

At line 83 of the code, the `positionModes` state variable of the `Market` contract is accessed to obtain the current position mode of the taker of the order. However, there is no assignment operation for the local variable `order` before this line of code, so the return value of `order.taker` is `address(0x0)`. As a result, `IMarket(msg.sender).positionModes(order.taker);` always returns the default value, which is the `OneWay` position mode. This causes all takers of executed trading orders to be treated as using the `OneWay` position mode. This is unreasonable.

MediumMemory Data Not UpdatedFixed

The passed-in memory data `order` is not updated after executing orders, so the `order.status` is still `Open`. The execution fee can never be charged successfully. ```solidity=269 function _executeOrder(MarketDataStructure.Order memory order, address to) internal { if (order.status == MarketDataStructure.OrderStatus.Open) { (,uint256 positionId) = IMarket(order.market).executeOrder(order.id); EnumerableSet.remove(notExecuteOrderIds[order.taker][order.market], order.id); if (order.status == MarketDataStructure.OrderStatus.Opened || order.status == MarketDataStructure.OrderStatus.OpenFail) { TransferHelper.safeTransferETH(to, order.executeFee); } emit Open(order.market, positionId, order.id); } } ```

MediumOnly `controller` Account Can VoteFixed

Based on the logic of the pointed out codes, it can be inferred that the business logic of the code utilizes a voting mechanism to determine the activation or deactivation of the price provided by the off-chain centralized program. However, the functions `disableFastPrice()` and `enableFastPrice()` can only be accessed by the `controller` account, resulting in the state variable `disableFastPriceVotes` only recording the voting status of the `controller` account, and the state variable `disableFastPriceVoteCount` only accumulating the number of votes from the `controller` account. According to the given description, even though the state variable `minAuthorizations` is greater than one, the provided code does not implement a multi-account voting mechanism. Instead, it relies solely on the `controller` account rather than multiple accounts to determine whether to use the price provided by the off-chain centralized program.

MediumThe Remaining `msg.value` Not Return BackFixed

When the native token sent by the user in these functions is greater than the expected value, the remaining part will not be returned to the user.

MediumUnable to Receive ETHFixed

In the `Pool` contract, if the `params.isOutETH` is true, it means the `Vault` should send ETH to the `RiskFunding` contract. ```solidity=144 _transfer(IManager(manager).riskFunding(), params.toRiskFund, params.isOutETH); ``` However, the `RiskFunding` contract is unable to receive ETH.

MediumUnchecked Return ValueFixed

The returned value, which tells whether the order is executed successfully, has not been used. If the order execution fails, it is unreasonable to still execute the subsequent logic.

LowData Coverage RiskFixed

The `orderId` starts from 0, and the `triggerOrderID` starts from max of type `uint32`(that is 4294967295). Both market orders and trigger orders are stored in the same order list. As the number of market orders increases, it results in overwritten data for trigger orders within the order list.

LowDesign Logic Of The TP and SL OrdersFixed

Invoke the function `setTPSLPrice()` in `Router.sol` to configure the take-profit and stop-loss prices for a particular position. The `liquidator` role account utilizes the function `liquidate()` in `Router.sol` to execute the take-profit or stop-loss action based on the value of the `action` parameter. If the underlying asset's market price meets the criteria for either take-profit or stop-loss, it will generate and execute a market order with `MarketDataStructure.OrderType` set to `Liquidate`, resulting in the user's position quantity becoming zero. There are three questions below: 1. Which function helps the `liquidator` to determine which position has set take-profit and stop-loss prices? 2. How does the `liquidator` role execute take-profit and stop-loss orders for users? Assuming the knowledge of a position with configured take-profit and stop-loss prices, if the price does not meet the take-profit condition and `liquidate()` fails to execute the take-profit, will it proceed to execute the stop-loss by calling `liquidate()` again?

LowDuplicated BorrowIG UpdateFixed

The following code will invoke the `_updateBorrowIG()` function in `Pool` contract to update the long/short borrowing rates of the `Pool` contract. 1. Line 446 of the `_executeLiquidityOrder()` function in the `Router` contract, 2. Line 214 of the `executeAddLiquidityOrder()` function in the `Pool` contract. 3. Line 288 of the `executeRmLiquidityOrder()` function in the `Pool` contract. If it is an add liquidity operation, then steps 1 and 2 are executed, as the code between these two function calls does not modify the contract state, resulting in a redundant update of the long/short borrowing rates. If it is a remove liquidity operation, then steps 1 and 3 are executed, as the code between these two function calls does not modify the contract state, resulting in a redundant update of the long/short borrowing rates.

Show 10 more

LowDuplicated State Variable `sharePrice` AssignmentFixed

The assignment of the state variable `sharePrice` at line 226 is redundant code and will be overwritten, as the code at line 233 calculates the latest `baseAsset` total assets and LP token quantity to determine the `sharePrice` of the `Pool` contract after adding liquidity.

LowFunction `transfer` of Contract `ERC20` Does NotFixed

It is expected that calls of the form `transfer(recipient, amount)` fail if the recipient address is the zero address.

LowFunction `transfer` of Contract `Pool` Does Not PFixed

It is expected that calls of the form `transfer(recipient, amount)` fail if the recipient address is the zero address.

LowFunction `transfer` of Contract `TradeToken` DoesFixed

It is expected that calls of the form `transfer(recipient, amount)` fail if the recipient address is the zero address.

LowFunction `transferFrom` of Contract `ERC20` DoesFixed

It is expected that calls of the form `transferFrom(from, dest, amount)` fail if the `dest` address is zero.

LowFunction `transferFrom` of Contract `Pool` Does NFixed

It is expected that calls of the form `transferFrom(from, dest, amount)` fail if the `dest` address is zero.

LowFunction `transferFrom` of Contract `TradeToken`Fixed

It is expected that calls of the form `transferFrom(from, dest, amount)` fail if the `dest` address is zero.

LowInaccurate Maintenance Margin Check ConditionFixed

According to the description of the Maintenance Margin, when the maximum leverage in the market is 100x, the maintenance margin rate is set at $0.5\%$. Similarly, when the maximum leverage is 50x, the maintenance margin rate becomes $1\%$. From this information, we can deduce that the calculation formula for the market's maintenance margin rate is $\frac{1}{MaxLeverage} * \frac{1}{2}$. Because initial margin rate equals $\frac{1}{MaxLeverage}$, the initial margin is greater than the maintenance margin, and preventing the liquidation of user positions immediately after opening. However, it seems that the code in line 608 only restricts the market's maintenance margin rate to be greater than zero and less than `1e6`. In this case, the market's maintenance margin rate may not necessarily be equal to $\frac{1}{MaxLeverage} * \frac{1}{2}$, which does not guarantee the prevention of immediate liquidation upon position opening by users.

LowInconsistency Between Comment And CodeFixed

According to the code at line 47 and lines 49-52, we can infer that: 1. If the off-chain price has not been updated for longer than `priceDuration`, it is considered an error in the off-chain price feed service, and the `Chainlink` price needs to be adjusted up or down by $2‰$. 2. If the off-chain price has not been updated for longer than `maxPriceUpdateDelay`, it is considered that the off-chain price feed service is inactive, and the `Chainlink` price needs to be adjusted up or down by $5\\%$. However, based on the logic and comments in the `getPrice()` function, we can see the opposite design: 1. If the off-chain price has not been updated for longer than `priceDuration`, the `Chainlink` price is adjusted by $5\\%$. 2. If the off-chain price has not been updated for longer than `maxPriceUpdateDelay`, the `Chainlink` price is adjusted by $2‰$.

LowInconsistency Between Comment And CodeFixed

The attribute `longMakerFreeze` in the `DataByMarket` data structure represents long position assets, while the attribute `shortMakerFreeze` represents short position assets. However, the comments and variable names express different meanings.

Show 10 more

LowIncorrect Code Execution PathFixed

The assignment of the state variable `marketType` occurs only after the `initialize()` function of the `Market` contract is called. Therefore, the validation of the market type through the `checkoutConfig()` function of the `MarketLogic` contract is only effective after the initialization of the `Market` contract. However, in the `createPair()` function of the `Manager` contract, the logic in lines 299 to 301 validates the `Market` contract configuration before the initialization of the `Market` contract. This results in the `marketType` state variable always being zero, causing the validation to fail in line 601 of the `checkoutConfig()` function.

LowIncorrect Error MessageFixed

```solidity=62 require(!marketConfig.updateMarginPaused, "Market:Set price paused"); ``` The correct error message should be `Market: Update margin paused`. --- ```solidity=310 require(position.amount > 0, "Market:order not exist"); ``` According to the logic of the code at lines 309-310, we can infer that its purpose is to verify whether the specified position exists. Therefore, if the boolean expression `position.amount > 0` returns false, the error message should be `Market: position does not exist`.

LowMissing `asset` Argument Value ValidationFixed

The `asset` parameter in the `createPair()` function represents the collateral token of the market and should align with the value of the `baseAsset` state variable in the `Pool` contract. However, the current code logic lacks validation for the values of `asset` and `baseAsset`.

LowMissing Zero Address ValidationFixed

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=29 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=59 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=93 tradeToken = _tradeToken; ``` - `_tradeToken` is not zero-checked before being used. --- ```solidity=99 inviteToken = _inviteToken; ``` - `_inviteToken` is not zero-checked before being used. --- ```solidity=33 fundingLogic = _fundingLogic; ``` - `_fundingLogic` is not zero-checked before being used. --- ```solidity=74 marginAsset = _marginAsset; ``` - `_marginAsset` is not zero-checked before being used. --- ```solidity=75 pool = _pool; ``` - `_pool` is not zero-checked before being used. --- ```solidity=92 fundingLogic = _fundingLogic; ``` - `_fundingLogic` is not zero-checked before being used. --- ```solidity=32 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=37 marketPriceFeed = _marketPriceFeed; ``` - `_marketPriceFeed` is not zero-checked before being used. --- ```solidity=43 WETH = _WETH; ``` - `_WETH` is not zero-checked before being used. --- ```solidity=44 vault = IManager(_manager).vault(); ``` - `_manager` is not zero-checked before being used. --- ```solidity=458 marketPriceFeed = _marketPriceFeed; ``` - `_marketPriceFeed` is not zero-checked before being used. --- ```solidity=22 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=36 rewardAsset = _rewardAsset; ``` - `_rewardAsset` is not zero-checked before being used. --- ```solidity=42 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=43 WETH = _WETH; ``` - `_WETH` is not zero-checked before being used. --- ```solidity=103 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=113 marketPriceFeed = _marketPriceFeed; ``` - `_marketPriceFeed` is not zero-checked before being used. --- ```solidity=58 manager = _manager; ``` - `_manager` is not zero-checked before being used. --- ```solidity=67 chainlinkFlags = _chainlinkFlags; ``` - `_chainlinkFlags` is not zero-checked before being used. --- ```solidity=72 priceHelper = _priceHelper; ``` - `_priceHelper` is not zero-checked before being used. --- ```solidity=94 secondaryPriceFeed = _secondaryPriceFeed; ``` - `_secondaryPriceFeed` is not zero-checked before being used. --- ```solidity=17 manager = _manager; ``` - `_manager` is not zero-checked before being used.

LowRedundant StatementsFixed

The `removeLiquidity()` function of the `Pool` contract includes the maker address information in its return value. However, the caller of this function, the `Router` contract, only uses the function's return value at lines 464 and 482, without utilizing the `maker` address information. Therefore, the return value can be safely removed.

LowStrange JudgementFixed

Such a judgment and requirement is strange and useless. ```solidity=88 if (fundingLogic != address(0)) { require(fundingLogic != address(0), "Market: invalid funding logic contract address"); } ```

LowThe Field `deadline` Used In `Router` ContractFixed

According to the code logic, we know that when the value of `block.timestamp` surpasses the `deadline`, the creation of open/close position orders is prohibited, resulting in the failure of invoking the `takerOpen()/takerClose()/takerOpenETH()` functions. We want to know more information about the `deadline` variable.

LowThe Value Of `sharePrice` Is Inconsistent.Fixed

The state variable `sharePrice` represents the value of an `LP` token in terms of the `baseAsset`. In the following two scenarios, `totalSupply` is zero, but the value of `sharePrice` is different. 1. When the state variable `sharePrice` is initialized to zero, it indicates that the `Pool` contract has been deployed and `totalSupply` is also zero. 2. In the `executeRmLiquidityOrder()` function, lines 313-317, when liquidity is removed and `totalSupply` becomes zero, the value of the state variable `sharePrice` is set to `1e10`. This may result in erroneous display of interface data.

LowTypoFixed

```solidity mapping(int8 => IPool.InterestData) public interestDate; ``` It seems the variable `interestDate` is more suitable to be named as `interestData`.

LowUnchecked ERC-20 `transfer()`/`transferFrom()` CaFixed

The return value of the transfer()/transferFrom() call is not checked. ```solidity=195 if (params.order.freezeMargin > 0) IERC20(marginAsset).transfer(IManager(manager).vault(), params.order.freezeMargin); ``` --- ```solidity=435 if (order.freezeMargin > 0) IERC20(marginAsset).transfer(msg.sender, order.freezeMargin); ```

Show 6 more

LowUnnecessary Conditional OperatorsFixed

```solidity _transfer(params.inviter, params.feeToInviter, baseAsset == WETH ? true : false); ``` Using the conditional operator to set true or false is unnecessary.

LowUnsafe Integer CastFixed

```solidity=216 uint256 amount = _liquidity.mul(uint256(int256(allMakerFreeze).add(int256(balance)).add(totalUnPNL).add(allMarketPos.makerFundingPayment))).div(totalSupply); ``` - The type conversion `int256(allMakerFreeze)` from type uint256 to type int256 may flip the value's sign. --- ```solidity=209 (, int256 _p, , ,) = priceFeed.getRoundData(uint80(roundId - i)); ``` - The type conversion `uint80(roundId - i)` from larger type uint256 to smaller type uint80 may truncate data.

LowUnused Argument `_market`Fixed

All markets adopt a uniform order execution fee. The parameter `_market` is not utilized in the function `getExecuteOrderFee()`.

LowUnused EventFixed

Some events are never emitted, which can lead to confusion and code maintainability issues. ```solidity=47 event SetReferrerDiscountShare(address referrer, uint256 discountShare); ``` - `SetReferrerDiscountShare` is declared in `InviteManager` but never emitted. --- ```solidity=43 event SetPaused(bool _open, bool _set, bool _openTrigger); ``` - `SetPaused` is declared in `MarketStorage` but never emitted. --- ```solidity=76 event SetValidationLogic(address validationLogic); ``` - `SetValidationLogic` is declared in `PoolStorage` but never emitted. --- ```solidity=32 event SetManager(address _manager); ``` - `SetManager` is declared in `Vault` but never emitted. --- ```solidity=72 event SetIndexMaxDeviationBasisPoints(uint256 _indexMaxDeviationBasisPoints); ``` - `SetIndexMaxDeviationBasisPoints` is declared in `FastPriceFeed` but never emitted.

LowUse The Modifier `onlyController` InsteadFixed

These functions can be replaced by the modifier `onlyController`, for a more consistent coding style.

LowVisibility Naming ConventionFixed

The name of a function or state field with `public` or `external` visibility should not start with an underscore, "`_`".