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

Execution Fee Loss Due to Premature Data Deletion

parth

Summary

User Funds Lost Due to Execution Fee Refund Failure

Vulnerability Details

During the execution of the PerpetualVault::_handleReturn() function, the _burn() function is called, which burns the user's shares and deletes the
depositInfo mapping that tracks user deposits. After burning the shares, the function initiates the refund fee process, which checks whether
depositInfo[depositId].executionFee > usedFee to determine if the execution fee is greater than the used fee. However, this condition will never be executed,
even if the execution fee is higher than the used fee because the depositInfo mapping—containing this crucial data like the execution fee that the user gave—has already been deleted. As a result, users may suffer fund losses due to unprocessed refunds.

​
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) { //checking for excess which will fail eventually
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
​
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}
​
​
function _burn(uint256 depositId) internal {
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
totalShares = totalShares - depositInfo[depositId].shares;
@> delete depositInfo[depositId]; //deleting the mapping of depositId
}
​

Impact

  • The user will not be able to get the excess fee back at the time of withdrawal.

  • Funds intended to be refunded will be stuck within the contract.

Tools Used

  • Manual Review

Recommendations

  • Reorder the Sequence of the function call in the _handleReturn() function.

  • Create a temporary variable to store the execution fee before deleting the mapping.

Updates

Lead Judging Commences

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