QA Report

[code423n4]

Summary of Findings for Low / Non-Critical issues

• L-01 : Inconsistencies in constructor parameters of the contracts

• L-02 : No reset equivalent for function prepareKeeperRequiredStakedBKD

• L-03 : Wrong use of amount in the emit Deposit event in VaultReserve.sol

• L-04 : Upgradability of the core contracts not considered

• NC-01 : modifier onlyPool not used anywhere in Vault contract

• NC-02 : Wrong and misleading dev comment in AddressProvider.sol

Details L-01

Title : Inconsistencies in constructor parameters of the contracts

The constructors for Controller, CvxCrvRewardsLocker, etc., use 'IAddressProvider _addressProvider' as parameter, and other values are obtained from the addressProvider functions.

On the contrary, the constructors for TopUpAction, Vault, LiquidityPool, GasBank, etc., use 'IController _controller' as parameter, and other values like addressProvider are derived from the controller functions.

There is a possibility of maliciously deploying multiple controllers, (though each can use same or different addressProvider), and have multiple sources of truth.

Recommended Mitigation Steps

Consistently use addressProvider as the contract parameter and derive other values from it, since the AddressProvider contract is a single source of truth. This will probibit any misuse or mistakes during deployment of contracts in future.

Details L-02

Title : No reset equivalent for function prepareKeeperRequiredStakedBKD

Impact

The protocol will not be able to reset the values that was prepared for KEEPER_REQUIRED_STAKED_BKD. In case reset is required, it has to be executed and prepared again for correcting any mistake.

Proof of Concept

Contract : Controller.sol Function : prepareKeeperRequiredStakedBKDO() and executeKeeperRequiredStakedBKD()

Recommended Mitigation Steps

Define a new function resetKeeperRequiredStakedBKD, to be consistent with the prepare, reset and execute pattern.

Details L-03

Title : Wrong use of amount in the emit Deposit event in VaultReserve.sol

In the emit event of Deposit() function, instead of using 'amount' , the correct value to use is 'received'

Impact

Possibility of emitting a wrong value in case the value of 'received' is more than 'amount'.

   uint256 received = newBalance - balance;
   require(received >= amount, Error.INVALID_AMOUNT);
   _balances[msg.sender][token] += received;

Proof of Concept

Contract : VaultReserve.sol Function : deposit

line 61 emit Deposit(msg.sender, token, amount);

Recommended Mitigation Steps

Change as below

line 61 emit Deposit(msg.sender, token, received);

Details L-04

Title : Upgradability of the core contracts not considered

Since the protocol is still in development, the features and functions may be needed to be modified/added in say the AddressProvider, Controller, etc., In that eventuality, it may be difficult to upgrade these contracts.

Recommended Mitigation Steps

Use upgradable proxy pattern of deployment for AddressProvider, Controller etc.,

Details NC-01

Title : modifier onlyPool not used anywhere in Vault contract

Proof of Concept

Contract : Vault.sol
modifier onlyPool() { require(msg.sender == pool, Error.UNAUTHORIZEDACCESS); ; }

Recommended Mitigation Steps

Since its a dead code, delete the modifier onlyPool()

Details NC-02

Title : Wrong and misleading dev comment in AddressProvider.sol

Proof of Concept

Contract : AddressProvider.sol Function : addStakerVault()

line 271 /* line 272 @notice Add a new staker vault and add it's lpGauge if set in vault.

There is no lpGause set in this contract, but its set in Controller.sol for the same function.

Recommended Mitigation Steps

Move the @notice comment to against the function addStakerVault in Controller.sol contract where its relevant.

[chase-manning]

I consider this report to be of particularly high quality