Liquidity Management

Summary

This report presents the findings of a security audit conducted on the smart contracts for the CodeHawks Cyfrin website, specifically the PerpetualVault project found in the provided GitHub repository. The audit focused on identifying potential vulnerabilities within the contracts, considering the specified scopes, compatibilities, and functionalities. The audit involved manual code review to assess the security posture of the codebase. Several potential vulnerabilities and areas for improvement were identified, ranging from informational issues to potential high-severity risks. The most critical finding is the lack of access control on critical functions in PerpetualVault.sol, which could allow unauthorized manipulation of the vault. Other identified issues include potential reentrancy risks, DoS vulnerabilities, operational risks related to keeper management, input validation weaknesses, and error handling concerns in external interactions. This report details these findings, their potential impact, and recommendations for remediation, along with code fixes for the identified vulnerabilities.

Vulnerability Details

1. Missing Access Control on Critical Functions in PerpetualVault.sol

Severity: High

Description:
Many crucial functions in PerpetualVault.sol, such as deposit(), withdraw(), requestOpenPosition(), requestClosePosition(), and administrative functions like updateKeeper(), updateGmxProxy(), setLiquidator(), and parameter setting functions, lack proper access control modifiers (e.g., onlyOwner). This means anyone could potentially call these functions if access control is not implemented through an external mechanism.

Impact:
An attacker could:

• Steal deposited funds by calling withdraw().

• Manipulate trading positions.

• Change critical system parameters, disrupting the vault or draining funds.

• Set a malicious liquidator.

Tools Used:
Manual Code Review

Recommendations:
Implement robust access control using the Ownable pattern or a similar mechanism. Restrict administrative functions and fund management functions to the contract owner or a designated admin role. Apply onlyOwner or onlyAdmin modifiers to all critical functions in PerpetualVault.sol that manage funds, system parameters, or contract configurations.

Code Fix:

This diff adds Ownable inheritance to PerpetualVault and adds the onlyOwner() modifier to all administrative and parameter setting functions, securing these critical functions to be callable only by the contract owner. It also adds a constructor to initialize Ownable.

2. Potential Reentrancy Risk in withdraw() function

Severity: Medium

Description:

The withdraw() function uses _transferWeth to send WETH. If _transferWeth is a standard transfer or call.value(), it could be vulnerable to reentrancy if the recipient is a malicious contract. While WETH itself mitigates some reentrancy risks, it's best practice to implement reentrancy protection, especially when dealing with user-controlled addresses.

Impact:

A malicious contract could potentially re-enter the withdraw() function during the token transfer and attempt to withdraw funds multiple times, leading to unauthorized fund drainage.

Tools Used:

Manual Code Review

Recommendations:

Implement reentrancy protection in the withdraw() function using the Checks-Effects-Interactions pattern or a reentrancy guard library (e.g., OpenZeppelin's ReentrancyGuard). Thoroughly examine the implementation of _transferWeth and ensure it's not vulnerable.

Code Fix:

This diff adds ReentrancyGuard inheritance to PerpetualVault and applies the nonReentrant1 modifier to the internal _withdraw function. This provides reentrancy protection for the withdrawal process. It's assumed _withdraw is the core withdrawal logic.2 If the external withdraw function was intended to be protected directly, then nonReentrant should be applied to the external withdraw function instead.

3. Unbounded orderQueue in PerpetualVault.sol (Potential DoS/Gas Griefing)

Severity: Medium

Description:

The orderQueue array in PerpetualVault.sol stores pending orders. If there is no limit on the queue size or mechanism to process and clear it regularly, it could grow indefinitely.

Impact:

An unbounded orderQueue could lead to:

• Denial of Service (DoS) due to excessive gas consumption when processing the queue.

• Gas Griefing by attackers filling the queue with spam orders, increasing gas costs for legitimate users.

Tools Used:

Manual Code Review

Recommendations:

Implement a mechanism to manage the orderQueue size. Options include:

• Limiting the maximum queue size.

• Implementing order expiration or cancellation for old orders.

• Regularly processing and clearing the queue in batches or based on time.

• Consider alternative data structures if unbounded growth is a critical concern.

Code Fix:

This diff adds a maxOrderQueueSize state variable, initialized to 100 as an example. It also adds a check in _requestOpenPosition to revert if the queue is full. A setMaxOrderQueueSize function with onlyOwner modifier is added to allow the owner to configure this limit. A basic clearOrderQueue() function is also added for admin queue management, but more sophisticated queue management strategies are mentioned in the comments as better alternatives for production.

4. Reliance on keeperGasLimit parameter (Potential Operational Risk)

Severity: Medium

Description:

The contract relies on a keeperGasLimit parameter. If this limit is set incorrectly (too low or too high), it can lead to operational issues and increased gas costs.

Impact:

• Incorrect keeperGasLimit can cause keepers to fail executing critical tasks (order execution, liquidations), leading to system instability and missed opportunities.

• Unnecessarily high gas limits increase operational costs.

Tools Used:

Manual Code Review

Recommendations:

• Implement robust monitoring and alerting for keeper operations and gas consumption.

• Consider making keeperGasLimit dynamically adjustable based on network conditions or keeper performance.

• Provide clear guidelines and documentation on setting the appropriate keeperGasLimit.

Code Fix:

While dynamic adjustment of keeperGasLimit is complex, we can at least provide an onlyOwner function to set it, which was already done in the access control fix. No further code diff is strictly needed for this code fix section, as the recommendation is mostly about operational best practices. The access control fix for setKeeperGasLimit function already addresses part of the recommendation. Let's add a comment to the setKeeperGasLimit function emphasizing monitoring.

Diff

This adds a comment to the setKeeperGasLimit function reminding developers about the importance of monitoring and potentially dynamically adjusting this parameter, emphasizing the operational aspect of this vulnerability.

5. Lack of Input Validation on Order Parameters

Severity: Medium

Description:

The requestOpenPosition() and requestClosePosition() functions take parameters like sizeDeltaUsd, leverage, and slippageBps without sufficient input validation.

Impact:

Lack of input validation could lead to:

• Logic errors and unexpected behavior from invalid input values.

• Potential exploitation by attackers providing extreme or malicious values.

Tools Used:

Manual Code Review

Recommendations:

Implement thorough input validation in requestOpenPosition() and requestClosePosition(). Validate:

• Reasonable ranges for parameters (leverage, slippage).

• Positive values where required (sizeDeltaUsd).

• Data types.

• Use require statements to enforce validation and revert transactions with informative error messages for invalid inputs.

Code Fix:

This diff adds require statements at the beginning of requestOpenPosition and requestClosePosition functions to perform input validation. It checks for:

• _order.sizeDeltaUsd being positive.

• _order.leverage being within allowed range (positive and not exceeding maxLeverage).

• _order.slippageBps not exceeding the contract's slippageBps.

• _order.market being a valid Market (not NONE).

• _order.positionId being positive for close position.

More validations can be added as needed, depending on the specific requirements and potential edge cases of the order parameters.

6. Missing Error Handling in GMX Interactions in GmxProxy.sol

Severity: Medium

Description:

GmxProxy.sol interacts with external GMX contracts. If these external calls fail, the GmxProxy might not handle errors gracefully.

Impact:

Unhandled errors in GMX interactions can cause:

• Failed order requests without proper notification.

• Stuck funds or positions if GMX operations fail mid-execution.

• Unexpected vault behavior in response to GMX call failures.

Tools Used:

Manual Code Review

Recommendations:

Implement robust error handling in GmxProxy.sol for all GMX interactions.

• Check return values of external calls.

• Use try-catch blocks to handle exceptions.

• Implement fallback logic or revert transactions gracefully on GMX call failures.

• Log error events for monitoring and debugging.

Code Fix:

This diff implements try-catch blocks around all calls to external GMX contracts (gmxVault.increasePosition, gmxVault.decreasePosition, gmxRouter.executeOrder, gmxRouter.cancelOrder) in GmxProxy.sol. If a GMX call fails (throws an exception), the catch block will:

1. Emit an event (e.g., GmxIncreasePositionFailed) logging the orderHash and the reason for the failure (the error data returned by GMX).

2. Revert the current transaction using revert GmxOperationFailed(reason). This ensures that if a GMX operation fails, the overall operation in the vault is also reverted, maintaining consistency.

Error events are added to GmxProxy.sol for logging purposes, which is crucial for debugging and monitoring GMX interactions. A custom error GmxOperationFailed (which would need to be defined in libraries/Errors.sol if it doesn't exist already) is used for reverting.

7. Lack of Rate Limiting/DoS Protection for Keeper Functions in KeeperProxy.sol

Severity: Low-Medium

Description:

KeeperProxy.sol allows keepers to call functions like processOrderQueue() and liquidatePosition(). Lack of rate limiting could allow spamming these functions.

Impact:

Excessive calls to keeper functions can lead to:

• Gas Griefing, increasing gas costs.

• Denial of Service (DoS), overloading the contract and hindering legitimate operations.

Tools Used:

Manual Code Review

Recommendations:

Implement rate limiting or DoS protection for keeper functions in KeeperProxy.sol. Consider:

• Throttling keeper calls based on time intervals.

• Request queuing or prioritization.

• Monitoring and potentially penalizing abusive keepers.

Code Fix:

Implementing a full rate limiting mechanism within this response would be more complex and depend heavily on the desired rate limiting strategy. For a basic conceptual code fix, we can add a simple example of time-based throttling. However, this is a simplified illustration. More robust rate limiting solutions might involve using mapping to track keeper activity, timestamps, or counters, and potentially using off-chain components for more advanced rate limiting.

For demonstration, let's add a simple last-call timestamp check to processOrderQueue() in KeeperProxy.sol. This is a very basic example and is NOT production-ready rate limiting.

This very basic diff adds a lastProcessOrderQueueCall timestamp variable and a check at the beginning of processOrderQueue(). It rejects calls if they are made too soon after the last successful call (in this example, within 30 seconds). This is a simplistic example and is not robust rate limiting. A production system would likely need a more sophisticated approach, potentially tracking per-keeper activity, using counters, or more advanced rate limiting techniques. The recommendation in the audit report outlines more robust approaches. The custom error RateLimited would need to be defined in libraries/Errors.sol