Description
The problem lies in the way execution fees are handled during deposit and withdrawal operations, and how they are refunded in the _handleReturn function. Let's break down the issue:
Key Points:
Execution Fee Payment:
When a user deposits funds the _payExecutionFee function is called, and the execution fee is stored in depositInfo[depositId].executionFee.
function deposit(uint256 amount) external nonReentrant payable {
_noneFlow();
...
if (positionIsClosed) {
MarketPrices memory prices;
_mint(counter, amount, false, prices);
_finalize(hex'');
} else {
@=> _payExecutionFee(counter, true);
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}
}
function _payExecutionFee(uint256 depositId, bool isDeposit) internal {
uint256 minExecutionFee = getExecutionGasLimit(isDeposit) * tx.gasprice;
if (msg.value < minExecutionFee) {
revert Error.InsufficientAmount();
}
if (msg.value > 0) {
payable(address(gmxProxy)).transfer(msg.value);
@=> depositInfo[depositId].executionFee = msg.value;
}
}
When a user withdraws funds, the _payExecutionFee function is called again, and the execution fee is overwritten in depositInfo[depositId].executionFee.
function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
_noneFlow();
flow = FLOW.WITHDRAW;
flowData = depositId;
...
depositInfo[depositId].recipient = recipient;
@=> _payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle();
} else {
MarketPrices memory prices;
_withdraw(depositId, hex'', prices);
}
}
function _payExecutionFee(uint256 depositId, bool isDeposit) internal {
uint256 minExecutionFee = getExecutionGasLimit(isDeposit) * tx.gasprice;
if (msg.value < minExecutionFee) {
revert Error.InsufficientAmount();
}
if (msg.value > 0) {
payable(address(gmxProxy)).transfer(msg.value);
@=> depositInfo[depositId].executionFee = msg.value;
}
}
Refund Logic:
In the _handleReturn function, when refundFee is true, the contract attempts to refund the execution fee. However it only refunds the fee stored in depositInfo[depositId].executionFee, which is the fee from the most recent operation (either deposit or withdrawal).
function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
...
@=> if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
@=> if (depositInfo[depositId].executionFee > usedFee) {
@=> try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
delete swapProgressData;
delete flowData;
delete flow;
}
This means that if a user pays a fee for both deposit and withdrawal only the withdrawal fee is refunded and the deposit fee is effectively lost.
Impact
The user pays two execution fees (one for deposit and one for withdrawal), but only one fee is refunded. This is unfair to the user as they should receive a refund for both fees.
Recommendation
It should add both fees of deposit and withdraw in _payExecutionFee.
function _payExecutionFee(uint256 depositId, bool isDeposit) internal {
uint256 minExecutionFee = getExecutionGasLimit(isDeposit) * tx.gasprice;
if (msg.value < minExecutionFee) {
revert Error.InsufficientAmount();
}
if (msg.value > 0) {
payable(address(gmxProxy)).transfer(msg.value);
- depositInfo[depositId].executionFee = msg.value;
+ depositInfo[depositId].executionFee += msg.value;
}
}
