Maverick Protocol

In the contract Pool.sol the role `factory.owner` has authority over the function `claimProtocolFees()`. Any compromise to the `owner` account may allow a hacker to take advantage of this authority.

Our current understanding is that the `addLiquidity()` and `removeLiquidity()` functions in Pool.sol should be only called by the Router contract, such that the related token transferring logics are successfully triggered in Router. The `addLiquidity()` and `removeLiquidity()` functions in Router.sol are the actual accessible points for external users to interact with. If `Pool.removeLiquidity()` is meant to be called by external users, then the balance of base and quote tokens should be that of a `msg.sender` instead of that of the contract address (`address(this)`). In addition, there are some possible math simplifications, like in `addLiquidity()`, the `quoteFeeEscrow` is added to `totalQuoteBalance`, subtracted when calculating `quoteIn`, and finally used added with `quoteIn` again in the function call of `_setLiquidity()`

Using `address(this).balance` or `token.balanceOf(address(this))` as the `inputAmount` is risky. Function `swapEthForToken()` in contract Router and functions `sellTokenForToken()`, `sellEthForToken()` and `sellTokenForETH()` in ZeroExRouter are functions that swaps between tokens and tokens (or between ETH and tokens). When the direction is swapping ETH to token, the amount to be swapped should be the attached `msg.value` in the specific transaction. When the direction is swapping token to ETH or token to token, the amount to be swapped should be the `msg.sender`'s input amount, which need to be pointed out by the function caller as a function parameter. Take `ZeroExRouter.sellTokenForToken()` as an example. A userA want to sell tokenA for tokenB, and the recipient is userA itself. The userA should first transfer amountA of tokenA to the contract (`tx-a1`), and then call `sellTokenForToken(tokenA, tokenB, userA, minAmount, )` (`tx-a2`). A hackerZ can monitor the pattern of the two transactions. The hackerC can wait for the execution of userA's `tx-a1`, and then front run userA's `tx-a2`, with `sellTokenForToken(tokenA, tokenB, hackerZ, minAmount, )` (`tx-z1`) to take advantage of userA.

This finding is not a security issue. We noticed that `uint8` ,`uint16`, `uint32`, `int128`, `uint224`, etc. are used for variable type declaration. We would like to mention that all other `int` types except `int256` would be implicitly converted to `int256` during code execution, since EVM is designed and works with `256bit/32byte`. The implicit conversion would lead to extra gas cost. Here attached an example showing the gas cost: ```solidity pragma solidity 0.8.4; contract A { //69729 gas uint8 a = 0; } contract B { //69324 gas uint256 a = 0; } ```

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. As a result, an inconsistency in the amount will occur and the transaction may fail due to the validation checks.

In Router and ZeroExRouter, when the pool address is about to be used, it would be get from `factory.lookup()`. The return value of `factory.lookup()` is from the mapping of `pools[poolKey]`. However, there's no sanity check to validate if a pool exists. It is possible that the pool is never created, and the `pools[poolKey]` is the default `address(0)`.

Functions `Router.swap` and `ZeroExRouter.sellEthForToken()` are declared with the `payable` keyword. However, the `msg.value` is never handled if the function is invoked in a transaction. (This finding is related to CKP-01 Risks of Using `balance` of an Address for Swapping)

The contract is serving as the underlying entity to interact with third party 0x protocol and prb-math library. The scope of the audit treats 3rd party entities as black boxes and assume their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, migrating to new LP pools, etc. Note that ZeroExRouter.sol is relied on the liquidity function interface of 0x protocol.

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.