Primex Finance

In the PrimeX protocol, the keeper can attack orders using spot trades or swaps. The protocol includes slippage protection by comparing the DEX execution price with the oracle price data within a certain gap. However, this protection is not applied to swap and spot trade positions. As a result, if a limit order is created for a swap or spot trade, there can be high slippage without the oracle price gap protection. The limit orders do not have the slippage protection by `amountOutMin` as well.

When an asset was removed from a bucket through `removeAsset()`, the assets of all open positions may be transferred to the treasury. The affected traders will lose their entire position. The issue arises from the fact that `health()` in `PositionLibrary` returns 0 when `position.positionAsset` is not an allowed asset in the bucket. This can only occur if the asset was removed, as this position could not exist otherwise. Because `health()` returns 0, those positions can be closed through `closePosition()` with `CloseReason.RISKY_POSITION`. Such closures will lead to the traders not receiving any profits (`topUpAvailableBalance()` will not be called) AND not receiving their deposit back, as `decreaseTraderDebt()` will be called with `params.primexDNS.treasury()` as a receiver. Furthermore, when a position is closed due to liquidation, the trader will not receive any funds back. The deposit and potential profit will be sent to the treasury in the `PositionLibrary.closePosition()`.

The Chainlink Volatility Feed measures the [realized volatility](https://docs.chain.link/data-feeds/rates-feeds#realized-volatility) of one asset against another. Note that realized volatility can be greater than or equal to 100% (see: [formula here](https://www.wallstreetmojo.com/realized-volatility/#h-realized-volatility-formula)), and such an instance would create one of two problems: * In the case where the volatility is exactly 100% any user holding this asset would have a health of 0 and hence be able to liquidate. * In the case where the volatility is over 100% the calculation of a user's health will fail due to overflow and hence no one will be able to liquidate.

The protocol currently utilizes OpenZeppelin's `ECDSAUpgradeable` version `4.3.1` in contracts `PMXBonusNFT`, `ReferralProgram`, and `WhiteBlackListReferral` for validating registration using `ECDSAUpgradeable.recover(bytes, bytes)`. However, it is important to note that version `4.7.3` introduced a high-severity patch to address a [vulnerability related to signature malleability](https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-4h98-2769-gh6h) in the `ECDSA.recover()` and `ECDSA.tryRecover()` functions. These functions were found to be vulnerable due to accepting EIP-2098 compact signatures in addition to the traditional 65-byte signature format. This vulnerability affected the functions that accept a single bytes argument, not the ones that take `r, v, s` or `r, vs` as separate arguments. As a result, the `PMXBonusNFT`, `ReferralProgram`, and `WhiteBlackListReferral` contracts are impacted since they utilize the single bytes argument version of `ECDSA`, and version `4.3.1` is affected by this vulnerability.

In the `FeeExecutor`, lists `updatedTimestamps` and `indexes` are used to track all index and timestamp updates for a bucket. The function `FeeExecutor._searchApproxIndex()` is designed to return an approximate index corresponding to a given bonus deadline and the current index. However, its implementation, which employs a binary search approach, assumes that only one bucket updates are recorded in the `updatedTimestamps` and `indexes` lists. This assumption is incorrect because multiple buckets can update the lists using the function `FeeExecutor._updateIndex()`. Based on `FeeDecreaser` and `InterestIncreaser` functions, the `_updateIndex()` function pushes different bucket indexes which can be different based on the bucket's specific `bar` rate. This incorrect assumption can lead to inaccurate results returned by the `_searchApproxIndex()` function.

The `BestDexLens.getBestMultipleDexes()` function filters out DEX results and attempts to optimize share splits across DEX results to return the best DEX routing path for the amount requested. However, several concerns arise from the current implementation: 1. **Overwriting the Negative Value**: Within the loop, a condition checks if `amount` equals 0 and assigns `VERY_NEGATIVE_VALUE` to `amountByDexByShare`. However, in the subsequent lines, this value is immediately overwritten without any related checks. 1. **Unsafe Gas Estimation**: The gas, represented in ETH units (`vars.gas`), is directly added or subtracted to/from the `amount`. Mixing gas estimations in ETH with an amount that might represent another token or asset unit could lead to inaccurate calculations or misrepresentations of value. 1. **Unsafe Casting to `int256`**: The `amount`, an unsigned integer, is directly cast to `int256`. Without ensuring the `amount` is within safe bounds, this could introduce potential overflows or unexpected negative values. 1. **Arbitrarily Large Negative Value Concern**: A value `VERY_NEGATIVE_VALUE` defined as `-1e72` is used to indicate an undesirable amount obtained from the DEX. The problem arises when `currentValue` is assigned this large negative value and subsequently compared to other values using the conditional `_params.isAmountToBuy ? (currentValue < bestValue || bestValue == 0) : currentValue > bestValue`. If `_params.isAmountToBuy` is `true`, then a `currentValue` of `VERY_NEGATIVE_VALUE` will almost always be considered smaller than `bestValue`, even if `bestValue` represents a more desirable outcome. This bug can lead to incomplete or suboptimal solutions.

When batch closing positions through `BatchManager.closeBatchPositions()`, each position listed in the array `_ids` is validated. If the position does not exist through `PositionManager.getPosition()`, the function will catch the error and continue to the next position id in the `_ids` array. Therefore the `i` variable may not track to `vars.length` and `vars.length` can be less than `i`. ```solidity if ( _closeReason == PositionLibrary.CloseReason.BATCH_STOP_LOSS || _closeReason == PositionLibrary.CloseReason.BATCH_TAKE_PROFIT ) vars.closeConditions[vars.length] = positionManager.getCloseCondition(vars.ids[i], _conditionIndexes[i]); ``` Therefore, the variable `i` used to obtain the position close condition can be incorrect. Since `i` can be ahead of `vars` the position id can be zero. This will return the close condition from position id `0` for the current position `_ids[i]` and can incorrectly close the current position being evaluated.

The architecture graph suggests that exclusive roles should possess substantial privileges, which should be meticulously time-locked for enhanced security. The factories inherit the `UpgradeableBeacon` contract for beacon upgrades. However, the `UpgradeableBeacon` contract inherits the `Ownable` contract, making the deployer the owner and granting them the ability to upgrade implementations through the `UpgradeableBeacon.upgradeTo()` function (see: [code](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4d4a509b1f8e521583d82c042ef4bce919fe8e12/contracts/proxy/beacon/UpgradeableBeacon.sol#L52C14-L52C23)). As a result, the deployer holds a significant privilege to upgrade all contracts, highlighting the need to secure this role and its associated responsibilities.

The `ActivityRewardDistributor.withdrawPmx()` function enables the admin to withdraw unused PMX tokens from the contract. During the execution of this function, the `bucketInfo.totalReward` is reduced by the withdrawal amount. However, the `bucketInfo.rewardPerDay` and `bucketInfo.endTimestamp` remain unchanged. As a result, there may not be enough PMX tokens available to cover the reward until the end timestamp. Additionally, the `lastUpdatedRewardTimestamp` is not updated as expected. As a consequence, the time span since this timestamp was last refreshed is inaccurately accounted for during subsequent reward accumulation. Specifically, when rewards are withdrawn, the `bucketInfo.fixedReward` value increases based on the duration since the `lastUpdatedRewardTimestamp`. Failure to update the `lastUpdatedRewardTimestamp` can result in the period being double-counted if it is not properly updated.

According to the [documentation](https://docs.primex.finance/guides/credit-buckets/liquidity-mining), it states that "During the stabilization period, Traders can now borrow funds from the Credit Bucket, and Lenders start earning interest on the liquidity they provide for margin trading." Based on this information, we assume that lenders only start earning interest after moving to the stabilization period, which occurs when the bucket launches. Additionally, the implementation of `_depositLM()` does not update indexes and rates. However, the `withdraw()` function allows lenders to withdraw funds before the bucket launches. Within this function, both `_updateIndexes()` and `_updateRates()` are called, causing the index to be updated and start accumulating before the bucket launches.