Submission Details
Severity:
When a keeper Cancels a withdrawal Flow Another address is refunded instead of the deposit id that called the withdrawal function
Summary
The caller of the withdraw function will lose the execution fee sent into the contract and the last depositor will get paid instead.
Vulnerability Details
When the keeper calls to cancel a flow,
And the flow is set to withdraw,
if the deposit id calling the withdraw function is not the last depositor
the caller will lose the execution fee he forwarded.
while the last depositor gets refunded for doing nothing.
/**
* @notice Withdraws collateral of user.
* @dev If position is GMX position, first call `settle` function to settle pending fees
* in order to avoid charging all fees from the withdrawer.
* @param recipient The address to receive the tokens.
* @param depositId The deposit ID to withdraw.
*/
function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
_noneFlow();
@audit>> flow = FLOW.WITHDRAW;
@audit>> flowData = depositId;
if (recipient == address(0)) {
revert Error.ZeroValue();
}
if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {
revert Error.Locked();
}
if (EnumerableSet.contains(userDeposits[msg.sender], depositId) == false) {
revert Error.InvalidUser();
}
if (depositInfo[depositId].shares == 0) {
revert Error.ZeroValue();
}
depositInfo[depositId].recipient = recipient;
_payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle(); // Settles any outstanding fees and updates state before processing withdrawal
} else {
MarketPrices memory prices;
_withdraw(depositId, hex'', prices);
}
}
Counter is the last deposit id, instead of using the flowdata we used counter
function _cancelFlow() internal {
if (flow == FLOW.DEPOSIT) {
uint256 depositId = counter;
collateralToken.safeTransfer(depositInfo[depositId].owner, depositInfo[depositId].amount); // treasury catches failure so what's up here
totalDepositAmount = totalDepositAmount - depositInfo[depositId].amount;
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner,
depositInfo[counter].executionFee
) {} catch {}
delete depositInfo[depositId];
} else if (flow == FLOW.WITHDRAW) {
try IGmxProxy(gmxProxy).refundExecutionFee(
@audit>> depositInfo[counter].owner, // don't use counter user deposit id saved inside flowdata
@audit>> depositInfo[counter].executionFee // don't use counter user deposit id saved inside flowdata
) {} catch {}
}
// Setting flow to liquidation has no meaning.
// The aim is to run FINAIZE action. (swap indexToken to collateralToken);
flow = FLOW.LIQUIDATION;
nextAction.selector = NextActionSelector.FINALIZE;
}
See in withdrawal handling
/**
* @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); // burning before refund // loss of fee to refund // high
if (refundFee) { //bug
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
// burn
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}
see keepers action also
function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
if (_isLongOneLeverage(_isLong)) {
_runSwap(metadata, true, prices);
} else {
// swap indexToken that could be generated from the last action into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(_isLong, acceptablePrice, prices);
}
@audit>> } else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
// swap indexToken that could be generated from settle action or liquidation/ADL into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
@audit>> uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);
Impact
Deposit id that made the call will lose his execution fee, while The last user will be refunded his/her fee without performing any transaction.
Tools Used
Manual Review
Recommendations
Obtain the deposit id from the flowdata and use it to process the execution fee back to the owner.
Updates
Lead Judging Commences
n0kto 9 months ago
Submission Judgement Published Assigned finding tags:
finding_counter_invalid_during_cancelFlow_after_withdrawing
Likelihood: Low, contract has to call cancelFlow after a withdraw, and the settle action is already executed by GMX. Impact: High, the fees will be distributed to the last depositor and not the withdrawer.
Prize pool breakdown
Total prize
50,000 USDC
nSLOC
1,91026
USDC / LOC
47,500
2,500
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.