re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L182-L197

Summary

re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault

      if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
        self.WNT.withdraw(self.withdrawCache.tokensToUser);
@>audit transfer ETH and call        (bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
        require(success, "Transfer failed.");
      } else {
        // Transfer requested withdraw asset to user
        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)));
      self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));

      // Burn user shares
@> burn is after      self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L182-L197

Since the function is only accessible by keeper (likely a router), which from the example of the mockRouter, would bundle the withdraw and "afterWithdrawalExecution" together. However since the router is out-of-scope, and there is still a possible chance that the user can make use of the router to re-enter into the function (without re-entrency lock), and be able to drain more fund that he actually deserves. This is submitted as a medium risk.

Vulnerability Details

Impact

drain of user funds.

Tools Used

Recommendations

burn user's share first, before executing external call at the end.