Summary
Whenever a User call to withdraw his token his desposit id is saved but this id is not used during execution fee refund calling this user to lose the msg.value passed in.
Vulnerability Details
There is a bug in the burn function but the fix there doesn't solve this issue as it still presists.
In a 1x Contract for example Alice calls to withdraw 1000 USD with 16 USD execution fee
We call the _withdraw function or we set and wait for keeper to call
After funds are processed the call back calls to finalize all actions
For a 1x leverage used as an example we set a SWAP action see flow below
*/
function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
uint256 shares = depositInfo[depositId].shares;
if (shares == 0) {
revert Error.ZeroValue();
}
if (positionIsClosed) {
_handleReturn(0, true, false);
} else if (_isLongOneLeverage(beenLong)) {
uint256 swapAmount = IERC20(indexToken).balanceOf(address(this)) * shares / totalShares;
@audit>> nextAction.selector = NextActionSelector.SWAP_ACTION;
@audit>> nextAction.data = abi.encode(swapAmount, false);
}
5 . SWAP action
} else if (_nextAction.selector == NextActionSelector.SWAP_ACTION) {
(, bool isCollateralToIndex) = abi.decode(
_nextAction.data,
(uint256, bool)
);
_runSwap(metadata, isCollateralToIndex, prices);
6 . runswap calls to line 1007 after swaping
} 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);
if (flow == FLOW.DEPOSIT) {
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
@audit>> } else if (flow == FLOW.WITHDRAW) {
@audit>> _handleReturn(outputAmount + swapProgressData.swapped, false, true);
} else {
7 . This then tries to refund Alice his 16 USD since their was no call to Gmx
* @notice this function is an end of withdrawal flow.
* @dev should update all necessary global state variables
*
* @param withdrawn amount of token withdrawn from the position
* @param positionClosed true when position is closed completely by withdrawing all funds, or false
*/
function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
@audit>>> (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);
@audit>>> if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
@audit>>> if (depositInfo[depositId].executionFee > usedFee) {
@audit>>> try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner,
@audit>>> depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
8 . From using deposit id we suddenly change to counter which is the last depositor not the present User.
Bob is the last user he will get a refund.
Alice id is 1 not counter although get the 1000 USD, alice will lose her refund 16 USD that was not used.
Impact
Loss of the refunded fee sent into the contract for the user calling withdrawal.
Tools Used
Manual Review
Recommendations
Use the deposit id and not counter to process refund.
