Native v2 (Security Review)

The `CreditVault.sol` contract uses the `supportMarket()` function to support the market corresponding to a `LPToken.sol` contract instance. Once the market is supported, the `LPToken.sol` address can withdraw its underlying ERC20 token to an arbitrary `to` address. While `supportMarket()` function performs various checks on the `LPToken.sol` contract instance, none of the checks can correctly identify if the instance in question is indeed deployed by the Native team or its partners. Adding a fake, malicious instance of `LPToken.sol` contract may lead to all of the ERC20 tokens in `CreditVault.sol` being drained.

Many of the functionalities in Native's system relies on processing transactions with calldata signed off-chain. These calldata include price quotations, request to remove collaterals, and requests for liquidations. If the calldata is processed on-chain at a sufficiently later time than the signature, it could lead to various adverse consequences due to the on-chain state or the market condition being very different from those at the time of signature generation. Some of the adverse effects include: * Traders being able to trade more than their capacity by exploiting the timestamp differences between `addCollateral()`, `tradeRFQT()`, and `removeCollateral()`. * The Native pools executing bad trades when market moves very quickly, particularly through the auto-sign feature. This risk was initially brought up with the Native team during the v1 security review, and the team considered this risk to be rather unlikely due to the fact that only trusted partners will be involved. However, there have been instances since the v1 security review where protocols have been exploited due to the off-chain signing mechanism being out of sync with the on-chain state or the market condition. Therefore, the auditors have chosen to highlight this issue again and increase the severity to Medium as this risk is concrete.

In the function `epochUpdate()`, the address `epochUpdater` can update the funding fees for traders at the end of each epoch. When browsing the assets to update for a given trader, a check makes sure that the exchange rate of the LP token does not increase by more than `1%`. However, it is possible to bypass this check and have an increase of more than `1%` by adding multiple times the same asset in the list.

The market makers facilitating trade on Native can charge a fee called the `widgetFee` from the swappers. The current maximum value allowed for this parameter is `10000`, which represents 100% of the swapper's input token amount being charged as fee. This should be lowered to mitigate the risk of a market maker overcharging the swapper from fees.

The `NativeRouter.sol` contract uses an internal function `_transferSellerToken()` to facilitate the transfer of token from the swapper (i.e., seller) to the market maker's treasury or the `NativeRouter.sol` contract. The contract handles the case where `msg.value > 0` (i.e., if the swapper wants to sell native ETH) by first wrapping the swapper's native ETH as WETH and then completing the transfer. However, there is another boolean argument `multiHop` used to enter this branch, and the wrapping only takes place if `msg.value > 0` AND `multiHop == false`. As a result, the native ETH will not be wrapped if`multiHop == true` even when the swapper somehow transfers ETH through the function call (`msg.value > 0`), resulting in excess ETH being held by the `NativeRouter.sol` contract that the contract owner must return using the `refundETH()` function.

The `CreditVault.sol`, `LPToken.sol`, `NativeRFQPool.sol`, and `NativeRouter.sol` contracts allow the owner to renounce ownership. While this functionality is standard, it poses a significant risk if triggered accidentally or maliciously, as the contracts rely on the owner's role to perform critical network functions. Once ownership is renounced, the associated privileges are permanently lost.

In the `ExternalSwapRouterUpgradeable.externalSwap()` function, there are two variables, `recipientDiff` and `routerDiff`, that record the token balance differences before and after the external contract call. However, if both values are non-zero (positive) and `recipientDiff >= routerDiff`, the `amountOut` will ignore the `routerDiff` and consider only `recipientDiff`. The intention might be that the `routerDiff` should also be transferred to the recipient and counted as part of the `amountOut`, as the function does not intend to collect funds for the router. In practice, we believe that only one of the two variables (`recipientDiff` and `routerDiff`) will be non-zero. The idea is likely to support external swap contracts that either swap to the `msg.sender` (which is the router contract in this case) or directly to the recipient.

The protocol assigns special privileges to several addresses. The privileges assigned to these addresses are rather strong, and may lead to a significant amount of lost funds if compromised. In addition, there is no mechanism to challenge the actions of these strongly privileged addresses (e.g., governance). We list the strongly privileged addresses and their privileges below: 1. The address `signer` in `CreditVault.sol` contract can sign any request to settle positions, remove collateral or liquidate positions. As a result, if compromised, it can: * sign malicious requests that will drain assets from users; * front-run as a liquidator a profitable liquidation opportunity submitted by another liquidator. 2. The address `owner` in `CreditVault.sol` contract can add a new market, enable/disable a supported market, allow a Native pool to spend tokens owned by the contract, whitelist/de-whitelist trader addresses and their associated settler addresses, whitelist/de-whitelist liquidator addresses, update the `signer` address, update the `epochUpdater` address, and update the `feeWithdrawer` address. As a result, if compromised, it can: * drain the protocol by whitelisting a malicious contract as a Native pool via `setNativePool()`, then allowing it to spend tokens owned by the contract via `setAllowance()`, and finally drain the vault from the malicious contract. 3. The protocol relies on the address `epochUpdater` in `CreditVault.sol` to call `CreditVault.epochUpdate() ` on time to properly update funding fees and collect/distribute accumulated fees. 4. The address `owner` in `LPToken.sol` contract can: * transfer and renounce ownership of the token; * pause/unpause the contracts, which will impact deposits and redeems; * update the minimum amount for deposits to any `uint256` value; * update to any `uint256` value the minimum interval to wait without sustaining early withdrawal penalties; * update the fee for early withdrawals from `0` to `100%`; * temporarily pause only redeems by setting `minRedeemInterval` with a high value and the early withdrawal fee to `100%`; * temporarily pause only deposits by setting `minDeposit` with a high value.

Many of the functionalities in Native's system relies on processing transactions with calldata signed off-chain. The format of the calldata is specified in `src/interfaces/IQuote.sol` contract under `struct RFQTQuote`. While we have generally assumed that the data is formatted correctly when the off-chain signatures are generated, we have not assumed data validity or data integrity. There are certain instances where additional checks on the values of the `RFQTQuote memory quote` input value may be useful. These instances are as follows: * The nonce check is missing in `NativeRouter.tradeRFQT()`, even though it is checked in `NativeRouter.tradeAutoSign()`. * In `NativeRouter._verifyAutoSignature()`, the values `quote.pool`, `quote.multiHop`, `quote.widgetFee`, `quote.effectiveSellerTokenAmount` are not checked. * In `NativeRouter._verifyWidgetSignature()`, the values `quote.quoteID` and `quote.amountOutMinimum` are not checked. * In `NativeRFQPool._verifyRFQSignature()`, the values `quote.pool`, `quote.effectiveSellerTokenAmount`, `quote.multihop`, `quote.widgetFee`, `quote.widgetFeeSignature`, `quote.externalSwapCalldata`, and `quote.amountOutMinimum` are not checked.