Badger-Vested-Aura contest - robee's results

Mult instead div in compares

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

Code instance:

    MyStrategy.sol, 205, require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check");

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instances:

    Deprecated safeApprove in MyStrategy.sol line 67: WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);
    Deprecated safeApprove in MyStrategy.sol line 66: AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
    Deprecated safeApprove in MyStrategy.sol line 64: AURA.safeApprove(address(LOCKER), type(uint256).max);

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    MyStrategy.sol.initialize _vault
    MyStrategy.sol.manualSetDelegate delegate

Hardcoded WETH

WETH address is hardcoded but it may differ on other chains, e.g. Polygon, so make sure to check this before deploying and update if necessary You should consider injecting WETH address via the constructor. (previous issue: https://github.com/code-423n4/2021-10-ambire-findings/issues/54)

Code instance:

    Hardcoded weth address in MyStrategy.sol

Init frontrun

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

Code instance:

    MyStrategy.sol, initialize, 56

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instances:

    MyStrategy.sol, reinvest is missing a reentrancy modifier
    MyStrategy.sol, initialize is missing a reentrancy modifier

Assert instead require to validate user inputs

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    

Code instance:

    MyStrategy.sol : reachable assert in line 56

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    MyStrategy.sol: function performUpkeep parameter performData isn't used. (performUpkeep is external)
    MyStrategy.sol: function checkUpkeep parameter checkData isn't used. (checkUpkeep is external)

Open TODOs

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Code instances:

Open TODO in MyStrategy.sol line 283 : // TODO: Hardcode claim.account = address(this)?

Open TODO in MyStrategy.sol line 421 : // TODO: Too many SLOADs

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L326
    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L429
    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L112
    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L423

Assert instead require to validate user inputs

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    

Code instance:

    MyStrategy.sol : reachable assert in line 56

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    MyStrategy.sol: function performUpkeep parameter performData isn't used. (performUpkeep is external)
    MyStrategy.sol: function checkUpkeep parameter checkData isn't used. (checkUpkeep is external)

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L92
    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L98
    https://github.com/code-423n4/2022-06-badger/tree/main/MyStrategy.sol#L86

-------- med ---------

Must approve 0 first

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code instances:

approve without approving 0 first MyStrategy.sol, 64, AURA.safeApprove(address(LOCKER), type(uint256).max);

approve without approving 0 first MyStrategy.sol, 66, AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);

approve without approving 0 first MyStrategy.sol, 67, WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

Caching array length can save gas

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

Code instances:

    MyStrategy.sol, _claims, 317
    MyStrategy.sol, _claims, 300

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: MyStrategy.sol, i, 317
    just change to unchecked: MyStrategy.sol, i, 153
    change to prefix increment and unchecked: MyStrategy.sol, i, 118
    change to prefix increment and unchecked: MyStrategy.sol, i, 300

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    MyStrategy.sol, 300
    MyStrategy.sol, 317
    MyStrategy.sol, 118

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instance:

In MyStrategy.sol,rearranging the storage fields can optimize to: 14 slots from: 15 slots. The new order of types (you choose the actual variables): 1. IBribesProcessor 2. IBalancerVault 3. IAuraLocker 4. IERC20Upgradeable 5. IERC20Upgradeable 6. IERC20Upgradeable 7. IERC20Upgradeable 8. IERC20Upgradeable 9. bytes32 10. bytes32 11. bytes32 12. uint256 13. address 14. bool 15. bool 16. bool 17. address

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    MyStrategy.sol, _getBalance, { return IVault(vault).balance(); }
    MyStrategy.sol, _getPricePerFullShare, { return IVault(vault).getPricePerFullShare(); }
    MyStrategy.sol, _isTendable, { return false; // Change to true if the strategy should be tended }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    MyStrategy.sol, _sendBadgerToTree
    MyStrategy.sol, _sendTokenToBribesProcessor
    MyStrategy.sol, _notifyBribesProcessor

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code instance:

    MyStrategy.sol