Primex Finance security record

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.

In both the `setupBucket()` and `decreaseRewardPerDay()` functions of the `ActivityRewardDistributor` contract, if `rewardPerDay` is equal to zero, the `bucketInfo.endTimestamp` is overridden to `type(uint256).max`. Additionally, the `getBucketAccumulatedReward()` and `_updateBucketInfo()` functions have the logic of `endTimestamp += unusedTime` when `rewardPerDay == 0`. The line `endTimestamp += unusedTime` will overflow if the `endTimestamp` is set to `type(uint256).max`. Consequently, almost all functions relying on `getBucketAccumulatedReward()` and `_updateBucketInfo()` will fail. From our discussion with the team, the `rewardPerDay` can be set to zero as emergency mitigation in case there is a bug to stop the accrual of incorrect rewards. Despite the likelihood of this occurring being low, it would cause extra chaos and worsen the emergency situation, which is why it is a medium-severity issue.

The `Reserve` currently uses `pToken.scaledTotalSupply()` instead of actual token supply `pToken.totalSupply()`, when validating the minimum percentage of token supply in the `Reserve` during the function `transferToTreasury()`. This methodology could result in inaccuracies in the calculated minimum percentage, which can lead to a lower than expected minimum percentage in the reserve. ``` _require( reserveBalance >= (restristions.minAmountToBeLeft + amount) && reserveBalance >= (pToken.scaledTotalSupply().wmul(restristions.minPercentOfTotalSupplyToBeLeft) + amount), Errors.NOT_SUFFICIENT_RESERVE_BALANCE.selector ); ```

When closing a spot trade position (or more specifically, on `vars.borrowedAmountIsNotZero` in `PositionManager._closePosition()#L682-690`) through `PositionManager._closePosition()` the `oracleTolerableLimit` is set to zero between `depositAsset` and `positionAsset`. This means when closing the position through `PositionLibrary.closePosition()`, the swapped amount returned by the DEX in the `PrimexPricingLibrary.multiSwap()` must be equivalent to or better than the price provided by the price oracle. This check is validated in the `multiSwap()` where the `getOracleAmountsOut()` must be greater or equal to the amount obtained from the DEX because `_needOracleTolerableLimitCheck` is set to `true` in the `PositionLibrary.closePosition()` function. This strict requirement can prevent spot traders from closing their positions.

The function getEstimatedMinProtocolFee() does not account for gas calculations for the newly introduced Optimistic chain integration. The variable l1CostWei will always yield a value of 0. While this may be intended, it does not conform with code implemented across other smart contracts on the protocol.