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

The `refundFee` can be stolen via front-running of withdrawals

sparrowblck

[M-01] Summary

The refundFee can be returned to the wrong address (stolen) via front-running of withdrawals

Vulnerability Details

In PerpetualVault::_handleReturn(), there is an optional returning of the refund fee to the depositor, in case the actual executionFee surpasses the usedFee (calculated based on the gas usage in the transaction). This is done by sending native tokens to the deposit's owner via the GmxProxy contract. The returning of fees is executed chiefly as part of the withdrawal flow.

The issue is that the owner of the deposit is determined with depositInfo[counter] instead of depositInfo[depositId], which always returns the owner of the latest deposit. This leaves the door for a front-running attack, in which the malicious actor could front-run a legitimate withdrawal request by depositing themselves and receiving the refund fee from the real owner of the original deposit.

  1. User A deposits 1000 USDC and sends 0.001 ETH as msg.value, in order to cover the executionFee.

  2. The remaining (unused) refund fee is 0.0005 ETH.

  3. User A submits a withdrawal transaction request (transaction is still in the memory pool)

  4. User B front-runs them by submitting a deposit request for 1000 USDC and a higher gas price.

  5. counter is updated to User B's deposit.

  6. User A's withdrawal is finally processed, at which point the 0.0005 ETH is sent to User B's address.

Impact

High - funds could be stolen from users relatively easily. Likelihood - lower, as the refunded amounts are likely not to be too large to attract huge interest. Also, the attacked would need to invest in the Vault for the lockTime period, before being able to withdraw their funds themselves.

Tools Used

Manual review.

Recommendations

Return the refund fee to the owner of the deposit with depositId instead of counter:

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 {}
+ try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[depositId].owner, depositInfo[depositId].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_counter_invalid_during_handleReturn

Likelihood: Medium/High, when withdraw on a 1x vault. Impact: High, the fees will be distributed to the last depositor and not the withdrawer.

Support

FAQs

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

Give us feedback!