Submission Details
Severity:
Users will not receive their Execution fee refund because of a flaw in implementation, burning before we call refund
Summary
The code burns the deposit id before trying to refund users their excess execution fee leading to a loss of funds as user will not get back these fees.
Vulnerability Details
We can burn before we attempt to transfer the remaining fee, effectively setting the the execution fee to 0, meaning that there is nothing to transfer
/**
* @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 {
(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);
@audit>> _burn(depositId); // burning before refund // loss of fee to refund // high
@audit>> if (refundFee) { //bug
@audit>> uint256 usedFee = callbackGasLimit * tx.gasprice;
@audit>> if (depositInfo[depositId].executionFee > usedFee) {
@audit>> try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
@audit>> }
}
// burn after
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}
Burn removes and deletes all deposit info
/**
* burn shares corresponding to `depositId`
* @param depositId `depositId` to burn
*/
function _burn(uint256 depositId) internal {
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
totalShares = totalShares - depositInfo[depositId].shares;
@audit>> delete depositInfo[depositId];
}
struct DepositInfo {
uint256 amount; // amount of deposit
uint256 shares; // amount of share corresponding to deposit amount
address owner; // depositor address
uint256 executionFee; // execution fee
uint256 timestamp; // timestamp of deposit
address recipient; // address of recipient when withdrawing, which is set at the moment of withdrawal request
}
deleting resets all unit to the default 0 amount, but refund is required
/**
* @dev Swap data is an array of swap data and is passed from an off-chain script.
* It could contain only Paraswap data, GMX swap data, or both of them.
* If both are included, the first element of the array should always be Paraswap.
* @param metadata Bytes array that includes swap data.
* @param isCollateralToIndex Direction of swap. If true, swap `collateralToken` to `indexToken`.
* @return completed If true, the swap action is completed; if false, the swap action will continue.
*/
function _runSwap(bytes[] memory metadata, bool isCollateralToIndex, MarketPrices memory prices) internal returns (bool completed) {
if (metadata.length == 0) {
revert Error.InvalidData();
}
if (metadata.length == 2) {
(PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
if (_protocol != PROTOCOL.DEX) {
revert Error.InvalidData();
}
swapProgressData.swapped = swapProgressData.swapped + _doDexSwap(data, isCollateralToIndex);
(_protocol, data) = abi.decode(metadata[1], (PROTOCOL, bytes));
if (_protocol != PROTOCOL.GMX) {
revert Error.InvalidData();
}
_doGmxSwap(data, isCollateralToIndex);
return false;
} 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);
// update global state
if (flow == FLOW.DEPOSIT) {
// last `depositId` equals with `counter` because another deposit is not allowed before previous deposit is completely processed
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
} else if (flow == FLOW.WITHDRAW) {
@audit>> _handleReturn(outputAmount + swapProgressData.swapped, false, true); //bug
} else {
// in the flow of SIGNAL_CHANGE, if `isCollateralToIndex` is true, it is opening position, or closing position
_updateState(!isCollateralToIndex, isCollateralToIndex);
}
Impact
Fees are not refunded back to the owner.
Tools Used
Manual Review
Recommendations
Burn deposit id after attempting to refund the deposit owner
Updates
Lead Judging Commences
n0kto 5 months ago
Submission Judgement Published 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.
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.