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

Refund Fee Loss Due to Deposit Info Deletion

secret__one

Summary

In the _handleReturn function, the contract deletes the depositInfo entry for a given depositId during the _burn operation. However, the refund logic, executed after the burn, still tries to access depositInfo, leading to silent refund loss.

Vulnerability Details

Inside the PerpetualVault.sol,

Affected Function: _handleReturn

/**
* @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);
​
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;
}

In the _handleReturn function:

  1. Burn operation deletes deposit info:

    /**
    * burn shares corresponding to `depositId`
    * @param depositId `depositId` to burn
    */
    function _burn(uint256 depositId) internal {
    EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
    totalShares = totalShares - depositInfo[depositId].shares;
    delete depositInfo[depositId]; // Data is deleted here
    }
    ​

  2. Refund logic accesses deleted data:

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

After _burn, the protocol tries to access depositInfo[depositId] for fee refunds. But since the data is deleted executionFee becomes 0. And, usedFee will always greater than zero, so refundFee will never transfered to the user.

Impact

Users permanently lose refundable execution fees.

Tools Used

Manual Review

Recommendations

Move the refund fee logic before calling _burn(depositId).

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_burn_depositId_before_refund

Likelihood: High, every time a user withdraw on 1x vault with paraswap Impact: Medium, fees never claimed to GMX and refund to the owner.

Support

FAQs

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

Give us feedback!