QA Report

[code423n4]

Summary

Low Risk Issues

	Issue	Instances
1	require() should be used instead of assert()	1
2	safeApprove() is deprecated	3
3	Open TODOs	2
4	Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions	1

Total: 7 instances over 4 issues

Non-critical Issues

	Issue	Instances
1	Using vulnerable version of OpenZeppelin	1
2	Missing initializer modifier on constructor	1
3	public functions not called by the contract should be declared external instead	1
4	constants should be defined rather than using magic numbers	1
5	Redundant cast	1
6	Missing event and timelock for critical parameter change	3
7	Use a more recent version of solidity	1
8	Inconsistent spacing in comments	4
9	Typos	4
10	Event is missing indexed fields	1

Total: 18 instances over 10 issues

Low Risk Issues

1. require() should be used instead of assert()

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

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

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

2. safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

65:           AURA.safeApprove(address(LOCKER), type(uint256).max);

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

File: contracts/MyStrategy.sol   #2

67:           AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);

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

File: contracts/MyStrategy.sol   #3

68:           WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

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

3. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There are 2 instances of this issue:

File: contracts/MyStrategy.sol   #1

284:      // TODO: Hardcode claim.account = address(this)?

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

File: contracts/MyStrategy.sol   #2

422:          // TODO: Too many SLOADs

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

4. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

20:   contract MyStrategy is BaseStrategy, ReentrancyGuardUpgradeable {

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

Non-critical Issues

1. Using vulnerable version of OpenZeppelin

The brownie configuration file says that the project is using 3.4.0 of OpenZeppelin which has a vulnerability in initializers that call external contracts, which this code does. You're protecting against it by having the comment stating to change all state at the end, but it would be better to upgrade and use the onlyInitializing modifier

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

55:      /// @dev add any extra changeable variable at end of initializer as shown

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

2. Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

20:   contract MyStrategy is BaseStrategy, ReentrancyGuardUpgradeable {

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

3. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

56:       function initialize(address _vault) public initializer {

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

4. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

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

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

5. Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

/// @audit address(BADGER)
430:          _processExtraToken(address(BADGER), amount);

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

6. Missing event and timelock for critical parameter change

Events help non-contract tools to track changes, and events prevent users from being surprised by changes

There are 3 instances of this issue:

File: contracts/MyStrategy.sol   #1

86        function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external {
87            _onlyGovernance();
88            withdrawalSafetyCheck = newWithdrawalSafetyCheck;
89:       }

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

File: contracts/MyStrategy.sol   #2

92        function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external {
93            _onlyGovernance();
94            processLocksOnReinvest = newProcessLocksOnReinvest;
95:       }

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

File: contracts/MyStrategy.sol   #3

98        function setBribesProcessor(IBribesProcessor newBribesProcessor) external {
99            _onlyGovernance();
100           bribesProcessor = newBribesProcessor;
101:      }

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

7. Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

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

8. Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file

There are 4 instances of this issue:

File: contracts/MyStrategy.sol   #1

85:       ///@dev Should we check if the amount requested is more than what we can return on withdrawal?

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

File: contracts/MyStrategy.sol   #2

91:       ///@dev Should we processExpiredLocks during reinvest?

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

File: contracts/MyStrategy.sol   #3

97:        ///@dev Change the contract that handles bribes

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

File: contracts/MyStrategy.sol   #4

183:          //NOTE: This probably will always fail unless we have all tokens expired

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

9. Typos

There are 4 instances of this issue:

File: contracts/MyStrategy.sol   #1

/// @audit hardcoded
105:      /// @notice because token paths are hardcoded, this function is safe to be called by anyone

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

File: contracts/MyStrategy.sol   #2

/// @audit sweeped
160:      /// @notice this provides security guarantees to the depositors they can't be sweeped away

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

File: contracts/MyStrategy.sol   #3

/// @audit compunded
218:      ///      after claiming rewards or swapping are auto-compunded.

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

File: contracts/MyStrategy.sol   #4

/// @audit Hardcode
284:      // TODO: Hardcode claim.account = address(this)?

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

10. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There is 1 instance of this issue:

File: contracts/MyStrategy.sol   #1

51:       event RewardsCollected(address token, uint256 amount);

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

[GalloDaSballo]

1. require() should be used instead of assert()

Disagree for this specific case, assert is fine, let the caller regret setting the wrong vault

2. safeApprove() is deprecated

Acknowledge, however we are using safeApprove the proper way, from 0, once

3. Open TODOs

Ack

4. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Disagree, we have it in the baseStrategy: https://github.com/Badger-Finance/badger-vaults-1.5/blob/5abb584f93d564dea039a6b6a00a0cca11dd2b42/contracts/BaseStrategy.sol#L431

1. Using vulnerable version of OpenZeppelin

Ack, not exploited hence no prob

2. Missing initializer modifier on constructor

We use initialized ser, check the bot

3. public functions not called by the contract should be declared external instead

Disagree, we need it for the deployment

4. constants should be defined rather than using magic numbers

Ack

5. Redundant cast

I believe it get's optimized away

6. Missing event and timelock for critical parameter change

Personally disagree

7. Use a more recent version of solidity

We like the version

8. Inconsistent spacing in comments

k

9. Typos

k

10. Event is missing indexed fields

Good idea

[IllIllI000]

@GalloDaSballo Ser, there is no constructor defined in this contract therefore the default one is used, where the initializer modified is not being used

[GalloDaSballo]

@IllIllI000 I've looked into it and had I agree with the finding, would recommend rephrasing to:
Implementation contract may not be initialized. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits.

Personally our UpgradeableProxy doesn't risk being self-destructed, that said if the finding is contextualized in that way I agree with it.

In terms of the Proxy, we deploy + initialize in the same transaction via constructor(admin, logic, data) or similar, meaning initialization will not be front-run on the side of the proxy

[GalloDaSballo]

I have changed my mind about assert we should have used require and we have changed the code

[jack-the-pug]

The following two Low severity findings should be Non-critical given the low impact:

• L-3: safeApprove() is deprecated
• L-3: Open TODOs

Agreed with the severities of the other findings.

Overall, this is an excellent QA report with top-notch formatting. I love how you put a short and clear description for each issue, with all the instances listed.