The transfer of ERC-20 tokens with blacklist functionality in process functions can lead to stuck vaults
Inside a few process functions are ERC-20 tokens transfered which could potentially have a blacklist functionality. This can lead to a DoS of the strategy vault. If for example, a blacklisted user withdraws funds.\\nSome ERC-20 tokens like for example USDC (which is used by the system) have the functionality to blacklist specific addresses, so that they are no longer able to transfer and receive tokens. Sending funds to these addresses will lead to a revert. A few of the process functions inside the deposit and withdraw contracts transfer ERC-20 tokens to addresses which could potentially be blacklisted. The system is not in an Open state when a keeper bot interacts with such a process function, and if the call to such a function reverts, the status can not be updated back to Open. Therefore, it will remain in the given status and a DoS for all users occurs. The only possibility that DoS stops would be when the user is no longer blacklisted, which can potentially last forever.\\nThe attack flow (could be accidental) would for example look like this:\\nUSDC Blacklisted user calls withdraw with the wish to withdraw USDC\\nwithdraw function passes and status is updated to GMXTypes.Status.Withdraw\\nKeeper calls the processWithdraw function\\nTransferring USDC tokens to blacklisted user reverts\\nTherefore vault is stuck inside GMXTypes.Status.Withdraw status and all users experience a DoS\\nHere are the code snippets of these dangerous transfers inside process functions:\\n```\\nfunction processDepositCancellation(\\n  GMXTypes.Store storage self\\n) external {\\n  GMXChecks.beforeProcessDepositCancellationChecks(self);\\n // rest of code\\n // Transfer requested withdraw asset to user\\n IERC20(self.depositCache.depositParams.token).safeTransfer(\\n  self.depositCache.user,\\n  self.depositCache.depositParams.amt\\n );\\n  // rest of code\\n  self.status = GMXTypes.Status.Open;\\n\\n  emit DepositCancelled(self.depositCache.user);\\n}\\n```\\n\\n```\\nfunction processDepositFailureLiquidityWithdrawal(\\n  GMXTypes.Store storage self\\n) public {\\n  GMXChecks.beforeProcessAfterDepositFailureLiquidityWithdrawal(self);\\n // rest of code\\n  // Refund user the rest of the remaining withdrawn LP assets\\n  // Will be in tokenA/tokenB only; so if user deposited LP tokens\\n  // they will still be refunded in tokenA/tokenB\\n  self.tokenA.safeTransfer(self.depositCache.user, self.tokenA.balanceOf(address(this)));\\n  self.tokenB.safeTransfer(self.depositCache.user, self.tokenB.balanceOf(address(this)));\\n // rest of code\\n  self.status = GMXTypes.Status.Open;\\n}\\n```\\n\\n```\\nfunction processWithdraw(\\n  GMXTypes.Store storage self\\n) external {\\n  GMXChecks.beforeProcessWithdrawChecks(self);\\n\\n  try GMXProcessWithdraw.processWithdraw(self) {\\n    if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {\\n   // rest of code\\n    } else {\\n      // Transfer requested withdraw asset to user\\n      IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(\\n        self.withdrawCache.user,\\n        self.withdrawCache.tokensToUser\\n      );\\n    }\\n\\n    // Transfer any remaining tokenA/B that was unused (due to slippage) to user as well\\n    self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));\\n    self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));\\n  \\n  // rest of code\\n\\n    self.status = GMXTypes.Status.Open;\\n  }\\n // rest of code\\n}\\n```\\n
Instead of transferring the ERC-20 tokens directly to a user in the process functions, use a two-step process instead. For example, create another contract whose only purpose is to hold assets and store the information about which address is allowed to withdraw how many of the specified tokens. In the process functions, send the funds to this new contract along with this information instead. So if a user has been blacklisted, the DoS only exists for that specific user and for the rest of the users the system continues to function normally.
DoS of the entire strategy vault, as the status can no longer be updated to Open until the user is no longer blacklisted. This can potentially take forever and forces the owners to take emergency action.
```\\nfunction processDepositCancellation(\\n  GMXTypes.Store storage self\\n) external {\\n  GMXChecks.beforeProcessDepositCancellationChecks(self);\\n // rest of code\\n // Transfer requested withdraw asset to user\\n IERC20(self.depositCache.depositParams.token).safeTransfer(\\n  self.depositCache.user,\\n  self.depositCache.depositParams.amt\\n );\\n  // rest of code\\n  self.status = GMXTypes.Status.Open;\\n\\n  emit DepositCancelled(self.depositCache.user);\\n}\\n```\\n