PerpDex - K-Bit

The checkPositionSizeAndIncrease function is currently executed in _createRequestPosition, which is problematic as it checks for the total position size (both currentLong and currentShort) at the time of the request, not at the time of execution. This creates a risk of exceeding maxLong or maxShort limits if other transactions (such as limit orders or other market positions) are executed between the request and the actual execution. As a result, the contract may allow opening positions beyond the allowed limits, leading to potential imbalances and risks to the contract's integrity.

Move the checkPositionSizeAndIncrease validation from _createRequestPosition to the execution stage. This ensures that position size limits are checked at the time of execution, accounting for any changes in the market or other transactions that may have occurred in between. This should apply to both the openMultiplePositions and openSinglePosition functions to ensure consistency.

Here’s how the validation can be implemented at the execution stage:

1. Remove Check from _createRequestPosition: Eliminate the call to checkPositionSizeAndIncrease in the request creation process, as this check is premature.

2. Add Check During Execution: Implement the checkPositionSizeAndIncrease check when executing the positions in both openMultiplePositions and openSinglePosition functions. This ensures that the position size is validated at the time of execution.

SOLVED: The Request Position function removed. Now it is just user sign and openPosition call without a request state.

The mergePosition function in the PerpDexContract does not check the status of the new position being merged, which can result in merging an already opened position—a scenario that should not occur. Additionally, the function performs a truncated leverage equality check, meaning positions with slightly different leverages (e.g., 3.5 vs 3.7) will be considered equal due to rounding, even though their actual leverages differ.

While the truncated leverage check might seem important at first, it is unnecessary because the liquidation price is correctly recalculated based on the total size and margin after merging, which inherently accounts for any leverage differences.

1. Position Status Check: Add a check to ensure that the new position being merged is not already open. This avoids unintended merges of open positions, which could cause unexpected contract behavior.

2. Remove Leverage Check: Since the liquidation price is recalculated based on the total size and margin, remove the leverage equality check altogether, as it adds unnecessary complexity without providing additional security or correctness.

SOLVED: The leverage comparison was removed.

The LPContract allows the usdt address to be updated multiple times via the setUsdtAddr function. This can introduce severe issues if a token with different decimals is set as the new usdt. For example, if a token with more decimals is used, users could withdraw less than intended, while a token with fewer decimals could result in over-withdrawing. This also applies to the Fee contract and the perpDex variable. Allowing updates to these addresses without checks creates a risk of inconsistencies in user balances, leading to potential financial loss.

The setUsdtAddr function should be restricted to only allow setting the token address once, ideally during contract initialization. If updates are necessary due to migrations, ensure the decimals of the new token match the previous token's decimals. This will avoid inconsistencies in balance tracking. Moreover, setting the usdt and perpDex addresses during initialization reduces the need for such updates during contract operation.

SOLVED: The contract is now checking for the decimals being always 6.

The signature verification system across the PerpDexContract does not account for potential cross-chain deployments or migrations. The current signature validation lacks inclusion of critical information such as chainId and contract address, which means that signatures created for one instance of the contract can be reused on another contract, either on the same chain or across different chains. This creates a significant risk, as an attacker could reuse a valid signature from one contract or chain to perform unauthorized actions on another.

Without incorporating chain and contract-specific data into the signature, the same signature is valid across multiple deployments or migrations, making it vulnerable to replay attacks and unauthorized actions on other networks.

1. Implement EIP-712 Standard: Use the EIP-712 standard for structured data signing, which is specifically designed to prevent cross-chain and cross-contract signature reuse. EIP-712 signatures include both chainId and address(this) by default, ensuring that signatures are bound to a specific chain and contract instance.

2. Manual Inclusion of chainId and address(this): If you decide not to implement the full EIP-712 standard, at a minimum, manually include the current chain's chainId and the contract address (address(this)) in the message hash for both admin and user signatures. This ensures that signatures are valid only for the intended contract instance on the current chain.

SOLVED: All actions are now checking unique user data an validating against it one time usage nonce.

In the closePosition function, the user’s signature (userSignedData) is validated using the checkUser function. However, the message used for signature comparison is parameter-controlled and does not include any critical data, such as the position ID, leverage, or other essential parameters related to the action. This allows the onlyCloseAdmin to reuse any previously valid signature from the user, even if it was signed for an unrelated action in another protocol or chain. As a result, the onlyCloseAdmin could maliciously close positions on behalf of the user without explicit permission.

This vulnerability arises because the signature is not tied to the specific parameters of the transaction, allowing signature replay and misuse across different contexts.

1. Incorporate All Critical Parameters into the Signature: The signature should include all relevant transaction data, such as positionId, tokenType, size, leverage, and other key details. This ensures that the signature is specific to the close position request and cannot be reused for other actions.

2. Implement a Nonce System: Add a nonce to the signature to ensure that each signature can only be used once. This prevents signature replay attacks and ensures that a signature is valid only for a single transaction.

3. Use EIP-712 Structured Data Signatures: Implement EIP-712 for signing structured data. This ensures that the signature verification is both secure and efficient, and it provides a robust way to sign and verify structured data across different contexts.

SOLVED: The code is not creating a unique message to be signed using getClosePositionMsg, which has the position id.

The Fee contract's setFeePercent and registerReferrer functions allow the contract owner to reuse any previously used signature for a user. This opens up a significant attack vector where signatures (such as those used in protocols like Permit2) can be exploited to change fee percentages or referral settings without user consent. The owner could generate the correct message and maliciously reuse a signature to change the user's fee structure. Since the message field is controlled by the owner, the system is susceptible to replay attacks. Additionally, the absence of a nonce mechanism allows the same signature to be reused indefinitely for different operations. Furthermore, there's no distinct separation between signatures used in setFeePercent and registerReferrer, allowing signatures to be cross-used between these functions.

1. Message Uniqueness and EIP-712 Compliance: Modify the message field to include contract-specific data such as address(this) and implement EIP-712 domain separators to ensure that the message is unique for each contract and operation. This will prevent the reuse of signatures across different protocols or functions.

2. Include Fee Data in Signatures: Ensure that the setFeePercent function requires the user to sign a message containing the exact fee percentages they agree to (both _protocolFeePercent and _referrerFeePercent), preventing the reuse of signatures for different fee structures.

3. Nonce System: Implement a one-time-use nonce system to ensure that each signature can only be used once. This will prevent replay attacks and ensure that the user has explicit control over each operation.

4. Consider Removing Signature Functionality: If the signature-based system is not providing any significant benefit and introduces unnecessary risks, consider removing it altogether. This would centralize the risk to the owner, but with proper control measures, the system may be more secure than allowing potentially exploitable signature logic.

RISK ACCEPTED: traderNonce is now in-place to prevent signature replay. However, admins can still call the function with any arbitrary data not signed or agreed by the user. Client stated that: A problem we are having is that the referrer are KOLs and they would rather not disclose their addresses to their referees (or at least make it less obvious). That is why I've tried to hide the referrer address during signing. (Even though they can see it if they rummage through kaiascan)

The current protocol implementation does not account for potential transfer fees that can be imposed by USDT (or any other tokens with transfer fees). Certain versions of USDT and other tokens may apply a small fee to transfers, meaning the actual amount received by the recipient will be less than the amount transferred. This can cause inconsistencies in contract logic, such as insufficient funds during withdrawals, incorrect balances, and miscalculations of profits or losses.

For example, in functions like withdraw, giveProfit, or any other operation involving USDT transfers, the contract assumes that the full transfer amount will be received, but in the case of a token with transfer fees, the recipient receives less than the intended amount.

1. Check for Transfer Fees: After every safeTransfer or safeTransferFrom operation involving USDT, check the actual balance of the contract or user before and after the transfer to ensure the correct amount was received. If the token has a transfer fee, adjust the contract’s logic accordingly.

2. Update Balance Calculations: Modify balance-tracking mechanisms to account for any differences between the transferred and received amounts, ensuring that the protocol doesn’t assume that 100% of the transferred amount reaches the recipient.

SOLVED: A new function named safeTransferAndCheckBalance was implemented, it does prevent the transfers of tokens that may have fees.

The calculateLiquidationPrice function suffers from rounding issues because the leverage is calculated using integer division. This truncation causes the liquidation price to deviate significantly from the expected theoretical value, particularly when using large positions with high leverage.

For example, consider a long position with:

- margin = 1800
- size = 50000
- initialPrice = 50000
- feeDenominator = 100000
- totalFeePercent = 100

The correct leverage is:

leverage = size / margin = 50000 / 1800 = 27.777777...

Using the theoretical liquidation price formula:

liquidation_price = (feeDenominator * initialPrice * (leverage - 1)) / ((feeDenominator - totalFeePercent) * leverage)

The correct liquidation price should be:

liquidation_price = (100000 * 50000 * (27.777777 - 1)) / ((100000 - 100) * 27.777777) ≈ 48248.24

However, with integer division (used in Solidity), the leverage becomes truncated:

leverage = size // margin = 50000 // 1800 = 27

This results in an incorrect liquidation price:

liquidation_price = (100000 * 50000 * (27 - 1)) / ((100000 - 100) * 27) ≈ 48196

This difference, caused by truncating the leverage, leads to incorrect liquidation prices: - For long positions, the liquidation price is higher than expected, delaying liquidation. - For short positions, the liquidation price is lower than expected, triggering liquidation too early.

To correct this, apply a scaling factor to the leverage calculation to preserve precision. If prices and margins use 6 decimals (e.g., 1 USDT = 1e6), the factor should be 1e6. This avoids rounding issues and ensures accurate liquidation price calculations. The revised formula is:

leverage = (size * factor) / margin;  // Where factor = 1e6 (6 decimals for price)

This will result in the correct liquidation price calculation:

liquidation_price = (100000 * 50000 * (leverage - 1e6)) / ((100000 - 100) * leverage) ≈ 48248

By applying this scaling factor, the liquidation price will closely match the theoretical value, preventing errors when users attempt to close positions between the real and computed liquidation prices.

SOLVED: 1e6 factor used in leverage calculation.

In the QaPrice contract, the latestRoundData function returns a fixed value of 1000 for updatedAt, which is incorrect and could be detected as a stalled price update. This can lead to invalid contract states, permanent deadlocks, or issues in dependent contracts (such as PerpDexContract). Additionally, getRoundData always returns the current block.timestamp for updatedAt, rather than the time at which the asset’s price was last updated. This behavior can create issues, especially where checks depend on accurate timestamps, such as in the getBisonAIPrice function, where prices must reflect recent updates. Moreover, allowing a price update of 0 could cause further complications in the system.

Ensure that the updatedAt field in latestRoundData reflects the actual time when the corresponding update*Price function was called for each asset, rather than returning a static value of 1000. For getRoundData, return the time when the update function was last executed, ensuring the data reflects the real update time. It is also advisable to prevent any updates that attempt to set the asset price to 0, as this could disrupt the functioning of the dependent contracts and lead to incorrect contract states.

SOLVED: QaPrice contract was removed.

In the submitAndGetBisonAIRoundId function, the logic incorrectly assumes that the priceData.feedHashes.length will always correspond to the number of TokenType elements (which range from BTC to PEPE, i.e., indexes 0 to 5). However, this is problematic because if the length of feedHashes exceeds the number of TokenType values (6), it causes an overflow when mapping indexes to TokenType, as PEPE (index 5) is the maximum valid value. This could result in incorrect price submissions or even skipped updates for valid tokens, leading to incorrect pricing and potential financial loss for users.

Refactor the code to either: 1. Iterate Over TokenType Enum: Ensure that the loop iterates over the valid TokenType enum (i.e., from 0 to the maximum value, 5 for PEPE). This avoids any overflow or mismatch issues between feedHashes and the actual token types.

1. Validate feedHashes Length: Alternatively, ensure that feedHashes.length is strictly equal to the number of TokenType elements (6). If feedHashes.length exceeds this limit, throw an error to prevent processing invalid data.

By enforcing these checks, you can ensure that price submissions correctly correspond to the appropriate token types without risking overflows or incorrect data mapping.

SOLVED: Feed hash length checked.

In the _closePosition function, when closing a position with a loss in the exceptional case where liquidation didn't run, the current logic incorrectly checks if (position.size <= loss) instead of checking the available margin. Since the margin represents the available amount for the position, the check should compare the loss against the margin, not the size of the position. If the loss exceeds the margin, the position should have been liquidated.

By checking against the size instead of the margin, the logic allows scenarios where losses larger than the margin can be processed without proper liquidation, which leads to incorrect handling of positions and potential financial discrepancies.

Replace the current if (position.size <= loss) check with if (position.margin <= loss). This ensures that any loss greater than the margin is treated as an exceptional case where liquidation should have occurred, and proper steps are taken to handle the loss correctly.

SOLVED: This logic was removed and simplified.

In the getPreviousPriceAndTime function, the parsePriceFeedUpdates is called with a timestamp range (current or current + 5 seconds). However, the subsequent loop includes a strict comparison to check if the timestamp in the price feed exactly matches the expected value. This can lead to the function reverting, as it does not account for the flexibility introduced by the timestamp range in the parsePriceFeedUpdates call.

Strict equality between the timestamp returned by parsePriceFeedUpdates and the requested timestamp is not always achievable due to block time variations, network latency, or oracle update intervals. The check needs to account for the entire range provided during the request, ensuring that valid prices within the expected time window are accepted.

Adjust the timestamp check in the loop to allow for a comparison within the provided time range. Instead of checking for strict equality, verify that the returned timestamp falls within the range of priceData.timestamps[0] to priceData.timestamps[0] + 5 seconds. This will ensure the comparison is valid and avoids reverting unnecessarily.

SOLVED: The +5 range was removed from the check.

The current implementation allows positions to be opened using one oracle feed (e.g., BisonAI) and closed using another (e.g., Pyth). This presents a significant arbitrage opportunity if there is a substantial price difference between the two feeds. A malicious actor could exploit discrepancies between feeds by opening a position with one data feed at a lower price and closing it using another feed at a higher price (or vice versa), generating guaranteed profits without any real market risk.

This vulnerability is exacerbated when using automated bots or external systems to trigger market or limit trades, where different data feeds might be swapped between actions. Additionally, if the front-end (UI) shows users prices from one feed but signs transactions based on another, users could unknowingly open positions with misleading data, leading to unexpected outcomes.

To prevent arbitrage and inconsistencies caused by different data feeds, the system should enforce that all trading actions (opening, modifying, closing) for a particular position must use the same data feed. This ensures consistency in the pricing information used throughout the lifecycle of a position, avoiding opportunities to exploit differences between oracle feeds.

Solution Outline:

1. Track Data Feed per Position: When a position is opened, store the specific oracle feed type (e.g., BisonAI, Pyth, etc.) used to open that position. This data feed should be associated with the position and enforced throughout all actions related to that position (such as closing or updating it).

2. Enforce Feed Consistency: Ensure that all subsequent actions (like closing the position, liquidating, etc.) must use the same oracle feed type as the one used during the opening of the position. If an action is attempted with a different feed, it should be rejected.

3. UI Integrity Check: Ensure the front-end (UI) is displaying data and signing transactions using the same oracle feed for consistency. This prevents discrepancies between what the user sees and the data being used for transactions.

RISK ACCEPTED: The function is being called by the admins only, this means they have full control of the parameters and which oracle they will use. This is being used as a backup method and considered a backend issue.

In the PerpDexContract, the set.*Admin functions (such as setLiquidationAdmin, setLimitOrderAdmin, etc.) do not check for duplicate entries when adding new admin addresses. This can lead to inefficient operations, as duplicate admin entries may unnecessarily inflate the array size and lead to redundant permission checks. Additionally, if an admin address is added multiple times, operations that rely on the admin list may behave unexpectedly or inefficiently.

Add checks in the set.*Admin functions to ensure that duplicate admin addresses are not added to the admin arrays. Before adding a new admin, verify that the address is not already present in the corresponding admin list. This can be achieved by iterating over the existing list and rejecting the addition of a duplicate.

NOT APPLICABLE: Always deleted before set.

The LPContract, QaPrice, Fee, and PerpDexContract all use a single-step ownership transfer system, which could introduce security risks if ownership is accidentally transferred to the wrong address or if the owner's private key is compromised during the transfer process. In a single-step system, ownership is transferred immediately upon calling the transferOwnership function, leaving no buffer for validation or correction.

It is recommended to implement a two-step ownership transfer mechanism. In this approach, the current owner should nominate a new owner, and the nominated owner must explicitly accept the ownership within a specific timeframe. This provides an additional layer of security and ensures that ownership transfer is deliberate and verifiable.

RISK ACCEPTED: The Zygo team accepted the risk of this finding.

In the PerpDexContract, the getBisonAIPrice function ensures that prices are always updated at the exact block.timestamp, but the getPythPrice function allows a 10-second window by calling getPriceNoOlderThan. This creates an inconsistency where Pyth price data might be outdated compared to the stricter timestamp check in the BisonAI implementation. Additionally, there is a TODO comment indicating that a check for price freshness has not been implemented. The absence of consistent timestamp validation and the use of a static 10-second window can lead to using stale prices, which poses a risk of financial losses when opening or closing positions based on outdated data. Moreover, prices with a value of 0 are not being handled properly, which can result in faulty operations.

The getLatestTokenPrice function, which aggregates prices from different oracles, lacks proper timestamp validation for all three oracles. This function should ensure that prices are updated at the same block.timestamp for each oracle to prevent inconsistencies. Furthermore, the answer from the price feed should be checked to ensure it is non-zero to avoid erroneous data.

1. Timestamp Validation Across All Oracles: In the getLatestTokenPrice function, add a check to ensure that the updatedAt value (the third parameter in price feeds) from all oracles matches the current block.timestamp. This ensures that all transactions using price data are based on up-to-date information. Apply this check to BisonAI, Pyth, and any other oracles used.

2. Dynamic validTimePeriodSeconds for Pyth Oracle: Replace the static 10-second window with the dynamic validTimePeriodSeconds parameter, which can be initialized based on the oracle's specific requirements (e.g., 120 seconds for Ethereum). Use this value when calling getPriceNoOlderThan(id, getValidTimePeriod()) to ensure consistency.

3. Non-Zero Price Check: Ensure that the answer from any oracle is non-zero before proceeding with any price-based calculations. If answer is zero, reject the price as invalid.

4. Implement Up-to-date Price Check: Complete the "TODO" for verifying if the price is up-to-date by comparing updatedAt with the current block.timestamp and reject any outdated price feeds before proceeding with transactions.

RISK ACCEPTED: A zero check was added, but the updatedAt cannot be checked in Pyth, and the TODO was taken care of by adding the 10 seconds in getPriceNoOlderThan().

The current implementation of the mergePosition function in the PerpDexContract requires external calls to specify when two positions should be merged. This can lead to integrity issues, as the merge relies on the caller to explicitly identify the positions to merge. This introduces an extra layer of complexity and potential risk if the positions are not merged when they should be, or if incorrect positions are merged.

Additionally, since merging two positions of the same direction (long or short) is a common operation, the system could be designed to automatically handle this when opening a new market or limit order. This would reduce user or system error and ensure position integrity.

Instead of relying on external calls to initiate merging, implement automatic merging logic within the openLimitOrder and openMarketOrder functions. When a user opens a new position, the system should automatically check if they already have an open position in the same direction (long or short) and merge the positions on-chain if necessary. This will streamline operations and improve contract integrity by ensuring all same-direction positions are always merged without requiring manual intervention.

SOLVED: The merge is now happening automatically when a position is opened and if the user has already another ongoing position. However, it was stated that current deployed versions of the contracts had a zero positionId, this means that if the owner of the position id 0 does open a new position that should merge in reality it will be considered as none-existent and cause a new position ID to be created.

The submitPrice function in the PerpDexContract allows empty proofs to be passed, which can result in the updatePriceFeeds call from Pyth (or submitStrict for BisonAI) to skip price updates. This means the contract could use stale or outdated prices when opening or closing positions, potentially leading to incorrect pricing and financial losses for users. This vulnerability could be exploited to manipulate positions and profit from stale price data. Furthermore, the function currently sets the value parameter for updatePriceFeeds based on the length of feedHashes, whereas it should be set according to the length of the proofs array, as required by the Pyth implementation.

The same issue applies to the submitStrict function, where it allows all elements of OraclePrices to have a length of zero, effectively skipping updates for all price feeds. This behavior can leave the system vulnerable to stale prices and user fund loss.

References: https://github.com/pyth-network/pyth-sdk-solidity/blob/main/MockPyth.sol#L48, https://github.com/Bisonai/orakl/blob/077c67fc3911076f37bf980dccd9e127c4e97217/contracts/v0.2/src/SubmissionProxy.sol#L413

1. Non-empty proofs Check: Ensure that proofs passed to the submitPrice function are non-zero length and properly validated. Any empty proofs should be rejected to prevent skipping price updates.

2. Signature Verification for User Interaction: When the function interacts with user-provided data, implement signature verification to ensure the authenticity of the data and prevent malicious submissions with invalid or empty proofs.

3. Correct value Calculation: Set the value sent to the updatePriceFeeds function based on the length of the proofs array, not feedHashes. This aligns with the expected behavior in Pyth's implementation.

4. Reject Zero-Length OraclePrices: In submitStrict, enforce that all elements of the OraclePrices array have non-zero lengths to ensure valid and timely price updates are processed.

SOLVED: The code is now checking for proof length.

The current test suite relies heavily on mocking all calls to Pyth and other oracles, which leads to an incomplete and potentially misleading test coverage. This approach hides potential issues related to real-time interactions with oracles, price feed updates, and edge cases related to live data. Additionally, with less than 50% test coverage, critical areas of the contract are not adequately tested, increasing the risk of undetected bugs, especially in scenarios that involve real data flows from external oracles.

Mocking oracle interactions in all cases prevents the detection of issues that could arise from delays, inaccuracies, or unexpected behavior in the oracle data during live deployment. This reduces the reliability of the test suite and limits confidence in the contract's behavior in production environments.

1. Fork the Chain for Pre-defined Oracle Tests: Instead of relying solely on mocked oracle data, use chain forking to create a controlled environment with real-time oracle data. By forking the blockchain to a specific block where the oracle values are known, you can accurately test the contract’s interactions with Pyth and other oracles in real conditions. This approach allows you to validate the accuracy of oracle-related logic, such as price feed updates and time-sensitive data.

2. Increase Test Coverage: Aim for at least 80% test coverage by creating comprehensive test cases that cover all critical paths, including real oracle interactions, liquidation scenarios, position opening/closing, and exceptional cases like liquidation failure. Ensure that the test suite includes both happy paths and edge cases, such as when oracle prices are outdated, or when multiple transactions are competing for the same position.

SOLVED: Test coverage has been heavily increased by the test suite.

The Fee contract includes a pause/unpause functionality but does not use it effectively, as the whenNotPaused or whenPaused modifiers are not applied to any critical functions. Additionally, both pausing and unpausing are controlled by the same role (the owner), which could present a risk if the owner loses access or behaves maliciously. In case of emergencies, it is good practice to separate the control of these two functionalities.

Implement the whenNotPaused and whenPaused modifiers on critical functions where you might need to halt operations during an emergency or security breach. Additionally, create two separate roles: one for pausing the contract, which could be assigned to a security admin, and another for unpausing, reserved for the contract owner. This separation adds another layer of security and flexibility for managing the contract during critical situations.

NOT APPLICABLE: The functions are already being used.

Both the Fee contract and PerpDexContract rely on signature verification for various operations (such as referral registration and position management). However, these contracts do not support EIP-1271, which enables contracts to validate signatures, limiting functionality to externally owned accounts (EOAs) only. Without EIP-1271 support, users interacting with these contracts via smart contract wallets or multi-sig wallets will be unable to perform signature-based actions, potentially restricting a significant portion of the user base.

Implement EIP-1271 support in both the Fee and PerpDexContract to allow signature verification for non-EOA addresses. This can be achieved by checking whether the signer is a contract and, if so, verifying the signature using the isValidSignature function defined in EIP-1271. This will enhance compatibility with contract-based wallets and multi-signature wallets, ensuring broader accessibility to users.

ACKNOWLEDGED: Smart contract not widely used in Klaytn at the moment. Perhaps a future task when there is needed.

The submitAndGetBisonAIRoundId function in the PerpDexContract cannot be executed when the contract is paused. This can create operational issues if price submissions or oracle interactions are needed during emergency pauses. This function is crucial for updating and verifying prices, and if it cannot be called during a pause, it could lead to stale prices or interruptions in functionality, causing contracts that depend on this data to malfunction.

Consider allowing the submitAndGetBisonAIRoundId function to be callable even when the contract is paused. This can be done by removing the whenNotPaused modifier from this function or by implementing a special access control mechanism that allows critical functions like price submissions to bypass the pause state. Ensure that only trusted roles can perform such actions during a paused state to maintain security.

SOLVED: Modifier removed from the function.

The checkPriceDataOrder function in the PerpDexContract duplicates logic that is already handled by the checkPythPriceFeedOrder function. This leads to redundant code and increases the risk of inconsistencies if one function is updated, but the other is not. Maintaining multiple sections of similar code complicates future maintenance and can introduce bugs.

Refactor the checkPriceDataOrder function to reuse the logic from checkPythPriceFeedOrder for Pyth oracle price feed validation. This consolidation will reduce duplication and ensure consistent behavior across both functions.

NOT SOLVED: The getPreviousPriceAndTime function is calling checkPriceDataOrder and in case of pyth, the checkPythPriceFeedOrder variant which duplicates the checks.

The current implementation in PerpDexContract uses updatePriceFeeds for Pyth oracle updates, which may not be the most gas-efficient approach. The Pyth documentation suggests using updatePriceFeedsIfNecessary, which only updates price feeds when necessary, potentially saving gas costs. In scenarios where the price data is already up-to-date, this function can skip unnecessary updates, resulting in more efficient gas usage.

Replace the updatePriceFeeds call with updatePriceFeedsIfNecessary in the codebase. This will ensure that price feeds are only updated when necessary, optimizing gas consumption. The function also takes into account priceIds and publishTimes, which can be used to determine if updates are required.

Example implementation:

pyth.updatePriceFeedsIfNecessary{value: updateFee}(
    proofs,
    priceIds,
    publishTimes
);

This modification will help improve the gas efficiency of the contract while ensuring that price data is still accurately updated when needed.

ACKNOWLEDGED: Pyth is mostly only used by us in Klaytn, so we would probably be paying for most updates anyways 2.

In the getPreviousPriceAndTime function, the checkPythPriceFeedOrder is redundantly called after parsePriceFeedUpdates. According to the Pyth documentation, the parsePriceFeedUpdates function guarantees that the returned priceFeeds array will match the order of the provided priceIds, ensuring the data is returned in the correct sequence. This validation is already handled by the checkPriceDataOrder function, making the additional call to checkPythPriceFeedOrder unnecessary and inefficient.

Remove the redundant checkPythPriceFeedOrder call in getPreviousPriceAndTime. This will reduce unnecessary checks and improve the efficiency of the function. Since the parsePriceFeedUpdates function guarantees the correct order, the additional check is not needed.

ACKNOWLEDGED: Not fixed. Pyth could change that it doesn't guarantee price to come in this order.

In the calculateLiquidationPrice function, an underflow occurs when the size is less than or equal to the margin, resulting in a leverage <= 1. The expression leverage - 1 will underflow in Solidity, causing the transaction to revert. While other parts of the code enforce a minimum leverage of 3, this function does not handle the case when leverage falls below 1, leading to potential underflows and unexpected reverts. This could result in contract malfunctions during liquidation price calculations when the leverage is in the range of [0, 1).

Ensure that the calculateLiquidationPrice function explicitly handles cases where leverage is less than or equal to 1. Introduce a custom error message to catch leverage values in the invalid range and provide a clear failure reason. For example, you could add a check before performing the liquidation price calculation to avoid underflows and offer better debugging information if leverage falls below the expected threshold.

SOLVED: The leverage is checked to be between 3 and 100.

The "Margin is too small" check in the PerpDexContract ensures that the inputMarginAmount is larger than the calculated fee. While this is a safe check, it is mathematically redundant in this context. The fee is derived from inputMarginAmount using a percentage-based calculation, and for inputMarginAmount to be equal to the fee, the leverage would need to be at least 1000. However, a leverage check later ensures that the leverage is capped at 100, making it impossible for the fee to equal or exceed the margin.

This renders the margin check unnecessary because the leverage cap ensures that the fee will always be smaller than the margin amount, and the issue this check aims to prevent cannot occur.

The "Margin is too small" check can be safely removed without risk, as the leverage cap of 100 already prevents scenarios where the margin would be too small in relation to the fee. This simplifies the code and avoids redundant validations.

SOLVED: The check is removed

In the openLimitOrder function, important validation checks such as "Next position id already exists.", "Price is 0", and "Leverage must be within range" are performed after other operations such as fee calculations and signature verifications. These validations ensure the correctness of critical inputs like position ID, price, and leverage, and should ideally be checked as early as possible to prevent unnecessary operations if the input data is invalid. Delaying these checks results in wasted gas for operations that could have been aborted earlier.

Move the following validation checks to the beginning of the openLimitOrder function, before performing any other operations such as fee calculations or signature verifications: - "Next position id already exists." - "Price is 0" - "Leverage must be within range"

This will ensure that invalid inputs are caught immediately, saving gas and preventing unnecessary calculations.

SOLVED: The check was moved earlier.

In the openLimitOrder function, the error message "Limit order position should be open" is misleading. The position is not yet opened but is pending execution, waiting for the conditions to be met. This terminology can confuse users or developers when debugging or handling errors.

Change the error message to reflect the actual state of the position, which is pending execution rather than open. A more accurate message would be "Limit order position should be pending". This clarifies the intent and improves the readability of error messages.

NOT APPLICABLE: The message is in the close function.

In the _createRequestPosition function, the removeRequestedOpenPosition is not optimized for gas efficiency, as it does not leverage the openPositionIndex to efficiently remove elements from the requestedOpenPositionIds array. Instead, the function loops over the array to find the matching position, leading to higher gas costs. By utilizing openPositionIndex for direct access, the process of removing positions from the array can be optimized to a single-step removal, as already done in cleanUpLimitOrder.

Store the openPositionIndex when creating the position in _createRequestPosition and use it to directly remove the element from requestedOpenPositionIds in removeRequestedOpenPosition. This will significantly reduce gas usage by avoiding the need to iterate through the array.

Here’s how this can be implemented:

1. Set openPositionIndex in _createRequestPosition: Assign the index of the new position in the requestedOpenPositionIds array when creating the request.

2. Use openPositionIndex in removeRequestedOpenPosition: Use the stored index to efficiently remove the position from the array.

SOLVED: The code does now use a single step to open a position.

Both the _closePosition and closePosition functions perform the same check: "Position is not open", which ensures that the position is in an open state before proceeding. This results in unnecessary redundancy, as the closePosition function already verifies that the position is open before calling _closePosition. By repeating the check in both functions, the contract incurs unnecessary gas costs and reduces code clarity.

Remove the redundant "Position is not open" check from either the closePosition or _closePosition function. The check should ideally be performed in the closePosition function, as this is where the main logic flow occurs, while the _closePosition function can assume that the position is valid.

SOLVED: The double check was removed.

In the _closePosition function, there is an inconsistency in the comparison of position size to margin and profit. The check position.size < position.margin + profit should likely be position.size <= position.margin + profit, similar to the check position.margin * 5 <= profit. This inconsistency may lead to edge cases where a position that should be handled correctly under one condition fails the check, potentially causing incorrect behavior in closing positions.

The use of strict < can cause unnecessary transaction failures when the position size is exactly equal to position.margin + profit, which should be a valid scenario.

Change the position.size < position.margin + profit check to position.size <= position.margin + profit to ensure consistency with other comparisons and avoid unnecessary transaction failures.

SOLVED: The equal check is now inplace.