Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

The withdraw function does not refund execute fees when the position is closed (positionIsClosed == true)

biakia

Summary

The withdraw function in the PerpetualVault contract allows users to withdraw their deposited collateral tokens. When users call this function, they may need to pay an execution fee (executionFee) by sending msg.value. This fee is stored in depositInfo[depositId].executionFee. However, if the position is closed (positionIsClosed == true), the withdraw function calls _handleReturn with refundFee = false, which means the execution fee is not refunded to the user. This results in the user losing the execution fee they paid, even though the withdrawal operation may not have incurred significant gas costs.

Vulnerability Details

Users call withdraw to withdraw their deposited collateral tokens and it will call _payExecutionFee :

function withdraw(address recipient, uint256 depositId) 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) {
revert Error.ZeroValue();
}
​
depositInfo[depositId].recipient = recipient;
_payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle(); // Settles any outstanding fees and updates state before processing withdrawal
} else {
MarketPrices memory prices;
_withdraw(depositId, hex'', prices);
}
}

In _payExecutionFee function, if msg.value > 0, the execution fee is paid and stored in depositInfo[depositId].executionFee

function _payExecutionFee(uint256 depositId, bool isDeposit) internal {
uint256 minExecutionFee = getExecutionGasLimit(isDeposit) * tx.gasprice;
​
if (msg.value < minExecutionFee) {
revert Error.InsufficientAmount();
}
if (msg.value > 0) {
payable(address(gmxProxy)).transfer(msg.value);
depositInfo[depositId].executionFee = msg.value;
}
}

The the withdraw function will call _withdraw function if there is no open position and in _withdraw function, it will call _handleReturn with refundFee = false

function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
uint256 shares = depositInfo[depositId].shares;
if (shares == 0) {
revert Error.ZeroValue();
}
if (positionIsClosed) {
_handleReturn(0, true, false);
}
...
...

If refundFee is false, the execution fee stored in depositInfo[depositId].executionFee is not refunded to the user:

function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
...
...
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}

Issue

  • When the position is closed (positionIsClosed == true), the withdraw function calls _handleReturn with refundFee = false. This means the execution fee paid by the user is not refunded, even though the withdrawal operation may not have incurred significant gas costs.

  • This behavior is unfair to users because they lose the execution fee they paid, even if the withdrawal operation is simple and does not require complex on-chain interactions (e.g., GMX position adjustments).

Impact

  • Financial Loss for Users: Users lose the execution fee they paid when withdrawing funds, especially in cases where the position is already closed and the withdrawal operation is straightforward.

The impact is High, the likelihood is Low, so the severity is Medium.

Tools Used

Manual Review

Recommendations

Consider following fix:

function withdraw(address recipient, uint256 depositId) 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) {
revert Error.ZeroValue();
}
if( curPositionKey == bytes32(0) && positionIsClosed == true){
require(msg.value == 0,"no need to pay execute fee");
}
...
...
Updates

Lead Judging Commences

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_withdraw_positionIsClosed_does_not_refund_fees

No fee needed in _payExecutionFee when position is closed. Make a PoC if you disagree.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.