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

Submitted by Citris, asimaranov. Selected submission by: Citris.

Relevant GitHub Links

Summary

re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault
plain text
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);
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.