OKX - Audit 3

In the contract `Entrance` the role `_owner` has authority over the functions shown in the diagram below. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and configure key parameters and manipulate the project.  In the contract `Entrance` the role `operator` has authority over the functions shown in the diagram below. Any compromise to the `operator` account may allow the hacker to take advantage of this authority and manipulate the project.  In addition, the contract `Entrance` is an upgradeable contract, meaning the owner can upgrade the contract without the community's commitment. If an attacker compromises the account, the attacker can change the implementation of the contract and drain tokens from the contract. ----- In the contract `DexRouter` the role `owner` has authority over the following functions： - setApproveProxy() to set approveProxy - setWNativeRelayer() to set the wNativeRelayer - setXBridge() to set the xBridge - initializePMMRouter() to initialize the PMM Router - setPMMFeeConfig() to set the fee rate and receiver of the PMM Router In addition, the role `xBridge` has authority over the following functions： - smartSwapByOrderIdByXBridge() to perform the token swaps with the smartswap - unxswapByOrderIdByXBridge() to perform the token swaps with unxswap - uniswapV3SwapToByXBridge() to perform the token swaps with uniswapV3 - PMMV2SwapByXBridge() to perform the token swaps with PMMV2Swap In addition, the contract `DexRouter` is an upgradeable contract, meaning the owner can upgrade the contract without the community's commitment. If an attacker compromises the account, the attacker can change the implementation of the contract and drain tokens from the contract. ----- In the contract `OkxNFTMarketAggregator` the role `owner` has authority over the following functions： - approveERC20(() to approve the ERC20 tokens to the operator - setMarketRegistry() to set the marketRegistry

The function `tradeV2()` allows the users to trade NFTs with specified markets. The comment in line 312 indicates only the market `wyvern` (id is 4 based on the implementation) is allowed to be used in `tradeV2()`, but the implementation in line 313: ```solidity=313 if(tradeDetails[i].marketId != 0 && tradeDetails[i].marketId !=4 && tradeDetails[i].marketId < 7) { ``` also allows the market with id 0.

In the function `_exeHop()`, the first hop executed swaps `batchAmount` of `hops[0].fromToken`. It should be the case that `hops[0].fromToken` is the same address as `_baseRequest.fromToken` in `_smartSwapInternal()`, but there are no checks to guarantee this.

The function `_transferTokens()` has a check to see if `msg.value` matches the amount of native tokens that have been transferred to other addresses. ```solidity=382 if (msg.value != 0) {require(msg.value == nativeAmount, MetaXInvestErrors.QUANTITY_MISMATCH);} ``` However, this check is only performed if `msg.value != 0`, when it should be always done. If `msg.value == 0` but `nativeAmount > 0`, it is possible for the contract to lose native currency that was stored within it before the current `msg.sender` interacted with it. Since this contract has a `receive()` function, it is expected that it will hold native currency, which can be lost due to the above.

The functions `bytesToAddress()` and `bytesToBytes4()` do not read any state variable so they can be restricted to pure. In addition, the function `verifyWyvern()` does not modify any state variable, so it can be changed to view.

The function `shareToAmount()` is used to calculate the withdrawal amounts with the specified LP tokens. After the update in the total supply of the LP, the following check is executed: ```solidity=156 require(totalSupply > amount, MetaXInvestErrors.NOT_ENOUGH); ```` According to UniswapV2 design, `MINIMUM_LIQUIDITY = 10**3` LP tokens have been locked. Therefore, a more precise statement is as follows: ```solidity require(totalSupply > amount + MINIMUM_LIQUIDITY, MetaXInvestErrors.NOT_ENOUGH); ```

In the two functions labeled `trade()`, there is a check on the provided `tradeData` that the `orderToAddress` is the `msg.sender`. However, this check is done for some of the markets instead of all of them.

In the implementation of Uniswap V2, the function `IUniswapV2Pair(pool).getReserves()` retrieves the reserves of tokens in which the first reserve corresponds to the token0 and the second one is the token1. The token0 and token1 are recorded according to the rule that address(token0) < address(token1). In the function `withdraw()` and `_withdraw()`, the passed `tokensOut` does not validate whether the tokens are in the order that `address(tokensOut[0].token) < address(tokensOut[1].token)`. If one of them is a zero address, it needs to be converted to the weth address first. ```solidity=50 function withdraw( address pool, TokenOutInfo[] calldata tokensOut, bytes calldata data ) external override { // Check Length require(tokensOut.length == 2, 'Length Wrong'); // Decode feeRate+mode from data // mod=2 for withdraw two tokens;mod=0 for withdraw token0;mod=1 for withdraw token1 (uint256 feeRate, uint256 mode) = _getFeeRate(pool, data); // Read Status uint256 tokenAmount = _balanceOf(pool, address(this)); // Transfer to Pool _transfer(pool, pool, tokenAmount); // remove liquidity (uint256 amount0, uint256 amount1) = IUniswapV2Pair(pool).burn(address(this)); _withdraw(pool, feeRate, mode, tokensOut, amount0, amount1); // Event emit Withdraw(pool, tokenAmount); } ``` In this case, it will revert or transfer the wrong token amounts if the tokens are flipped.

The function `amountToShare()` is used to calculate the liquidity given the amounts of two tokens. However, there is no check that the passed `tokenAmounts` has length 2 and the token order matches with the tokens in the underlying pool that has the requirement `address(token0) < address(token1)`. In the case that one of them is the eth, it needs to be converted to the weth first.

In the function `smartSwapByInvest()`, the `fromToken` balance of the contract is redistributed proportionally with the ratio `batchesAmount[i] / baseRequest.fromTokenAmount` as shown in line 447-449. ```solidity=446 uint256[] memory newBatchesAmount = new uint256[](batchesAmount.length); for (uint256 i = 0; i < batchesAmount.length; ++i) { newBatchesAmount[i] = batchesAmount[i] * amount / baseRequest.fromTokenAmount; } ``` That means the sum of `batchesAmount[i]` should be the same as `baseRequest.fromTokenAmount`, but there is no such validation in the current implementation of `smartSwapByInvest()`.