Exchange owner might steal users' funds using reentrancy
The practice of pulling funds from a user (by using safeTransferFrom) and then later pushing (some) of the funds back to the user occurs in various places in the `Exchange` contract. In case one of the used token contracts (or one of its dependent calls) externally calls the `Exchange` owner, the owner may utilize that to call back `Exchange.recoverLostFunds` and drain (some) user funds.\\n```\\nfunction convertFundsFromInput(address \\_from, address \\_to, uint256 \\_inputAmount, uint256 \\_minOutputAmount) external override returns (uint256 \\_outputAmount)\\n{\\n address \\_sender = msg.sender;\\n Transfers.\\_pullFunds(\\_from, \\_sender, \\_inputAmount);\\n \\_inputAmount = Math.\\_min(\\_inputAmount, Transfers.\\_getBalance(\\_from)); // deals with potential transfer tax\\n \\_outputAmount = UniswapV2ExchangeAbstraction.\\_convertFundsFromInput(router, \\_from, \\_to, \\_inputAmount, \\_minOutputAmount);\\n \\_outputAmount = Math.\\_min(\\_outputAmount, Transfers.\\_getBalance(\\_to)); // deals with potential transfer tax\\n Transfers.\\_pushFunds(\\_to, \\_sender, \\_outputAmount);\\n return \\_outputAmount;\\n}\\n```\\n\\n```\\nfunction joinPoolFromInput(address \\_pool, address \\_token, uint256 \\_inputAmount, uint256 \\_minOutputShares) external override returns (uint256 \\_outputShares)\\n{\\n address \\_sender = msg.sender;\\n Transfers.\\_pullFunds(\\_token, \\_sender, \\_inputAmount);\\n \\_inputAmount = Math.\\_min(\\_inputAmount, Transfers.\\_getBalance(\\_token)); // deals with potential transfer tax\\n \\_outputShares = UniswapV2LiquidityPoolAbstraction.\\_joinPoolFromInput(router, \\_pool, \\_token, \\_inputAmount, \\_minOutputShares);\\n \\_outputShares = Math.\\_min(\\_outputShares, Transfers.\\_getBalance(\\_pool)); // deals with potential transfer tax\\n Transfers.\\_pushFunds(\\_pool, \\_sender, \\_outputShares);\\n return \\_outputShares;\\n}\\n```\\n\\n```\\nfunction convertFundsFromOutput(address \\_from, address \\_to, uint256 \\_outputAmount, uint256 \\_maxInputAmount) external override returns (uint256 \\_inputAmount)\\n{\\n address \\_sender = msg.sender;\\n Transfers.\\_pullFunds(\\_from, \\_sender, \\_maxInputAmount);\\n \\_maxInputAmount = Math.\\_min(\\_maxInputAmount, Transfers.\\_getBalance(\\_from)); // deals with potential transfer tax\\n \\_inputAmount = UniswapV2ExchangeAbstraction.\\_convertFundsFromOutput(router, \\_from, \\_to, \\_outputAmount, \\_maxInputAmount);\\n uint256 \\_refundAmount = \\_maxInputAmount - \\_inputAmount;\\n \\_refundAmount = Math.\\_min(\\_refundAmount, Transfers.\\_getBalance(\\_from)); // deals with potential transfer tax\\n Transfers.\\_pushFunds(\\_from, \\_sender, \\_refundAmount);\\n \\_outputAmount = Math.\\_min(\\_outputAmount, Transfers.\\_getBalance(\\_to)); // deals with potential transfer tax\\n Transfers.\\_pushFunds(\\_to, \\_sender, \\_outputAmount);\\n return \\_inputAmount;\\n}\\n```\\n\\n```\\nfunction recoverLostFunds(address \\_token) external onlyOwner\\n{\\n uint256 \\_balance = Transfers.\\_getBalance(\\_token);\\n Transfers.\\_pushFunds(\\_token, treasury, \\_balance);\\n}\\n```\\n
Reentrancy guard protection should be added to `Exchange.convertFundsFromInput`, `Exchange.convertFundsFromOutput`, `Exchange.joinPoolFromInput`, `Exchange.recoverLostFunds` at least, and in general to all public/external functions since gas price considerations are less relevant for contracts deployed on BSC.
```\\nfunction convertFundsFromInput(address \\_from, address \\_to, uint256 \\_inputAmount, uint256 \\_minOutputAmount) external override returns (uint256 \\_outputAmount)\\n{\\n address \\_sender = msg.sender;\\n Transfers.\\_pullFunds(\\_from, \\_sender, \\_inputAmount);\\n \\_inputAmount = Math.\\_min(\\_inputAmount, Transfers.\\_getBalance(\\_from)); // deals with potential transfer tax\\n \\_outputAmount = UniswapV2ExchangeAbstraction.\\_convertFundsFromInput(router, \\_from, \\_to, \\_inputAmount, \\_minOutputAmount);\\n \\_outputAmount = Math.\\_min(\\_outputAmount, Transfers.\\_getBalance(\\_to)); // deals with potential transfer tax\\n Transfers.\\_pushFunds(\\_to, \\_sender, \\_outputAmount);\\n return \\_outputAmount;\\n}\\n```\\n