QA Report

[code423n4]

1. Title : Needed to be checked Claim auraBAL from locker

Since if there was no false in it. It would be better to adding false for "Reverts if no locks expired" so in this case down below :

File : MyStrategy.sol Line. 223

        LOCKER.getReward(address(this));

can be changed to :

        LOCKER.getReward(address(this), false);

Tool Used

Manual Review

2. Title : Missing Event on harvest() in MyStrategy.sol

Harvest event that every strategy MUST have. Since Event is an inheritable member of a contract. An event is emitted, it stores the arguments passed in transaction logs. These logs are stored on blockchain and are accessible using address of the contract till the contract is present on the blockchain.

File : MyStrategy.sol Line. 219

3. Title : require() should be used instead of assert()

The require function should be used to check return values from calls to external contracts or to guarantee that valid conditions, such as inputs or contract state variables, are satisfied.

1.) File : MyStrategy.sol Line. 57

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

4. Title : Simplify return

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

On this case it can be shorter by just call return protected; instead of return protectedTokens; .

POC

https://github.com/sambacha/yearn-strategy-boilerplate-nodejs/blob/master/contracts/Strategy.sol#L156

Tool Used

Manual Review

5. Title : OPEN TODOs

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

1.) File : MyStrategy.sol Line. 85

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

2.) File : MyStrategy.sol Line. 91

     ///@dev Should we processExpiredLocks during reinvest?

3.) File : MyStrategy.sol Line. 135

/// @dev Does this function require `tend` to be called?

4.) File : MyStrategy.sol Line. 284

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

6. Title : Numbers can be set as Constants

File : MyStrategy.sol Line.205

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

7. Title : Consider make the constracts pausable

There are many external risks so the suggestion was it should be consider making the contracts pausable, so if in the case of an unexpected event, the admin can pause transfers.

Tool Used

Manual Review

##POC
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

Recommended Mitigation Steps

Consider making contracts Pausable

[GalloDaSballo]

Title : Needed to be checked Claim auraBAL from locker

Not sure why unlocking and claiming rewards should mix

Title : Missing Event on harvest() in MyStrategy.sol

Disagree, we moved the event to the Vault: https://github.com/Badger-Finance/badger-vaults-1.5/blob/5abb584f93d564dea039a6b6a00a0cca11dd2b42/contracts/Vault.sol#L126

Title : require() should be used instead of assert()

Disagree it's the one case where it's fine

Title : Simplify return

Changing the name of the variable?
Ah ok the Warden thinks we're using Yearn V2 code, this is incorrect per the code provided in the Context Readme

Rest i pretty much disagree