Primex Finance security record

The keeper reward mechanism exposes a risk to front-running. Currently, the `KeeperRewardDistributor.updateReward()` function is used to distribute keeper rewards. However, an attacker can exploit the system by monitoring the mempool and sending a transaction with a few input mutations, enabling them to fraudulently claim the reward without fulfilling the keeper effort. The `KeeperRewardDistributor.updateReward()` function currently calculates rewards based on the gas cost multiplied by a coefficient and the position size. In other words, larger rewards are given for larger position sizes, as long as the reward value exceeds the transaction gas cost. This incentivizes attacking bots to engage in front-running. The following is the list of impacted functions: 1. `BatchManager.closeBatchPositions()` 2. `LimitOrderManager.openPositionByOrder()` 3. `PositionManager.closePositionByCondition()`

Chainlink updates the feeds periodically (heartbeat idle time). The application should check whether the timestamp of the latest answer is updated within the latest heartbeat or within the time limits acceptable for the application (see: [Chainlink doc](https://docs.chain.link/docs/data-feeds/#check-the-timestamp-of-the-latest-answer)). The `getExchangeRate()` and `getOracleVolatility()` functions of the `PriceOracle` contract do not check the timestamp of the `answer` from the `IAggregatorV3(gasOracleAddress).latestRoundData()`.

The following contracts are susceptible to cross-chain signature replay due to a lack of chain id and expiration mechanisms in their signed messages: 1. The PMX bonus NFT allows for the signing of minting parameters, dictating the type of bonus that can be activated by its owner. While the signature hashes the `MintParameters` which include the chain ID, it lacks an expiration date. This means the parameters remain valid indefinitely and new NFTs can be always minted with the signed parameters. Given that only those with the `NFT_MINTER` role can access the mint function, this issue serves primarily as an informational note. 1. The referral whitelist enables users to enroll as referrers in the referral program. The signed message contains the contract name, function, and referrer. However, if the whitelist contract is deployed across multiple chains using the same signer, the signature remains valid indefinitely on all chains. This is due to the omission of both chain ID and an expiration date in the signature. 1. The referral system permits signing referral messages without incorporating expiration dates or chain IDs. This design allows the message to be used across any EVM-compatible chain, and without a set expiration, the referral message remains valid indefinitely. It is crucial to inform referrers of these cross-chain signature replay risks when generating signed messages. <br> For the replay attack on the referral contracts to be successful, certain additional conditions must be met. For instance, the signer must be whitelisted or be the admin in multiple chains. Additionally, based on our discussion with the PrimeX team, even though the protocol aims to be deployed on multiple chains, they plan to run the referral program on only one chain. Therefore, the risk is relatively low in this scenario. Nonetheless, we strongly urge addressing this issue despite the low risk, as the situation can change or the assumptions can be invalidated depending on the operation. It is crucial to proactively mitigate any potential vulnerabilities to ensure the long-term security and stability of the system. As a result, we escalated the severity to medium for this issue.

It is possible for a user to open a limit order position that can never be closed. If it is ever the case that the user's `amountIn` (deposit * leverage) divided by their limit price is greater than the `maxPositionSize` between the `deposit` and `position` asset, then the limit order will never be closed due to the check of the `maxPositionSize` in the `LimitPriceCOM.canBeFilled()` function. The severity of this issue is not as high since the user can still close the position themselves when they notice it. However, we strongly believe that this can be easily overlooked and can confuse users as to why the limit order is not executing. We suggest that the team fixes this issue for a better user experience and thus giving a medium severity. The following is the detailed analysis: Given the following scenario/assumptions: 1. Suppose a user deposits `depositAmount` of token A with leverage of `L`, and asks for a limit price `limitPrice` relative to token B. 2. Let `maxP = maxPositionSize[tokenA][tokenB]`. 3. The `amountIn` will be `L * depositAmount`, and suppose that `amountIn / limitPrice > maxP`. <br> **CLAIM**: All calls to the `LimitPriceCOM.canBeFilled()` function will return `false` given the user's position. To see this consider the following cases: * Case 1: `amountOut > maxP` * The call will return `false` since the position is greater than the max size. * Case 2: `amountOut <= maxP` * Note that `amountIn = price * amountOut`. Now suppose that the `price < limitPrice` (if this is not the case the call will return `false`). Then: * `amountIn = price * amountOut < limitPrice * amountOut` which implies: * `amountIn / limitPrice < amountOut <= maxP` which directly contradicts assumption 3. * Hence, it is impossible for the `price` to be lower than the limit price if `amountOut =< maxP`.

The protocol integrates with Aave, with generally low risk, and is only used prior to bucket launch. However, there are a few potential risks to highlight: 1. Aave may not have enough liquidity to withdraw if the token is lent out. 2. There is a non-zero risk of a blackswarm event causing Aave to become insolvent. 3. The `Bucket._withdrawAllLiquidityFromAave()` function does not check the real returned balance or balance difference between the `IPool(aavePool).withdraw()` call.

The `LimitOrderManager.createLimitOrder()` function allows traders to create limit orders and locks in the protocol fee charged to the trader based on the deposit size converted to native currency or the protocol token PMX at the time of order creation. Therefore, if the value of the deposit asset changes significantly before the order is executed, the effective protocol fee (in terms of the deposit asset) could differ from the originally locked-in fee. This could lead to a situation where the protocol either receives a lower-than-expected fee or where traders pay higher fees when the order is executed. Furthermore, the trader can update the protocol fee as long as the order is not executed through the function `LimitOrderManager.updateOrder()`. This mechanism could be exploited by traders to lock in a more favorable protocol fee, particularly in times of high deposit asset volatility.

The proxy contracts used in the Transparent Upgradeable Proxy pattern are compiled using solidity 0.8.10, but the implementation contracts are compiled using solidity 0.8.18.

In general, the protocol has some limits on what the token can be supported to interact: 1. The protocol only supports tokens with at most 18 decimals: the protocol aligns the "multiplier" on the decimals to 18 in several places: `BatchManager`, `LimitPriceCOM`, `TakeProfitStopLossCCM`, `BestDexLens`, and `PositionLibrary` contracts. The 18 decimals are also how the `WadRayMath` library handles the WAD precision. 2. The protocol does not support token charging fees on transfers: despite that the `TokenTransfersLibrary.doTransferFromTo()` function returns the balance difference as the output, all places calling the function do not use the output. In other words, all places calling the function assume to get the full amount on transfer. 3. The protocol does not support rebaseable tokens: the contract `TraderBalanceVault` maintains the accounting of token balances in `balances`. Tokens with elastic supply (also known as rebasing tokens) and tokens with dishonest implementation would not be compatible with the logic intended by this contract.

The `PrimexLens.getLiquidationPrice()` function calculates the liquidation price for a given position amount and borrowed amount. The function looks up the bucket address based on the provided `_bucket` name using the function `primexDNS.getBucketAddress()`. However, this function reverts when the bucket is not active. Therefore, the external `getLiquidationPrice()` viewer becomes unusable when the bucket is not active.

The `SpotTradingRewardDistributorStorage` contract declares the `dns` variable, but the `SpotTradingRewardDistributor.initialized()` function does not set its value. The `dns` variable is not used in `SpotTradingRewardDistributor`, but if there is a function that requires the use of `dns`, the variable should be initialized during a contract upgrade.