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

Users will never receive their execution fees during token withdrawal.

0xakira

Vulnerability details

When a user wants to withdraw his tokens from the protocol while swapping a collateralToken for an indexToken, the _handleReturn function will be called with input true in the refundFee argument. This means that the user will get back the execution fee under certain conditions. But before the refundExecutionFee function is called, the _burn() function is called, which in its turn completely removes mapping by the given id.

function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
//code
_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 {}
}
}
// code
function _burn(uint256 depositId) internal {
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
totalShares = totalShares - depositInfo[depositId].shares;
delete depositInfo[depositId];
}

Impact

Since after burning and deleting the deposit there is a check and passing to the arguments of the function from the mapping on the already deleted id, then zeros are passed and thus the user does not receive his execution fee.

Recommended Mitigation Steps

you should make local variables in this function and use them after the _burn function.

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);
+ uint256 executionFee = depositInfo[depositId].executionFee;
+address depositOwner = depositInfo[depositId].owner;
_burn(depositId);
​
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
- if (depositInfo[depositId].executionFee > usedFee) {
+ if (executionFee > usedFee)
- try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
+ try IGmxProxy(gmxProxy).refundExecutionFee(depositOwner, executionFee - usedFee) {} catch {}
}
}
​
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}
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!