Description
In the protocol when ever a user calls PerpetualVault::deposit or PerpetualVault::withdraw an executionFee is transferred by the user upfront usually much more than the actual fee in order to cover the execution costs and the remaining is refunded to the user at the end of the call.
In Withdraw , _handleReturn observe this segment
if (amount > 0) {
_transferToken(depositId, amount);
}
emit Burned(depositId, depositInfo[depositId].recipient, depositInfo[depositId].shares, amount);
@> _burn(depositId);
console.log("execution Fee before Burn", depositInfo[depositId].executionFee);
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(
@> depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee
) {} catch {
}
}
}
The depositId of a user is burned and depositInfo[depositId] of a user is deleted before refunding the execution fee to the user i.e depositInfo[depositId].executionFee will always be 0 and the if statement will never be true , denying users their refunds or any extra upfront they sent for the transaction
Impact
Users can never Refund their Execution Fee during Withdrawals, denying the users of their rightful refunds ultimately affects the reputation and trust in the protocol.
Proof of Concept
The Poc Requires some changes to be made
In the PerpetualVaultTest::depositFixture testing helper function add the following line
vm.deal(user, executionFee * tx.gasprice + 1 ether);
and also add import {console} from "forge-std/console.sol"; in the test file and PerpetualVault.sol
In PerpetualVault::withdraw make the following change
- if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {
+ if (depositInfo[depositId].timestamp + lockTime > block.timestamp) {
revert Error.Locked();
}
we do this as we set lockTime to 0 before withdrawal to simulate locktimepassed and also to make a successful gmxExecute within the same block (ease of use instead of generating new oracle params )
-
In the scenario below required a deposit and a withdrawal into the PerpetualVault i.e using the
DEX but generating calladata for the GMX is much easier so we simulate DEX swap but actually do GMX swap (using DEX or GMX doesn't matter as long as we reach the refund logic)
In PerpetualVault::_runSwap function segment make the following changes
(add the "+" parts and comment out the " - " parts and read comments provided for a better understanding)
+ add statements with +
- comment out (donot remove) the statements with -
.
..
...
....
......
if (metadata.length != 1) {
revert Error.InvalidData();
}
console.log("ping 1 ");
(PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
if (_protocol == PROTOCOL.DEX) {
console.log("reached this 1");
// comment out the dex swap part
- // uint256 outputAmount = _doDexSwap(data, isCollateralToIndex);
// add this gmx swap part
+ _doGmxSwap(data, isCollateralToIndex);
// update global state
if (flow == FLOW.DEPOSIT) {
// the outputAmount values from trading in gmx
// The gmx swap actually requires 2 transactions
// where as DEX required only 1 transaction , so we hard code
// Output Amount here and deal the required collateral in the test
// simulating GMX deposit but in DEX style i.e in one transaction
+ uint256 outputAmount = 5868596470015897108;
// 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) {
+ uint256 outputAmount = 19925273714;
//This is the actual function with the refund Fee part
_handleReturn(outputAmount + swapProgressData.swapped, false, true);
}
.....
....
...
..
.
In the PerpetualVault::_handleReturn function add the following console.log statements
...
..
.
if (amount > 0) {
_transferToken(depositId, amount);
}
+ console.log("execution Fee before Burn", depositInfo[depositId].executionFee);
+ console.log(
"Fee Difference before Burn", depositInfo[depositId].executionFee - (callbackGasLimit * tx.gasprice)
);
emit Burned(depositId, depositInfo[depositId].recipient, depositInfo[depositId].shares, amount);
_burn(depositId);
+ console.log("execution Fee before Burn", depositInfo[depositId].executionFee);
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
+ console.log("execution Fee before If statement", depositInfo[depositId].executionFee);
+ console.log("usedFee", usedFee);
if (depositInfo[depositId].executionFee > usedFee) {
+ console.log("Refund 2 Owner: ", depositInfo[counter].owner);
+ console.log("Refund 2 : ", depositInfo[counter].executionFee - usedFee);
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee
) {} catch {
}
}
}
Note: Paste the Test Below in PerpetualVault.t.sol , run it with -vv option to view the logs
function testBurnRefundFee() public {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
depositFixture(alice, 2e10);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory swapData = new bytes[](1);
address[] memory gmxPath = new address[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
uint256 minOutputAmount = 2e10 * prices.shortTokenPrice.min / prices.longTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.DEX, abi.encode(gmxPath, 2e10, minOutputAmount));
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
assertEq(PerpetualVault(vault).isLock(), true);
GmxOrderExecuted(true);
assertEq(uint8(PerpetualVault(vault).flow()), 0);
assertEq(PerpetualVault(vault).positionIsClosed(), false);
console.log("----------");
console.logBool(PerpetualVault(vault).beenLong());
console.log("----------");
(PerpetualVault.NextActionSelector selector,) = PerpetualVault(vault).nextAction();
assertEq(uint8(selector), 0);
console.log("Log Begin :", ERC20(address(PerpetualVault(vault).indexToken())).balanceOf(address(vault)));
console.log(
"Collateral Token begin :", IERC20(PerpetualVault(vault).collateralToken()).balanceOf(address(vault))
);
console.log("total shares_ Begin:", PerpetualVault(vault).totalShares());
deal(address(PerpetualVault(vault).collateralToken()), address(PerpetualVault(vault)), 19925273714);
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(0);
vm.startPrank(alice);
uint256[] memory depositIds = PerpetualVault(vault).getUserDeposits(alice);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(false);
vm.deal(alice, executionFee * tx.gasprice);
PerpetualVault(vault).withdraw{value: executionFee * tx.gasprice}(alice, depositIds[0]);
(PerpetualVault.NextActionSelector selector1,) = PerpetualVault(vault).nextAction();
assertEq(uint8(selector1), 2);
assertEq(uint8(PerpetualVault(vault).flow()), 3);
vm.stopPrank();
prices = mockData.getMarketPrices();
swapData = new bytes[](1);
gmxPath = new address[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
address indexToken = PerpetualVault(vault).indexToken();
uint256 indexTokenBalance = IERC20(indexToken).balanceOf(vault);
minOutputAmount = indexTokenBalance * prices.longTokenPrice.min / prices.shortTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.DEX, abi.encode(gmxPath, indexTokenBalance, minOutputAmount));
vm.prank(PerpetualVault(vault).keeper());
PerpetualVault(vault).runNextAction(prices, swapData);
(PerpetualVault.NextActionSelector selector_,) = PerpetualVault(vault).nextAction();
GmxOrderExecuted(true);
console.log("Alice Balance:", alice.balance);
assertEq(alice.balance,0);
}
Tools Used
Manual Review
Recommendations
call the _burn(depositId) after refunding the fee to the user
Make the following changes in PerpetualVault::_handleReturn function
...
..
.
emit Burned(depositId, depositInfo[depositId].recipient, depositInfo[depositId].shares, amount);
- _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 {
}
}
}
+ _burn(depositId);
// update global state
delete swapProgressData;
delete flowData;
delete flow;
}
This is safe as the withdraw and runNextAction both have nonReentrant modifier but it does break the CEI(checks-effects-interactions) pattern , so if you strictly wanna follow CEI then cache the depositInfo[depositId].executionFee and depositInfo[counter].owner before the refund function , this way even if burn is called before the refund , the execution fee can be refunded using the cached memory variables .
