Badger-Vested-Aura contest - Funen's results

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

1. Title : Remove function for saving gas

When tend with no-op it could be this function below just doesn't do anything. It can be deleted instead for saving more gas

    // Example tend is a no-op which returns the values, could also just revert
    function _tend() internal override returns (TokenAmount[] memory tended) {
        revert("no op");
    }

2. Title : : Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

Tool Used

Manual Review

Occurances

contracts/MyStrategy.sol#L186            "You have to wait for unlock or have to manually rebalance out of it"