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

User pays executionFee and is never refunded when they should be during withdrawal

fresh

Summary

In PerpetualVault::withdraw-> the user is charged an exectuionFee, which is then sent to gmxProxy.

This fee is used when creating orders on gmx. But throughout the protocol, is refunded when the user action in certain states do not require any order creation or interaction with the gmx.

But, this is not followed in withdraw-> and the user is charged executionFeeand never refunded, even though gmxis never used. They should be refunded.

Vulnerability Details

Lets go through the code path:

** User calls withdrawand is charged executionFee. This fee is charged regardless of the following steps, which are based on the state and current position.

** _payExecutionFeesends the ETH amount sent in as msg.valueto gmxProxy

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;
​
/*///////////////////////////////////////*/
// sends msg.value ETH amount to gmx proxy
-> _payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle(); // Settles any outstanding fees and updates state before processing withdrawal
​
/*///////////////////////////////////////////*/
// this scenario, runs this code
-> } else {
MarketPrices memory prices;
_withdraw(depositId, hex'', prices);
}
}

** If the position is closed, the curPositionKeywill be 0 -> which the logic has specific instructions for handling that scenario. It directly calls _withdraw

/**
* @notice Handles the internal withdrawal process for a given deposit ID.
* @dev This function calculates the share of the withdrawal, handles the swap if necessary,
* and updates the position accordingly.
* @param depositId The ID of the deposit to withdraw.
*/
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);
-> } else if (curPositionKey == bytes32(0)) { // vault liquidated
_handleReturn(0, true, false);

** The logic does the same operation for position being closed and curPositionKey being 0. The next step of the code path is calling _handleReturn.

** It is IMPORTANT to note that in both scenarios, the input passed to _handleReturnis 0, true, false

** the falsesignifies that refundFeeis false-> and user wont get a refund

/**
-> * @notice this function is an end of withdrawal flow.
* @dev should update all necessary global state variables
*
* @param withdrawn amount of token withdrawn from the position
* @param positionClosed true when position is closed completely by withdrawing all funds, or false
*/
​
-> function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
uint256 shares = depositInfo[depositId].shares;
uint256 amount;
if (positionClosed) {
amount = collateralToken.balanceOf(address(this)) * shares / totalShares;
} else {
uint256 balanceBeforeWithdrawal = collateralToken.balanceOf(address(this)) - withdrawn;
amount = withdrawn + balanceBeforeWithdrawal * shares / totalShares;
}
if (amount > 0) {
_transferToken(depositId, amount);
}
emit Burned(depositId, depositInfo[depositId].recipient, depositInfo[depositId].shares, amount);
_burn(depositId);
​
/*////////////////////////////////////////////////////////*/
// THIS NEVER GETS EXECUTED BECAUSE refundFee IS FALSE !!!!
-> if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
​
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}

** that is the end of the code path and the user successfully withdraws -> BUT they are charged an executionFeethat they should never need to pay for this scenario.

** There is specific logic to refund the fee to the user when gmxis never used, and this is followed in the rest of the codebase, just not here.

Impact

The user is charged an executionFee-> WHICH IS DOUBLE the amount of an increase order

It is double because for withdraws, the protocol charges execution fee for 2 interactions with GMX. THis is followed for all withdraws, so the user is charged DOUBLE the execution fee.

Tools Used

Manual Review

Recommendations

in withdraw-> when calling handleReturn- do not pass in falsefor refundFee

Instead, pass in truefor refundFeewhen calling _handleReturn

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.