Gas Optimizations

[code423n4]

Summary

Gas Optimizations

	Issue	Instances
1	Repeatedly changing isClaimingBribes back to false wastes gas	1
2	Modifier should be skipped for sweepRewards()	1
3	Avoid contract existence checks by using solidity version 0.8.10 or later	9
4	Multiple accesses of a mapping/array should use a local variable cache	5
5	internal functions only called once can be inlined to save gas	3
6	<array>.length should not be looked up in every loop of a for-loop	2
7	require()/revert() strings longer than 32 bytes cost extra gas	1
8	Using bools for storage incurs overhead	3
9	Use a more recent version of solidity	1
10	It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied	3
11	++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)	3
12	Splitting require() statements that use && saves gas	1
13	Using private rather than public for constants, saves gas	3
14	Use custom errors rather than revert()/require() strings to save gas	6

Total: 42 instances over 14 issues

Gas Optimizations

1. Repeatedly changing isClaimingBribes back to false wastes gas

Changing the value from false to true incurs a Gsset (20000 gas), checking it in the receive() incurs a Gwarmaccess (100 gas), and changing it back incurs a Gsreset (2900 gas). Instead you can store the last value of hiddenHandDistributor and change the require() in the receive() to only allow funds when hiddenHandDistributor is msg.sender, which after the first set, would change the Gsset to a Gsreset, and avoid the later Gsreset

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

310          isClaimingBribes = true;
311          hiddenHandDistributor.claim(_claims);
312:         isClaimingBribes = false;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L310-L312

2. Modifier should be skipped for sweepRewards()

sweepRewards() calls sweepRewardToken() for each token in the array, and each call ends up checking for governance, which incurs the cost of external calls to look up the governance and or strategist addresses, each of which incur the cost of at the minimum an EXTCODESIZE (700 gas) plus the cost of an external call

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

108:         _onlyGovernanceOrStrategist();

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L108

3. Avoid contract existence checks by using solidity version 0.8.10 or later

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

There are 9 instances of this issue:

File: contracts/MyStrategy.sol

/// @audit token()
57:           assert(IVault(_vault).token() == address(AURA));

/// @audit balanceOf()
111:          uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this));

/// @audit balanceOf()
305:                  beforeBalance[i] = IERC20Upgradeable(token).balanceOf(address(this));

/// @audit balanceOf()
329:                  uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]);

/// @audit balanceOf()
362:          uint256 toDeposit = IERC20Upgradeable(want).balanceOf(address(this));

/// @audit balance()
397:          return IVault(vault).balance();

/// @audit getPricePerFullShare()
401:          return IVault(vault).getPricePerFullShare();

/// @audit safeTransfer()
423:          IERC20Upgradeable(token).safeTransfer(address(bribesProcessor), amount);

/// @audit safeTransfer()
429:          IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L57

4. Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory

There are 5 instances of this issue:

File: contracts/MyStrategy.sol

/// @audit earnedData[i] on line 154
154:              rewards[i] = TokenAmount(earnedData[i].token, earnedData[i].amount);

/// @audit harvested[0] on line 226
275:              harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

/// @audit harvested[0] on line 275
278:          _reportToVault(harvested[0].amount);

/// @audit harvested[0] on line 278
279:          if (harvested[0].amount > 0) {

/// @audit harvested[0] on line 279
280:              _deposit(harvested[0].amount);

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L154

5. internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

416       function _notifyBribesProcessor() internal {
417:          bribesProcessor.notifyNewRound();

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L416-L417

File: contracts/MyStrategy.sol   #2

421:      function _sendTokenToBribesProcessor(address token, uint256 amount) internal {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L421

File: contracts/MyStrategy.sol   #3

428:      function _sendBadgerToTree(uint256 amount) internal {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L428

6. <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 2 instances of this issue:

File: contracts/MyStrategy.sol   #1

300:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300

File: contracts/MyStrategy.sol   #2

317:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317

7. require()/revert() strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

184           require(
185               balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
186               "You have to wait for unlock or have to manually rebalance out of it"
187:          );

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L184-L187

8. Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

24:       bool public withdrawalSafetyCheck;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L24

File: contracts/MyStrategy.sol   #2

26:       bool public processLocksOnReinvest;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L26

File: contracts/MyStrategy.sol   #3

28:       bool private isClaimingBribes;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L28

9. Use a more recent version of solidity

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

3:    pragma solidity 0.6.12;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L3

10. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

118:          for(uint i = 0; i < length; i++){

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L118

File: contracts/MyStrategy.sol   #2

300:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300

File: contracts/MyStrategy.sol   #3

317:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317

11. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 6 gas per loop

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

118:          for(uint i = 0; i < length; i++){

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L118

File: contracts/MyStrategy.sol   #2

300:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300

File: contracts/MyStrategy.sol   #3

317:          for (uint256 i = 0; i < _claims.length; i++) {

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317

12. Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

184           require(
185               balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
186               "You have to wait for unlock or have to manually rebalance out of it"
187:          );

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L184-L187

13. Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

45:       bytes32 public constant AURABAL_BALETH_BPT_POOL_ID = 0x3dd0843a028c86e0b760b1a76929d1c5ef93a2dd000200000000000000000249;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L45

File: contracts/MyStrategy.sol   #2

46:       bytes32 public constant BAL_ETH_POOL_ID = 0x5c6ee304399dbdb9c8ef030ab642b10820db8f56000200000000000000000014;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L46

File: contracts/MyStrategy.sol   #3

47:       bytes32 public constant AURA_ETH_POOL_ID = 0xc29562b045d80fd77c69bec09541f5c16fe20d9d000200000000000000000251;

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L47

14. Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 6 instances of this issue:

File: contracts/MyStrategy.sol

184           require(
185               balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
186               "You have to wait for unlock or have to manually rebalance out of it"
187:          );

205:              require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage

290:          require(address(bribesProcessor) != address(0), "Bribes processor not set");

341:          require(beforeVaultBalance == _getBalance(), "Balance can't change");

342:          require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change");

437:          require(isClaimingBribes, "onlyWhileClaiming");

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L184-L187

[GalloDaSballo]

1. Repeatedly changing isClaimingBribes back to false wastes gas

We could also use 1 and 2, agree

2. Modifier should be skipped for sweepRewards()

We could also just call sweeprewards separately for what it's worth

3. Avoid contract existence checks by using solidity version 0.8.10 or later

I fail to see how we would action this

4. Multiple accesses of a mapping/array should use a local variable cache

These are memory values, I don't believe this to be valid

5. internal functions only called once can be inlined to save gas

Agree

6. .length should not be looked up in every loop of a for-loop

Agree

7. require()/revert() strings longer than 32 bytes cost extra gas

Valid but we're fine

8. Using bools for storage incurs overhead

Not particularly useful for Governance Variables

9. Use a more recent version of solidity

Nope

 10. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

MSTORE is 3 gas ser

11. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

++i saves 5 gas compared to i++

12. Splitting require() statements that use && saves gas

Acknowledged

13. Using private rather than public for constants, saves gas

That changes the functionality of the contract

14. Use custom errors rather than revert()/require() strings to save gas

We're on 6.12 ser