Primex Finance security record

The functions getAmountsIn(), getAmountsOut(), and getGas() do not implement the possibility of calculating beforehand the amount of tokens and gas that will be used to swap through the function _swapExactTokensForTokens(). Users will have to perform swaps by trial and error without previously knowing accurate calculation estimates.

The current implementation of the `Bucket` contract permits users to interact with the contract even after it has been delisted. This raises significant concerns, especially regarding the `Bucket.increaseDebt()` function, as it places additional responsibility and burden on the delisted bucket. Consequently, it becomes possible to acquire more debt through existing positions by reducing deposits using the `PositionManager.decreaseDeposit()` function. In this scenario, anyone can close these positions by specifying the close reason as `BUCKET_DELISTED`. It is important to note that the verification of the bucket status occurs in the `primexDNS.getBucketAddress()` function. This verification provides protection to the `Bucket.deposit()` function and the `PositionLibrary.createPosition()` functions implicitly, thereby limiting the attack factor to the `PositionManager.decreaseDeposit()` function.

Both keeper rewards and spot trading rewards are susceptible to being attacked through wash trading. The function `PositionManager.closePositionByCondition()` allows anyone to close an existing position as long as the close reason condition is satisfied in `PositionLibrary.closePosition()`. Unlike the function `PositionManager.closePosition()` where the reason is restricted to `CLOSE_BY_TRADER`, the `CloseReason` can be decided by the keeper where the reason can be `CLOSE_BY_TRADER`, `RISKY_POSITION`, `BUCKET_DELISTED`, or `LIMIT_CONDITION`. Therefore, in addition to close conditions, this function can be also used to close positions for liquidations, bucket deprecation, and traders. Since the keeper rewards are paid by the protocol's treasury, this close reason flexibility in the function and the ability to open and close positions in the same transaction, the `closePositionByCondition()` function allows anyone to steal keeper rewards by repeatedly opening and closing positions to drain treasury rewards through `CLOSE_BY_TRADER` or `LIMIT_CONDITION`. The malicious trader can close the existing position using `CLOSE_BY_TRADER` or setting the close condition where the newly created position can be closed in the same block. Similarly, the spot trading rewards can also be stolen through similar exploit scenarios by repeatedly opening and closing positions. When a user opens a position via `PositionManager.openPosition()`, their trading activity is recorded via `_updateTraderActivity()`. Crucially, the volume that they are treating in USD terms is added to their total trading activity in that period of time, which is a variable used to calculate the rewards they receive from the `SpotTradingRewardsDistributor` contract. However, when the position is closed by the user via `PositionManager.closePosition()`, the trading activity the user received before is not rescinded. Hence they can open and close positions multiple times in order to increase their trading activity without actually benefitting the protocol. In this scenario, rewards are determined using the formula: ``` reward += (periodInfo.totalReward * periodInfo.traderActivity[trader]) / periodInfo.totalActivity; ``` The formula added intricacy when evaluating the economic feasibility of the attack, as rewards are influenced by the overall trading activity of other users as well. This issue is mitigated by the fact that the protocol charges a protocol fee when opening the position. As long as the protocol fee is significantly larger than the sum of the rewards, the attack is mitigated because it would not be economically feasible. However, this also means that the deployment configuration value should be carefully vetted before launching the protocol, and some on-chain values need to be continuously monitored since some of the variants will not be under the protocol's control.

The function _swapWithArbitraryDex(), although private, can be called indirectly by users with malicious inputs that are note validated properly. One of the inputs is _params.dexRouter which is used as the target address of an external function call, making it possible for a malicious user to craft arbitrary transactions with input parameters that could damage the entire protocol.There are multiple attack vectors associated with having this functionality enabled, mostly relating to the fact that it is possible for an attacker to impersonate the vulnerable smart contract. More specifically, it may be possible to drain the contract of all its funds, native or tokenized, as well as perform unauthorized actions acting as the DexAdapter. By taking into consideration the permissions and authorized actions that can be taken as DexAdapter, the impact may be more or less severe.As an example, the provided proof of concept can be used to drain the contract DexAdapter of all its NATIVE_CURRENCY. Furthermore, by using a combination of calls, delegate calls and reentrancy, a malicious smart contract may abuse the doApprove() function to gain access to tokens owned by DexAdapter.

Provided protocol specification, and specifically the recent changes, regarding Oracle contract's implementation state that if Oracle Route's length is equal to four, then Pyth and Chainlink Oracles cannot be neither the first nor the last step in the whole path. This requirement was formulated to assure only expected token conversions are permitted. However, it was observed that the implementation of such mechanism is not complete. Namely, it does verify that Pyth and Chainlink Oracles are not the first step in the four-step route, but they do not assert they are not the last. Hence, the scenarios that were supposed to be prevented by design are actually possible.

The function setProtocolParamsByAdmin() does not implement access control allowing any user to delegate calls to PositionManagerExtension.While this does not present an immediate threat since all administrative functions on PositionManagerExtension possess proper access control, current or future functions that do not have any access control implemented may be at risk.

After EIP1559 the gas cost per transaction is split into a base fee and a priority fee. The priority fee is awarded to the validator of that block, whereas the base fee is burned. The priority fee can be set as high as the user wants, usually with the intention of increasing their likelihood of getting included in a block. In the scenario where a someone is both a keeper and a validator of a specific block, they have the opportunity to perform an attack which involves paying themselves an usually high priority fee in order to increase the rewards they receive as part of the gas refund. Since the rewards are given directly in terms of the value of the native token, such an attack could seriously harm the protocol's treasury. Note that, while such an attack is currently not possible on some L2s (such as Arbitrum) given their centralized sequencer model, it may be in the future if they transition to a more distributed architecture. The issue is mitigated by the fact that `KeeperRewardDistributor.updateReward()` function has a cap on the `maxGasPriceForReward` to be related to the gas price from the oracle. Additionally, the protocol charges a protocol fee when opening the position. As long as the protocol fee is significantly larger than the reward, the attack is mitigated because it would not be economically feasible. As a result, we lowered the severity for this issue.

It is crucial to validate inputs, even if the inputs come from trusted addresses, to avoid human error. A lack of robust input validation can only increase the likelihood and impact in the event of mistakes. Following is the list of places that can potentially benefit from stricter input validation: 1. qs-badge-fixed `Bucket.initialize()#L17`: the interface of `liquidityMiningRewardDistributor` should be validated. 2. qs-badge-acknowledged `Bucket.setMaxTotalDeposit()#L183`: the `_maxTotalDeposit` can be set lower than the current token supply. **Update:** The team clarified that this is a feature. 3. qs-badge-acknowledged `Bucket.setReserveRate()#L149`: add a validation on `_fee`. **Update:** the team clarified that the fee can be zero. 4. qs-badge-fixed `Bucket.setFeeBuffer()#156`: add a validation on `_feeBuffer`. **Update:** Added `_feeBuffer < WadRayMath.WAD + (WadRayMath.WAD / 100)` in the `Bucket` contract. 5. qs-badge-fixed `BucketsFactory.constructor()#L31`: the interface of `_bucketImplementation` should be validated. **Update:** Also, all the overwrote `upgradeTo()` functions the factories have the same check. 6. qs-badge-fixed `FeeExecutor.setTierBonus()#L44`: the `bonus[i].percent` should not be zero. The percent is verified in `_activateBonus` to confirm active status. 7. qs-badge-acknowledged `BonusExecutor.setMaxBonusCount()#L58`: the `_maxCount` should be set lower than the existing bonus count. **Update:** The team clarified that this is a feature. 8. qs-badge-fixed `PositionLibrary.increaseDeposit()#L211`: the `amountToTrader` should be greater than zero before updating the trader's balance in the trader balance vault. **Update:** Instead of adding a validation, a condition check is done before `traderBalanceVault.topUpAvailableBalance()`. 9. qs-badge-fixed `PositionLibrary.decreaseDeposit()#L288`: the `position.amount` should be greater than zero before updating the trader's balance in the trader balance vault. 10. qs-badge-fixed `PositionLibrary.closePosition()#L327`: the `vars.amountToReturn` should be greater than zero before updating the trader's balance in the trader balance vault. **Update:** Instead of adding a validation, a condition check is done before `traderBalanceVault.topUpAvailableBalance()`. 11. qs-badge-acknowledged `PositionLibrary.createPosition()#L555`: the price feed oracle should exist before creating the leveraged position. **Update:** The existence of the oracle is checked when adding a new asset to the bucket. 12. qs-badge-acknowledged `PositionLibrary.createPositionByOrder()#L631`: - the close condition parameters should be validated. **Update:** the team clarified that it is intended to not check the price of closing conditions. Other checks take place in the setCloseConditions in PositionLibrary. - check that `_params.order.depositedAsset != _params.order.positionAsset`. **Update:** the team clarified that the deposit asset of a margin position may be equal to the position asset. - make sure the token pair exists on DEX: `priceOracle.getPriceFeedsPair(_params.order.positionAsset, _params.order.depositedAsset)`. **Update:** the team clarified that adding new restrictions is unnecessary because the existence of the oracle between deposit and position assets will be checked on later stage in `PositionLibrary.openPosition()`. - if `!isSpot`, check that `position.bucket.allowedAssets(_params.order.positionAsset)`. **Update:** the team clarified that the allowedAssets is checked when creating an order. 13. qs-badge-acknowledged `PositionLibrary.openPosition()#L699`: the close condition parameters should be validated. **Update:** It is intended to not check the price of closing conditions. Other checks take place in the setCloseConditions in PositionLibrary. 14. qs-badge-fixed `PrimexPricingLibrary.getAmountIn()#L168`: the first token of `_params.routes.length` should be `_params.tokenA`. **Update:** Also, the team added the check that the last token is the tokenB. 15. qs-badge-fixed `PToken.unlockDeposit()#L88`: the `deposits` length should be greater than zero; otherwise, the function can unexpectedly revert. 16. qs-badge-fixed `PToken.depositProlongation()#L108`: the `deposits` length should be greater than zero; otherwise, it is possible to extend a non-existent locked deposit. **Update:** the `depositProlongation()` function no longer exists. 17. qs-badge-fixed `PositionManager._onlyExists()#L767`: the `positions` length should be greater than zero; otherwise, it is possible to return true for a non-existent position when the positions list is empty. 18. qs-badge-fixed `LimitOrderManager.orderExists()#L457`: the `orders[index]` length should be greater than zero; otherwise, it is possible to return true for a non-existent order when the `orders[index]` list is empty. 19. qs-badge-fixed `LimitOrderLibrary.createLimitOrder()#L357`: the deadline should be validated when creating limit orders. 20. qs-badge-acknowledged `LimitOrderLibrary.creat

Numerous functions within the `BestDexLens` contract involve iterating through multiple dexes and conducting intricate calculations to determine the optimal route. This concern becomes particularly pronounced when an increasing count of dexes is integrated into the protocol, raising apprehensions about the computational load leading to potential out-of-gas errors. The central problematic function is `getBestMultipleDexes()`, which iteratively traverses a dex list. This function, in turn, is invoked by `getBestDexForOpenablePosition()`, `getBestDexByPosition()`, and `getBestDexByOrder()`. We consider the severity as low because, in the scope of this audit, the functions are only used in the `PrimexUpkeep.checkUpkeep()` function. From our understanding, the `checkUpkeep()` is only intended to be for a static call to simulate the order execution and retrieve a filtered result that can be performed. However, if any of the functions are used outside our knowledge unexpectedly, the severity can be higher.

If the owner renounces their ownership, all ownable contracts will be left without an owner. As a result, any function guarded by the `onlyOwner` modifier will no longer be able to be executed, such as upgrading the `UpgradeableBeacon` implementation contract. Furthermore, critical role transfers are not following a two-step pattern. The two-step pattern ensures that there is an additional layer of verification and authorization before transferring critical roles, reducing the risk of unauthorized access or malicious actions.