QA Report

[code423n4]

BeefyVaultOperator.sol

deposit: setMaxAllowance should be called in the constructor

In the deposit function on line 48, we call ExchangeHelpers.setMaxAllowance(token, vault); to allow the vault to spend token.

Each time assets are deposited in the vault, we shouldn't have to allow it to spend the token again. I recommend to call ExchangeHelpers.setMaxAllowance(token, vault) only once in the constructor for each vault and token. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

Recommendation:

for (uint256 i; i < vaultsLength; i++) {
    operatorStorage.addVault(vaults[i], tokens[i]);
    ExchangeHelpers.setMaxAllowance(tokens[i], vaults[i]);
}

BeefyZapBiswapLPVaultOperator.sol and BeefyZapUniswapLPVaultOperator.sol

zapAndStakeLp: setMaxAllowance should be called in the constructor

In the _zapAndStakeLp function on line 189 and subsequently on line 194 and 195 we call ExchangeHelpers.setMaxAllowance(); to allow the vault to spend token.

Each time assets are deposited in the vault, we shouldn't have to allow it to spend the token again. I recommend to call ExchangeHelpers.setMaxAllowance() only once in the constructor for each vault and token. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

This will also avoid calling ExchangeHelpers.setMaxAllowance(IERC20(swapToken), router); in the _withdrawAndSwap function on line 162.

Recommendation:

for (uint256 i; i < vaultsLength; i++) {
    operatorStorage.addVault(vaults[i], routers[i]);

    IBiswapPair pair = IBiswapPair(IBeefyVaultV6(vaults[i]).want());

    ExchangeHelpers.setMaxAllowance(IERC20(address(pair)), address(vaults[i]));
    ExchangeHelpers.setMaxAllowance(IERC20(pair.token0()), routers[i]);
    ExchangeHelpers.setMaxAllowance(IERC20(pair.token1()), routers[i]);
}

YearnCurveVaultOperator.sol

depositETH: setMaxAllowance should be called in the constructor

In the depositETH function on line 78, we call ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer)); to allow the withdrawer to spend weth.

Each time assets are deposited in the vault, we shouldn't have to allow the withdrawer to spend weth again. I recommend to call ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer)) only once in the constructor. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

Recommendation:

ExchangeHelpers.setMaxAllowance(IERC20(_weth), address(_withdrawer));

StakingLPVaultHelpers.sol

addLiquidityAndDepositETH: setMaxAllowance should be called in addVault

In the addLiquidityAndDepositETH function on line 45, we call ExchangeHelpers.setMaxAllowance(lpToken, vault); to allow the vault to spend lpToken.

Each time assets are deposited in the vault, we shouldn't have to allow the vault to spend lpToken again. I recommend to call ExchangeHelpers.setMaxAllowance(lpToken, vault); only once in the addVault function of the YearnVaultStorage. I also recommend to add a setMaxAllowance function only callable by the owner that would allow to set the max allowance in case the allowance has decreased.

This will also avoid calling it in the _addLiquidityAndDeposit function on line 77

Recommendation:

ExchangeHelpers.setMaxAllowance(curvePool.lpToken, vault);

[Yashiru]

setMaxAllowance should be called in the constructor (Acknowledged)

• It could be interesting in the case of a vault token (shares).
• This could become very complex when it comes to LP tokens, as there is much more to approve.

Conclusion

As it is the factory that has the approval in this context, we cannot be sure that no other operator will change the allocation. So we prefer call the setMaxAllowance in final process to be sure that the factory has the allowance.