QA Report

[code423n4]

QA Report

Table of Contents

• Summary
• Typos
• Comment Missing function parameter
• Function missing comments
• Related data should be grouped in struct
• Event should be emitted in setters
• Unchecked inputs

summary

Few vulnerabilities were found examining the contracts. The main concerns are with unchecked inputs in setter functions.

Typos

PROBLEM

There are a few typos in some comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CompoundHandler.sol

CompoundHandler.sol:90: bytes memory//no parameter is declared

Preparable.sol

Preparable.sol:148: _setConfig is declared twice

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AaveHandler.sol

AaveHandler.sol:26: topUp() //bytes memory extra 

CompoundHandler.sol

CompoundHandler.sol:26: topUp() //bytes memory extra 

CompoundHandler.sol:109: topUp() //CToken ctoken

TopUpAction.sol

TopUpAction.sol:203: register() //return value missing comment
TopUpAction.sol:442: addUsableToken() //return value missing comment

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AaveHandler.sol

AaveHandler.sol:69: getUserFactor()

CompoundHandler.sol

CompoundHandler.sol:128: _getAccountBorrowsAndSupply()

CTokenRegistry.sol

CTokenRegistry.sol:128: _isCTokenUsable()

TopUpAction.sol

TopUpAction.sol:36: lockFunds()
TopUpAction.sol:91: getSwapper()
TopUpAction.sol:180: initialize()
TopUpAction.sol:821: _updateTopUpHandler()
TopUpAction.sol:871: _removePosition()
TopUpAction.sol:884: _removeUserPosition()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to all functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BkdLocker.sol

BkdLocker.sol:26:mapping(address => uint256) public balances;
BkdLocker.sol:27:mapping(address => uint256) public boostFactors;
BkdLocker.sol:28:mapping(address => uint256) public lastUpdated;
BkdLocker.sol:29:mapping(address => WithdrawStash[]) public stashedGovTokens;
BkdLocker.sol:30:mapping(address => uint256) public totalStashed;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, the mitigation could be:

struct UserData {
  uint256 balances;
  uint256 boostFactors;
  uint256 lastUpdated;
  WithdrawStash[] stashedGovTokens;
  uint256 totalStashed;
}

And it would be used as a state variable:

mapping(address =>  UserData) userData;

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:

ChainlinkOracleProvider.sol

ChainlinkOracleProvider.sol:40: setStalePriceDelay()

Controller.sol

Controller.sol:31: setInflationManager()

CvxCrvRewardsLocker.sol

CvxCrvRewardsLocker.sol:119: setWithdrawalFlag()

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Unchecked inputs

PROBLEM

There should be a non-zero (address or integer) check in all setters

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ConvexStrategyBase.sol

ConvexStrategyBase.sol:181: setCommunityReserve()
ConvexStrategyBase.sol:227: setImbalanceToleranceIn()
ConvexStrategyBase.sol:243: setImbalanceToleranceOut()

CvxCrvRewardsLocker.sol

CvxCrvRewardsLocker.sol:150: setTreasury()
CvxCrvRewardsLocker.sol:189: setDelegate()

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks.