QA Report

[code423n4]

QA Report

Table of Contents

• Non-Critical Findings
  • NC-01: Events not indexed
• Low Risk
  • L-01: initialize functions can be frontrun
  • L-02: Events not emitted for important state changes
  • L-03: Deposit event not emitted if ETH is deposited
  • L-04: Zero-address checks are missing
  • L-05: Open @TODOs left in the code
  • L-06: Possible divide by zero

Non-Critical Findings

NC-01: Events not indexed

Description

Indexed event parameters are stored in the topics part of the log instead of the data part, which allows for faster indexing/querying because of the use of bloom filters for topics. Up to three parameters in every event can be indexed. While this costs a little extra gas, doing so allows for faster and more efficient/effective event lookups.

Findings

strategies/ConvexStrategyBase.sol#L65\ strategies/ConvexStrategyBase.sol#L70\ strategies/ConvexStrategyBase.sol#L71\ strategies/ConvexStrategyBase.sol#L72\ strategies/StrategySwapper.sol#L35\ strategies/StrategySwapper.sol#L36\ SwapperRegistry.sol#L17\ SwapperRegistry.sol#L18\ SwapperRegistry.sol#L19\ CvxCrvRewardsLocker.sol#L47\ actions/topup/TopUpActionFeeHandler.sol#L37\ tokenomics/Minter.sol#L57\ pool/PoolFactory.sol#L75\ pool/PoolFactory.sol#L76

Recommended mitigation steps

Add indexed parameter especially for address parameters where their faster lookup for security monitoring issues can be a good trade-off for the extra gas consumed.

Low Risk

L-01: initialize functions can be frontrun

Description

The initialize function used to initialize important contract state can be called by anyone.

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract. In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Findings

StakerVault.sol#L65\ AddressProvider.sol#L53\ LpToken.sol#L33\ vault/Erc20Vault.sol#L17\ vault/EthVault.sol#L20

Recommended mitigation steps

Use the constructor to initialize non-proxied contracts.

For initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.

L-02: Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

oracles/ChainlinkOracleProvider.sol#L42

Recommended mitigation steps

Emit events for state variable changes.

L-03: Deposit event not emitted if ETH is deposited

Description

A Deposit event is emitted if a token is deposited but if ETH (token == address(0)) is deposited, no event is emitted.

Findings

vault/VaultReserve.sol#L52

function deposit(address token, uint256 amount)
    external
    payable
    override
    onlyVault
    returns (bool)
{
    if (token == address(0)) {
        require(msg.value == amount, Error.INVALID_AMOUNT);
        _balances[msg.sender][token] += msg.value; // @audit-info missing event `Deposit`
        return true;
    }
    uint256 balance = IERC20(token).balanceOf(address(this));
    IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
    uint256 newBalance = IERC20(token).balanceOf(address(this));
    uint256 received = newBalance - balance;
    require(received >= amount, Error.INVALID_AMOUNT);
    _balances[msg.sender][token] += received;
    emit Deposit(msg.sender, token, amount);
    return true;
}

Recommended mitigation steps

Emit Deposit event for ETH deposits.

L-04: Zero-address checks are missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

vault/Vault.sol#L92

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_pool != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);

L-05: Open @TODOs left in the code

Description

There are several open TODOs left in the code.

Findings

strategies/ConvexStrategyBase.sol#L4\ strategies/ConvexStrategyBase.sol#L5\ actions/topup/TopUpAction.sol#L713

Recommended mitigation steps

Check, fix and remove the todos before it is deployed in production

L-06: Possible divide by zero

Description

The function getPriceUSD() in oracles/ChainlinkOracleProvider.sol allows the price of an asset to be 0 due to following check:

require(answer >= 0, Error.NEGATIVE_PRICE);

Therefore, wherever getPriceUSD() is used as the divisor, in certain cases it could lead to a divide by zero error reverting the transaction.

Findings

oracles/OracleProviderExtensions.sol#L15\ oracles/ChainlinkOracleProvider.sol#L47

Recommended mitigation steps

Consider only allowing the price to be > 0 in getPriceUSD():

require(answer > 0, Error.NEGATIVE_PRICE);

or check the divisor before using.