Badger-Vested-Aura contest - joestakey's results

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.

Gas Report

Table of Contents

• Comparison operators
• Custom errors
• Default value initialization
• Event emitting of local variable
• Inline functions
• Prefix increments
• Require instead of AND
• Revert strings length
• Tight variable packing
• Use a more recent version of Solidity

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas, approximately 20 gas per comparison.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

max >= _amount.mul(9_980).div(MAX_BPS)

TOOLS USED

Manual Analysis

MITIGATION

Replace >= with >. In this case 1 is negligible compared to the value compared, we can omit the increment.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement.

It not only saves gas upon deployment - ~5500 gas saved per custom error instead of a require statement, but it is also cheaper in a function call, 22 gas saved per require statement replaced with a custom error.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

• require(balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,"You have to wait for unlock or have to manually rebalance out of it")
• require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check")
• require(address(bribesProcessor) != address(0), "Bribes processor not set")
• require(beforeVaultBalance == _getBalance(), "Balance can't change")
• require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change")
• require(isClaimingBribes, "onlyWhileClaiming")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance:

Replace

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

with

if (beforeVaultBalance != _getBalance()) {
		revert ChangedBalance();
}

and define the custom error in the contract

error ChangedBalance();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes 22 gas per variable initialized.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

uint i = 0
uint i = 0
uint i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Inline functions

PROBLEM

When we define internal functions to perform computation:

• The contract’s code size gets bigger
• the function call consumes more gas than executing it as an inlined function (part of the code, without the function call)

When it does not affect readability, it is recommended to inline functions in order to save gas. It saves ~3,000 gas per inlined function upon deployment, and 30 gas per function call.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

function _deposit(uint256 _amount)
function _getBalance()
function _getPricePerFullShare())
function _notifyBribesProcessor()

TOOLS USED

Manual Analysis

MITIGATION

Inline these functions where they are called

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments: it returns the incremented variable instead of returning a temporary variable storing the initial value of the variable. It saves 5 gas per iteration

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

i++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas. It saves 50 gas per require statement broken down.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

-require(balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,"You have to wait for unlock or have to manually rebalance out of it");
+require(balanceOfPool() == 0) 
+require(LOCKER.balanceOf(address(this)) == 0);

You can also improve gas savings by using custom errors

Revert strings length

IMPACT

Revert strings larger than 32-bytes will incur an extra cost upon deployment. It costs ~9,500 extra gas per extra 32-byte upon deployment.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

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

• This string has 67 bytes.

TOOLS USED

Manual Analysis

MITIGATION

Reduce the string message to something smaller than 32-byte to save approximately 19,000 gas upon deployment

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool type variables are of size 1 byte, there's a slot here that can get saved by moving one address closer to a bool. This saves a gsset - 20,000 gas.

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

bool public withdrawalSafetyCheck;
// If nothing is unlocked, processExpiredLocks will revert
bool public processLocksOnReinvest; 

bool private isClaimingBribes;

IBribesProcessor public bribesProcessor;

IBalancerVault public constant BALANCER_VAULT = IBalancerVault(0xBA12222222228d8Ba445958a75a0704d566BF2C8);

address public constant BADGER = 0x3472A5A71965499acd81997a54BBA8D852C6E53d;
address public constant BADGER_TREE = 0x660802Fc641b154aBA66a62137e71f331B6d787A

TOOLS USED

Manual Analysis

MITIGATION

Place BADGER before withdrawalSafetyCheck to save one storage slot

+address public constant BADGER = 0x3472A5A71965499acd81997a54BBA8D852C6E53d;
bool public withdrawalSafetyCheck;
// If nothing is unlocked, processExpiredLocks will revert
bool public processLocksOnReinvest; 

bool private isClaimingBribes;

IBribesProcessor public bribesProcessor;

IBalancerVault public constant BALANCER_VAULT = IBalancerVault(0xBA12222222228d8Ba445958a75a0704d566BF2C8);

address public constant BADGER_TREE = 0x660802Fc641b154aBA66a62137e71f331B6d787A

Use a more recent version of Solidity

PROBLEM

Use a more recent version of solidity. The advantages here are: built-in overflow/underflow checks (>= 0.8.0) Low level inliner (>= 0.8.2) Cheaper runtime gas (especially relevant when the contract has small functions) Optimizer improvements in packed structs (>= 0.8.3) custom errors (>= 0.8.4), external calls skipping contract existence (>= 0.8.10).

PROOF OF CONCEPT

Instances include:

MyStrategy.sol

pragma solidity 0.6.12

TOOLS USED

Manual Analysis

MITIGATION

Update the solidity version.