Separation of concerns and consistency in vaults
The `FujiVaultFTM` contract contains multiple balance-changing functions. Most notably, `withdraw` is passed an `int256` denoted amount parameter. Negative values of this parameter are given to the `_internalWithdraw` function, where they trigger the withdrawal of all collateral. This approach can result in accounting mistakes in the future as beyond a certain point in the vault's accounting; amounts are expected to be only positive. Furthermore, the concerns of withdrawing and entirely withdrawing are not separated.\\nThe above issue applies analogously to the `payback` function and its dependency on `_internalPayback`.\\nFor consistency, `withdrawLiq` also takes an `int256` amount parameter. This function is only accessible to the `Fliquidator` contract and withdraws collateral from the active provider. However, all occurrences of the `_withdrawAmount` parameter are cast to `uint256`.\\nThe `withdraw` entry point:\\n```\\nfunction withdraw(int256 \\_withdrawAmount) public override nonReentrant {\\n updateF1155Balances();\\n \\_internalWithdraw(\\_withdrawAmount);\\n}\\n```\\n\\n_internalWithdraw's negative amount check:\\n```\\nuint256 amountToWithdraw = \\_withdrawAmount < 0\\n ? providedCollateral - neededCollateral\\n : uint256(\\_withdrawAmount);\\n```\\n\\nThe `withdrawLiq` entry point for the Fliquidator:\\n```\\nfunction withdrawLiq(int256 \\_withdrawAmount) external override nonReentrant onlyFliquidator {\\n // Logic used when called by Fliquidator\\n \\_withdraw(uint256(\\_withdrawAmount), address(activeProvider));\\n IERC20Upgradeable(vAssets.collateralAsset).univTransfer(\\n payable(msg.sender),\\n uint256(\\_withdrawAmount)\\n );\\n}\\n```\\n
We recommend splitting the `withdraw(int256)` function into two: `withdraw(uint256)` and `withdrawAll()`. These will provide the same functionality while rendering the updated code of `_internalWithdraw` easier to read, maintain, and harder to manipulate. The recommendation applies to `payback` and `_internalPayback`.\\nSimilarly, withdrawLiq's parameter should be a `uint256` to prevent unnecessary casts.
```\\nfunction withdraw(int256 \\_withdrawAmount) public override nonReentrant {\\n updateF1155Balances();\\n \\_internalWithdraw(\\_withdrawAmount);\\n}\\n```\\n