Summary

The withdraw function in the PerpetualVault.sol does not allow users to specify an amount to withdraw. Instead, it appears to withdraw the entire amount associated with the depositId. This design limits user flexibility, as they cannot withdraw partial amounts and must withdraw the full deposit in a single transaction. This issue impacts the usability and efficiency of the contract.

Vulnerability Details

The withdraw function is designed to process withdrawals based on a depositId but does not include an amount parameter. This means users cannot specify how much they want to withdraw and are forced to withdraw the entire deposit. The function relies on the depositInfo[depositId].shares value to determine the withdrawal amount, which suggests it is designed for full withdrawals only.

Affected Code:

function withdraw(address recipient, uint256 depositId) public payable nonReentrant {

// ... (checks and validations)

if (depositInfo[depositId].shares == 0) {

revert Error.ZeroValue();

}

// ... (logic for withdrawal)

}

Root Cause:

• The function lacks an amount parameter, which is a standard feature in withdrawal functions to allow partial withdrawals.

• The contract assumes that users will always want to withdraw the entire deposit associated with the depositId.

Impact

1. Poor User Experience: Users cannot withdraw partial amounts, which may not align with their needs or expectations.

2. Gas Inefficiency: Users who want to withdraw smaller amounts multiple times must create multiple deposits, increasing gas costs.

3. Reduced Functionality: The lack of an amount parameter restricts the contract's usability in scenarios where partial withdrawals are necessary (e.g., gradual liquidation of funds).

Tools Used

Manuel Review

Recommendations

To address this issue, the withdraw function should be modified to include an amount parameter and handle partial withdrawals. Here is an example of how the function could be updated:

function withdraw(address recipient, uint256 depositId, uint256 amount) public payable nonReentrant {

_noneFlow();

flow = FLOW.WITHDRAW;

flowData = depositId;

​

if (recipient == address(0)) {

revert Error.ZeroValue();

}

if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {

revert Error.Locked();

}

if (EnumerableSet.contains(userDeposits[msg.sender], depositId) == false) {

revert Error.InvalidUser();

}

if (depositInfo[depositId].shares == 0 || amount == 0 || amount > depositInfo[depositId].shares) {

revert Error.InvalidAmount();

}

​

depositInfo[depositId].recipient = recipient;

_payExecutionFee(depositId, false);

​

if (curPositionKey != bytes32(0)) {

nextAction.selector = NextActionSelector.WITHDRAW_ACTION;

_settle();

} else {

MarketPrices memory prices;

_withdraw(depositId, amount, prices);

}

}

Key Changes:

1. Added an amount parameter to the function signature.

2. Added validation to ensure the amount is greater than zero and does not exceed the user's shares.

3. Updated the _withdraw function call to include the amount parameter.