Summary
The function PerpetualVault:_handleReturn:L1129-1156 is the end of the withdrawal flow, as stated in the NatSpec for this function. It transfers the deposited collateralTokens back to the depositor via the _transferToken function and then burns the depositor’s shares by calling the _burn function. The _handleReturn function’s third parameter, bool refundFee, is used to determine if the user should be refunded excess (unused) execution fee for the withdrawal.
If we take a look at what the _burn function does, we can see that it removes the deposit from the EnumerableSet, subtracts the shares from totalShares, and then deletes the struct at the depositId index in the depositInfo mapping.
function _burn(uint256 depositId) internal {
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
totalShares = totalShares - depositInfo[depositId].shares;
@> delete depositInfo[depositId];
}
In the _handleReturn function, we can see the _burn function is called before we check if we should refund the fees. If the refundFee parameter is true and excess fees should be refunded, the next if statement gets the executionFee from the now-deleted DepositInfo struct at the depositId index in the depositInfo mapping.
The deletion will result in depositInfo[depositId].executionFee always being 0 at this point. This means this if statement will always evaluate to false, and the withdrawer will never get refunded the unused executionFee.
@> _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 {}
}
}
Vulnerability Details
As the _handleReturn function is internal, we can see that the only case where this parameter is set to true directly is inside the PerpetualVault:_runSwap:L976-1019. This means it will occur if a swap is done using only a DEX swap (ParaSwap) and the flow is a withdrawal. As the _runSwap calls are always a result of either _run or _runNextAction calls by the keeper, they decide if a withdrawal in this case should be done via DEX and GMX or just DEX. The executionFee is paid by the user withdrawing upon starting the withdrawal flow, because they cannot know if GMX will be used or not at that point. The refundFee can also be set to true by encoding it in the state variable nextAction.data, but this does not appear to be currently done.
There is also an additional problem with the refunding of fees in the _handleReturn function. But this code will not be executed due to the if (depositInfo[depositId].executionFee > usedFee) always evaluating to false, as explained in the summary. So it is not currently a vulnerability, but it will be if this issue is fixed. We can see that the counter state variable is used to index the depositInfo mapping, and not the depositId variable, which is the correct index for the withdrawal being handled. This will result in the last depositor, whose depositInfo data is stored at the index counter, having their executionFee refunded instead. So that needs to be fixed if this issue is corrected, which will result in that now-unreachable code executing.
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
@> try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
Poc
Add the following in the test/mock/MockData.sol file:
function getParaSwapDataReverse(
address receiver
) external pure returns (bytes memory) {
bytes memory rev = abi.encodePacked(receiver);
bytes
memory original = hex"000000000000000000000000def171fe48cf0115b1d80b88dc8eab59176fee5700000000000000000000000000000000000000000000000000000002540be400000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000007c446c67b6d000000000000000000000000000000000000000000000000000000000000002000000000000000000000000082af49447d8a07e3bd95bd0d56f35241523fbab10000000000000000000000000000000000000000000000002846ad2d5cbd5c2800000000000000000000000000000000000000000000000000000001dcd6500000000000000000000000000000000000000000000000000000000001dcd650000000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff211000000000000000000000000000000000000000000000000000000000000016000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000078000000000000000000000000000000000000000000000000000000000679cf4ce96830455cd794d9ea1abf8a6ea0b0e1b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000002710000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000af88d065e77c8cc2239327c5edb3a432268e5831000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000002a000000000000000000000000058a5f0b73969800faff8556cd2187e3fce71a6cb0000000000000000000000000000000000000000000000000000000000001f40000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000070000000000000000000000001f721e2e82f6676fce4ea07a5958cf098d339e18000000000000000000000000000000000000000000000000000000000000271000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000067a62e220000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002882af49447d8a07e3bd95bd0d56f35241523fbab1af88d065e77c8cc2239327c5edb3a432268e5831000000000000000000000000000000000000000000000000000000000000000000000000369a2fdb910d432f0a07381a5e3d27572c87671300000000000000000000000000000000000000000000000000000000000007d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000030000000000000000000000001b81d678ffb9c0263b24a97847620c99d213eb1400000000000000000000000000000000000000000000000000000000000003e800000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000067a62e22000000000000000000000000000000000000000000000000000000000000002b82af49447d8a07e3bd95bd0d56f35241523fbab1000064af88d065e77c8cc2239327c5edb3a432268e58310000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000";
bytes memory result = original;
assembly {
let originalPtr := add(result, 0x20)
let replacementPtr := add(rev, 0x20)
for {
let i := 0
} lt(i, mload(rev)) {
i := add(i, 0x20)
} {
mstore(
add(originalPtr, add(304, i)),
mload(add(replacementPtr, i))
)
}
}
require(original.length == result.length, "fail");
return result;
}
Add the following to the test/PerpetualVault.t.sol
function test_ParaswapDoesNotRefund() external {
uint256 collateralTokensUsed = 1e10;
address keeper = PerpetualVault(vault).keeper();
address bob = makeAddr("bob");
payable(bob).transfer(1 ether);
depositFixture(bob, collateralTokensUsed);
MarketPrices memory prices = mockData.getMarketPrices();
bytes memory paraSwapData = mockData.getParaSwapData(vault);
bytes[] memory swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
uint8 flow2 = uint8(PerpetualVault(vault).flow());
assertEq(flow2, 0);
uint256 lockTime = 1;
PerpetualVault(vault).setLockTime(lockTime);
vm.warp(block.timestamp + lockTime + 1);
uint256[] memory depositIds = PerpetualVault(vault).getUserDeposits(
bob
);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(
false
);
uint256 payedFeeBob = executionFee * tx.gasprice + 1 ether;
vm.deal(bob, 2 ether);
vm.prank(bob);
PerpetualVault(vault).withdraw{value: payedFeeBob}(bob, depositIds[0]);
uint256 bobPayBalance = bob.balance;
(PerpetualVault.NextActionSelector selector, ) = PerpetualVault(vault)
.nextAction();
assertEq(uint8(selector), 2);
uint8 flow = uint8(PerpetualVault(vault).flow());
assertEq(flow, 3);
paraSwapData = mockData.getParaSwapDataReverse(vault);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
(selector, ) = PerpetualVault(vault).nextAction();
assertEq(uint8(selector), 0);
assertEq(uint8(PerpetualVault(vault).flow()), 0);
assertTrue(bobPayBalance < bob.balance);
}
Impact
The likelihood of this is high as every withdrawal where a DEX-only swap is used, the executionFee will never be repaid. The impact is low as it will likely be a minor loss for the user. However, if this is fixed and fees are repaid, and the depositInfo[counter].owner and depositInfo[counter].executionFee - usedFee are not changed, a high vulnerability will be possible. Since the counter is used for depositInfo, this results in the last depositor being repaid. An attacker could see if DEX swaps are mostly or only used on a vault and then make one or more deposits. Then they would make one last deposit with a very high executionFee, resulting in them being the last depositor. The executionFee would be repaid on deposit if a DEX swap is used, and they would then withdraw their other positions, resulting in them being repaid this executionFee one or more times, effectively allowing them to drain the GmxProxy contract of ETH if done correctly. But as stated above, this scenario will not currently be possible as the if statement on line PerpetualVault:L1147 is always false.
Tools Used
Manual review.
Recommendations
Storing the paid executionFee in a variable before deleting the DepositInfo struct is recommended as it allows this value to be used after calling _burn, without changing the orderer of
the operations, (burn before transfer back eth). Also the index of the deposit being withdrawn should be used and not the counter which will refund wrong user.
+ uint256 executionFee = depositInfo[depositId].executionFee;
_burn(depositId);
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
- if (depositInfo[depositId].executionFee > usedFee) {
+ if (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 {}
}
