QA Report

[code423n4]

Table of Contents

• Non-Critical Findings
  • [NC-01] Unnecessary inequality
• Low Risk
  • [L-01] Zero-address checks are missing
  • [L-02] Events not emitted for important state changes
  • [L-03] Open @TODO left in the code
  • [L-04] - Restrict ETH funds receivable by contracts
  • [L-05] Possibly wrong amount value in AmmGauge staking/unstaking event

Non-Critical Findings

[NC-01] Unnecessary inequality

Description

The used inequality amount <= 0 check is not necessary as the amount variable is of type uint256 (unsigned integer, representing only positive integers) and can not host negative numbers.

Findings

tokenomics/LpGauge.sol#L59

if (amount <= 0) return 0;

tokenomics/AmmGauge.sol#L63

if (amount <= 0) return 0;

Recommended mitigation steps

Consider changing it to:

if (amount == 0) return 0;

Low Risk

[L-01] 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 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

BkdLocker.sol

L49: rewardToken = _rewardToken;\ L50: govToken = IERC20(_govToken);

tokenomics/AmmGauge.sol

L39: ammToken = _ammToken;

tokenomics/BkdToken.sol

L21: minter = _minter;

tokenomics/FeeBurner.sol

L32: _addressProvider = IAddressProvider(addressProvider_);

tokenomics/KeeperGauge.sol

L48: pool = _pool;

tokenomics/VestedEscrow.sol

L60: rewardToken = IERC20(rewardToken_);\ L65: fundAdmin = fundAdmin_;

tokenomics/VestedEscrowRevocable.sol

L43: treasury = treasury_;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_rewardToken != address(0), "Zero-address");

[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

KeeperGauge.claimRewards - function claimRewards(address beneficiary, uint256 endEpoch)

LpGauge.claimRewards - function claimRewards(address beneficiary)

Recommended mitigation steps

Emit events for state variable changes.

[L-03] Open @TODO left in the code

Description

There is an open TODO left in the code (and misspelled :D)

Findings

tokenomics/InflationManager.sol#L532

//TOOD: See if this is still needed somewhere

Recommended mitigation steps

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

[L-04] - Restrict ETH funds receivable by contracts

Description

Native ETH fund transfers to contracts should be, if possible, limited to a limited set of contracts to prevent accidental native fund transfers from other sources.

Findings

FeeBurner.sol#L35 - Restrict ETH funds receivable by FeeBurner to be only from Backd ETH Pool

RewardHandler.sol#L30 - Restrict ETH funds receivable by RewardHandler to be only from EthVault

Recommended mitigation steps

Modify the receive() and fallback functions to only accept transfers from the expected contracts. For instance:

receive() external payable {
  require(msg.sender == wethAddress, 'Only WETH');
}

fallback() external payable {
  require(msg.sender == wethAddress, 'Only WETH');
}

[L-05] Possibly wrong amount value in AmmGauge staking/unstaking event

Description

Both staking and unstaking in the AmmGauge contract emit events. The third event value amount is incorrectly used and should be replace with the actual staked/unstaked amount (for instance, for fee-on transfer tokens the value could be different than the supplied function parameter amount).

Findings

tokenomics/AmmGauge.sol#L114

emit AmmStaked(account, ammToken, amount); // @audit-info use `staked` as the last event value

tokenomics/AmmGauge.sol#L136

emit AmmUnstaked(msg.sender, ammToken, amount); // @audit-info use `unstaked` as the last event value

Recommended mitigation steps

Use the correct staked/unstaked value as the event value.

[GalloDaSballo]

[NC-01] Unnecessary inequality

Agree

[L-01] Zero-address checks are missing

Agree

[L-02] Events not emitted for important state changes

Non-Critical

[L-03] Open @TODO left in the code

Non-Critical

[L-04] - Restrict ETH funds receivable by contracts

Disagree as dogma without explanation is not useful

[L-05] Possibly wrong amount value in AmmGauge staking/unstaking event

Valid, but non-critical / informational as it's for events

Overall a neat report, most findings are non-critical though