UnibotX security record

`DiamondFactoryV3._closePosition()` Liquidation fees are applied to the amounts returned by `DiamondFactoryV3._removeUniLiquidity()` which are equivalent to the initial want token deposit minus the reserve and the amount borrowed with leverage. For example, assuming a liquidation fee of 5% and a 4x leverage with zero reserve ratio, the final fees applied on the original deposited amount turn out to be close to 20%, which is a high value. The issue described can lead users if liquidation is allowed for non-keeper addresses (owners of the position e.g) or keepers to get back a value even higher than the initial deposit (worst case scenario).

`DiamondFactoryV3._trySwapExactOutput()` uses the average price tick (`oracleTimeWeightedSec = 900`) to estimate the maximum `quoteAmount` and can be the reason why the swap will possibly throw leading `DiamondFactoryV3._trySwapExactOutput()` to revert due to the requirement stated below: ``` require(_amountIn < amountInWithSlippage, "Slippage protect"); ``` This will cause healthy positions to revert when closing them and possibly turn to unhealthy or even to bad positions. Please note that this issue depends on a random factor (the price volatility during the last `oracleTimeWeightedSec`), and it is hard to predict when a user will be able to claim his position again.

The lending pool used by the contract can be set and updated with the function `setLendingPoolAddress()`. Only the owner of the contract has the right to execute that function. However, if the lending pool is malicious or if it gets exploited, it gets the right to drain all the `borrowToken` owned by the contract.

`LendingPoolV3.repayBorrow()` checks if the caller (which should be factory) is in the whitelist. However, if a factory borrows from a lending pool, and then is removed from the whitelist, it will not be able to call `LendinPool.repayBorrow()`.

The contracts `DiamondFactoryV3` and `BalanceVaultV3` have no constructor but have a function `init()`. As a result, once the contracts are deployed, everyone can call the function `init()` and become the owner of these contracts.

The function `_findPositionIndex()` checks with a for loop if a user owns a given position based on its `_positionId`. However, the loop does not use the library `SafeMath` and uses an `uint8` variable which can store a maximum of 256 values. As a result, an overflow can happen on the variable `i`, which would lead to an infinite loop. Such a situation can be mitigated with the state variable `maxPositionNumber` of the contract `DiamondFactoryController`, but there is no upper limit for its value. If `maxPositionNumber` is not set properly, closing a position that has an id higher than 255 will cause the transaction to throw.

If a particular position reaches a `healthFactor < 1` it can be liquidated through a call to `DiamondFactoryV3.liquidate()`. This function checks that a caller is registered as a keeper in `DiamondFactoryController` when `requireKeeper` flag is set. However, a user can call `DiamondFactoryV3.closePosition()` on an unhealthy position he owns and bypass the keeper requirement imposed in `liquidate()`.

A reentrancy vulnerability is a scenario where an attacker can repeatedly call a function from itself, unexpectedly leading to potentially disastrous results. This is done through a function that does not respect Checks-Effects-Interactions. The [Checks-Effects-Interactions](https://solidity.readthedocs.io/en/latest/security-considerations.html#use-the-checks-effects-interactions-pattern) coding pattern is meant to mitigate any chance of other contracts manipulating the state of the blockchain in unexpected and possibly malicious ways before control is returned to the original contract. As the name implied, only after checking whether appropriate conditions are met and acting internally on those conditions should any external calls to, or interactions with, other contracts be done. All the functions listed below need to have a re-entrancy guard set: - `DiamondFactoryV3.liquidate()` - `DiamondFactoryV3.stopLoss()` - `DiamondFactoryV3.openPosition()` - `DiamondFactoryV3.closePosition()` - `DiamondFactoryV3.addCollateral()` - `DiamondFactoryV3.decreaseCollateral()` - `DiamondFactoryV3.collectFee()` - `DiamondFactoryV3.updateStopLossPrice()` - `BalanceVaultV3.deposit()` - `BalanceVaultV3.withdraw()` - `BalanceVaultV3.transferBalanceFromVault()` - `LendingPoolV3.deposit()` - `LendingPoolV3.withdraw()` Even if a function seems re-entrancy safe due to its interaction with a trusted third-party contract (known token contract, e.g.), it should not be trusted.

A common attack in DeFi is the sandwich attack. Upon observing a position opening or closing, an attacker can front-run the victim transaction by buying one of the assets, letting the victim execute the transaction, and then executing another trade after the victim by trading back the amount gained in the first trade. In both `DiamondFactoryV3._provideUniLiquidity()` and `DiamondFactoryV3._removeUniLiquidity()` the following parameters are not set: - When minting liquidity: - `MintParams.amount0Min` - `MintParams.amount1Min` - When decreasing liquidity (burning): - `DecreaseLiquidityParams.amount0Min` - `DecreaseLiquidityParams.amount1Min` As a slippage prevention mechanism, the official documentation of Uniswap V3 recommends that non-zero values should be used for the parameters `amount0Min` and `amount1Min` when using in production the functions: - `NonfungiblePositionManager.mint()` https://docs.uniswap.org/protocol/guides/providing-liquidity/mint-a-position#calling-mint ; - `NonfungiblePositionManager.decreaseLiquiditymint()` https://docs.uniswap.org/protocol/guides/providing-liquidity/decrease-liquidity#decrease-liquidity ; This best practice is not respected when the functions are called by the functions `_provideUniLiquidity()` and `_removeUniLiquidity()`.

Keepers play an essential role in the protocol when enabled. They liquidate insolvent positions and activate stop losses when a price tick goes below or above stop loss limits defined by the owners of positions. It implies that the protocol solvency highly depends on a limited set of addresses which could easily become single points of failure. On top of that, the function `removeKeeper()` is used to remove a keeper. However, the fact that the last allowed keeper can be removed is not checked. As a result, the protocol can remain without any keeper.