StrategyGeistEth

There are several avenues through which the contract’s owner address can remove funds from the strategy contract to another arbitrary address.

The owner can call the setVault function with an arbitrary and possibly malicious contract address, setting the strategy’s vault to that arbitrary contract. This new arbitrary contract is now free to execute retireStrat and extract the strategy’s funds.

Additionally, the owner can call the setUnirouter function with an arbitrary and possibly malicious contract address, setting the strategy’s unirouter to that arbitrary contract. The owner can then approve the maximum allowance for any token to that arbitrary address by calling addRewardToNativeRoute, potentially leading to an extraction of the strategy’s funds.

Add a contract level timelock for the setUnirouter and setVault functions.

To mitigate any centralization risk, it is always recommended that controlling addresses be multi-sig and timelocked. It is known that Beefy Finance’s owner address is in fact a timelocked multi-sig, therefore this centralization risk is considered low.

Both the transfer and swapExactTokensForTokens functions have return values that should be checked, otherwise some unexpected exception may occur. It is important to have some logic in the event these executions fail.

Check the return values, or opt for a safeTransfer alternative.

The strategy contains 8 individual IERC20(want).balanceOf(address(this)) calls, but this code is already abstracted in the balanceOfWant function.

Reuse the balanceOfWant function in these 8 spots for improved code readability and maintainability.

The functions callReward, panic, userAccountData, outputToNative, rewardToNative, and nativeToWant are marked as public, but are never called from inside the contract.

The functions callReward, panic, userAccountData, outputToNative, rewardToNative, and nativeToWant can be marked external for gas optimization.

The StrategyGeist contract inherits from the StratManager and FeeManager contracts, but notice that the FeeManager contract also inherits from the StratManager contract.

Alter the inheritance hierarchy so that there are no redundancies, or provide a veritable reason for such an inheritance structure.

The require(msg.sender == vault, "!vault") require statement appears 3 separate times at the beginning of beforeDeposit, withdraw, and retireStrat.

Consider making this requires statement into a reusable modifier.

The contract contains two require statements: require(_borrowRate <= borrowRateMax, "!safe") on line 1596 and require(_borrowRate <= borrowRateMax, "!rate") on line 1616 that enforce the same restrictions, but yield different error strings.

Consider standardizing these error strings.

The implementation for the harvest and managerHarvest functions is exactly the same, the only difference being that the manager can call the managerHarvest function.

Remove the managerHarvest function or provide a veritable reason for its existence.