All tests are missing, no tests = guarantee that the code is unsafe
Not sure why the controller was brought on, but am assuming is as pseudo-registry.
If you with to validate a vault you could compare vs the registry directly via registry.getProductionVaults
I wouldn't recommend that, and instead would recommend allowing the caller to pass in the values (unless the Router has mixed use, but you sent me no additional info so I cannot comment on this).
I would then simplify the withdrawal pattern which looks very off and will most likely cause possibility of griefing and reverts except in the most basic case (defeating the purpose of writing the code)
I'm pretty sure there's more controllers and some vaults may use those.
The controller in the Registry is the main one, but other controllers do exist so using that approach may cause issues / reverts
While you can get the balance from vault and strategy in the same way, a 1.5 vault will not use the controller, so all calls to it will not work.
This means the Router is relegated to using older vaults.
## Odd "all shares patterns" will cause reverts https://github.com/immunefi-team/BadgerVaultRouter/blob/f0e9330b4bb92d0fcd3dd8b1802e747a4db26362/contracts/router/BadgerVaultRouter.sol#L159-L160
IERC20(allVaults[i]).safeTransferFrom(msg.sender, address(this), sharesToWithdraw);
If you forcefully withdraw from all vaults, and the caller only has one, they will not be able to withdraw as the transfers will fail
if (withdrawnTotal >= amount) {
I think you want to revert here otherwise the shares have been burnt and wont be sent back
## Unconventional notation
Easier to scan if written 10_000
_MAX_BASIS = 100_00
## Unclear totalToken
function totalToken(address token) public view returns (uint256) {
The function returns the total want in the Vault + Strategy, not clear why
require(actualOutput >= before + minOutput, "Insufficient output amount");
While logically similar, it may be best to use the delta out, and do a > comparison
As the above can return true for zero
0 >= 0 + 0
## Technically strategy may have idle want as well
Although strat may charge a withdrawal fee
Also assuming that the loss will be the withdrawalMaxDeviationThreshold is incorrect.
The maxLoss can be that, but it may be zero.
There is no way of knowing beforehand (although 99.99999999% of vaults have zero loss mechanism)
This is fine because Setts always use 18 decimals but may not work for other projects where each vault has specific decimals
return (amount * 1e18) / IBadgerVault(controller.vaults(token)).getPricePerFullShare();