QA Report

[code423n4]

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Constants instead of magic numbers
• Events indexing
• Event should be emitted in setters
• Function missing comments
• Public functions can be external
• TODOs left in the contract
• Uint256 alias
• Unused function parameter
• Assert should not be used
• safeApprove() is deprecated
• Setters should check the input value

summary

Few vulnerabilities were found examining the contract.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing natspec function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

address _vault
address delegate
bool newWithdrawalSafetyCheck
bool newProcessLocksOnReinvest
address token
address[] calldata tokens
uint256 _amount
uint256 _amount
IRewardDistributor hiddenHandDistributor, IRewardDistributor.Claim[] calldata _claims
bytes calldata performData
address token, uint256 amount
address token, uint256 amount
uint256 amount

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

9_980

TOOLS USED

Manual Analysis

MITIGATION

Define a constant variable for it.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

event RewardsCollected(address token, uint256 amount)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to this event.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

function manualSetDelegate
function setWithdrawalSafetyCheck
function setProcessLocksOnReinvest
function setBribesProcessor

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

function checkUpkeep

TOOLS USED

Manual Analysis

MITIGATION

Add comments to this function

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

function initialize()
function getProtectedTokens()
function manualProcessExpiredLocks()

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

TODOs left in the contract

IMPACT

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

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

TODO: Hardcode claim.account = address(this)?
TODO: Too many SLOADs

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs/Resolve related issues.

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

uint i = 0

TOOLS USED

Manual Analysis

MITIGATION

replace uint with
uint256

Unused function parameter

IMPACT

Unused function parameters should be removed

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

bytes calldata performData

TOOLS USED

Manual Analysis

MITIGATION

remove this function parameter

Assert should not be used

IMPACT

Properly functioning code should never reach a failing assert statement. If it happened, it would indicate the presence of a bug in the contract. A failing assert uses all the remaining gas. In this case it is the initialize function and would not impact users, but it would still be wasting the team's funds if something were to go wrong.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Replace the assert statement with a require statement or a custom error

safeApprove() is deprecated

PROBLEM

This function is deprecated.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

AURA.safeApprove(address(LOCKER), type(uint256).max)
AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max)
WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max)

TOOLS USED

Manual Analysis

MITIGATION

safeIncreaseAllowance() and safeDecreaseAllowance() should be used instead.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

function manualSetDelegate(address delegate)
function setBribesProcessor(IBribesProcessor newBribesProcessor)

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero address checks to delegate and newBribesProcessor.

[GalloDaSballo]

I really dislike this format