Summary
If the "processDepositCancellation" flow is entered, a malicious contract user with no receive function can get the vault stuck on "Deposit" status
Vulnerability Details
If the vault token deposited by malicious user is native, and the deposit flow ends with the GMX keeper calling "processDepositCancellation()" function on the vault, it is trivial to get the status of the vault stuck on "Deposit". The probability of entering the "processDepositCancellation" flow can be maximized by the user by entering the minimum slippage allowed by the vault and spamming the vault with deposits until the slippage is too tight to pass. Other potential avenues for entering "processDepositCancellation()" flow include setting the slippage too high, or if there isn’t enough available supply to buy into GM
This can be seen in the "processDepositCancellation()" function detailed in the GMXDeposit library:
function processDepositCancellation(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositCancellationChecks(self);
GMXManager.repay(
self,
self.depositCache.borrowParams.borrowTokenAAmt,
self.depositCache.borrowParams.borrowTokenBAmt
);
if (self.depositCache.depositParams.token == address(self.WNT)) {
self.WNT.withdraw(self.WNT.balanceOf(address(this)));
(bool success, ) = self.depositCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
IERC20(self.depositCache.depositParams.token).safeTransfer(
self.depositCache.user,
self.depositCache.depositParams.amt
);
}
self.status = GMXTypes.Status.Open;
emit DepositCancelled(self.depositCache.user);
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXDeposit.sol#L193-L222
PoC
diff --git a/test/gmx/local/GMXDepositTest.t.sol b/test/gmx/local/GMXDepositTest.t.sol
index 872610a..36b994e 100644
--- a/test/gmx/local/GMXDepositTest.t.sol
+++ b/test/gmx/local/GMXDepositTest.t.sol
@@ -136,6 +136,27 @@ contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
assertEq(uint256(vault.store().status), 0);
}
+ function test_processDepositCancelMaliciousRevert() external {
+ MaliciousUser maliciousUser = new MaliciousUser();
+ deal(address(maliciousUser), 10 ether);
+ deal(address(WETH), address(maliciousUser), 10 ether);
+ vm.startPrank(address(maliciousUser));
+
+ _createNativeDeposit(address(WETH), 1e18, 0, SLIPPAGE, EXECUTION_FEE);
+
+ vm.expectRevert();
+
+ mockExchangeRouter.cancelDeposit(
+ address(WETH),
+ address(USDC),
+ address(vault),
+ address(callback)
+ );
+
+
+ assertEq(uint256(vault.store().status), 1);
+ }
+
function test_processDepositFailure() external {
vm.startPrank(user1);
@@ -348,3 +369,8 @@ contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
}
+
+
+contract MaliciousUser {
+
+}
Impact
Vault status will be stuck on "Deposit" and vault won't be able to accept any new deposits, withdrawals, rebalances or compounds
Recommendations
Pay native depositors back in WETH instead of ETH. It simplifies the code and removes the attack vector
diff --git a/contracts/strategy/gmx/GMXDeposit.sol b/contracts/strategy/gmx/GMXDeposit.sol
index 1b28c3b..270e55f 100644
--- a/contracts/strategy/gmx/GMXDeposit.sol
+++ b/contracts/strategy/gmx/GMXDeposit.sol
@@ -203,18 +203,11 @@ library GMXDeposit {
);
-
- if (self.depositCache.depositParams.token == address(self.WNT)) {
- self.WNT.withdraw(self.WNT.balanceOf(address(this)));
- (bool success, ) = self.depositCache.user.call{value: address(this).balance}("");
- require(success, "Transfer failed.");
- } else {
-
- IERC20(self.depositCache.depositParams.token).safeTransfer(
- self.depositCache.user,
- self.depositCache.depositParams.amt
- );
- }
+
+ IERC20(self.depositCache.depositParams.token).safeTransfer(
+ self.depositCache.user,
+ self.depositCache.depositParams.amt
+ );
self.status = GMXTypes.Status.Open;
