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

depositInfo Deleted Before executionFee Is Read From It ( Hence Always Would Be 0 )

sakshamseth5

Summary

When refunding a user in _handleReturn we perform burn() first (which deleted the depositInfo[] of the user) and then do the refund which reads the depositInfo[depositId].executionFee , but since depositInfo[] has been deleted before , depositInfo[depositId].executionFee would be read as 0 hence no refunds.

Vulnerability Details

Consider the following ->

1.) There's a 1x Long perp vault position open on GMX (positionIsClosed is false).

2.) A user wants to withdraw hence calls withdraw() at L253 , and pays the execution fee at L272 _payExecutionFee(depositId, false);

, and _withdraw() is invoked .

3.) Since position is 1x long , inside _withdraw() this branch would be executed (L1097 - L1102)->

} else if (_isLongOneLeverage(beenLong)) { // beenLong && leverage == BASIS_POINTS_DIVISOR
uint256 swapAmount = IERC20(indexToken).balanceOf(address(this)) * shares / totalShares;
nextAction.selector = NextActionSelector.SWAP_ACTION;
// abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
nextAction.data = abi.encode(swapAmount, false);
}

4.) Since next action is now SWAP_ACTION , when keeper runs runNextAction() , this branch would be called (L382 - L387) ->

else if (_nextAction.selector == NextActionSelector.SWAP_ACTION) {
(, bool isCollateralToIndex) = abi.decode(
_nextAction.data,
(uint256, bool)
);
_runSwap(metadata, isCollateralToIndex, prices);

5.) Therefore , _runSwap is called and assuming metada.length == 1 (passed in _runNextAction) this branch of runSwap would be invoked ->

else {
if (metadata.length != 1) {
revert Error.InvalidData();
}
(PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
if (_protocol == PROTOCOL.DEX) {
uint256 outputAmount = _doDexSwap(data, isCollateralToIndex);
// update global state
if (flow == FLOW.DEPOSIT) {
// last `depositId` equals with `counter` because another deposit is not allowed before previous deposit is completely processed
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
} else if (flow == FLOW.WITHDRAW) {
_handleReturn(outputAmount + swapProgressData.swapped, false, true);
} else {
// in the flow of SIGNAL_CHANGE, if `isCollateralToIndex` is true, it is opening position, or closing position
_updateState(!isCollateralToIndex, isCollateralToIndex);
}

6.) We perform the swap through _doDexSwap and then since flow is WITHDRAW ->

else if (flow == FLOW.WITHDRAW) {
_handleReturn(outputAmount + swapProgressData.swapped, false, true);

7.) Therefore , _handleReturn is called with refundFee as true (because we wanna refund since no interaction with GMX took place) ->

[https://github.com/CodeHawks-Contests/2025-02-gamma/blob/main/contracts/PerpetualVault.sol#L1129]

and then amount is calculated to be transferred and then transferred via _transferToken

8.) Then we burn the depositId (L1143) and inside burn we delete the depositInfo of the user ->

function _burn(uint256 depositId) internal {
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
totalShares = totalShares - depositInfo[depositId].shares;
delete depositInfo[depositId];
}

9.) Then in _handleReturn since refundFee was true we do ->

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

10.) But since depositInfo is deleted in burn , depositInfo[depositId].executionFee would always be 0 and therefore no refund would take place in the above refund fee branch (this refund is for excess fee or since this swap was completed by the dex and not on gmx side).

Impact

The above refund would never take place since burn is performed before refund .

Tools Used

Manual analysis

Recommendations

Call burn after refunding the fee.

Updates

Lead Judging Commences

n0kto Lead Judge 5 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.