QA Report

[code423n4]

QA Report

Non-Critical Issues

safeApprove() is deprecated

With reference to SafeERC20.sol, safeApprove() is deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().

Consider using these functions instead of safeApprove() in these instances:

protocol/contracts/RewardHandler.sol:
  52:        IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount);
  64:        IERC20(token).safeApprove(spender, type(uint256).max);

protocol/contracts/tokenomics/AmmConvexGauge.sol:
  61:        IERC20(ammToken).safeApprove(booster, type(uint256).max);

protocol/contracts/tokenomics/FeeBurner.sol:
 118:        IERC20(token_).safeApprove(spender_, type(uint256).max);

protocol/contracts/zaps/PoolMigrationZap.sol:
  27:        IERC20(underlying_).safeApprove(address(newPool_), type(uint256).max);

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

protocol/contracts/BkdLocker.sol:
  72:        _replacedRewardTokens.set(rewardToken, block.timestamp);
  73:        lastMigrationEvent = block.timestamp;
 125:        WithdrawStash(block.timestamp + currentUInts256[_WITHDRAW_DELAY], amount)
 141:        if (stashedWithdraws[i].releaseTime <= block.timestamp) {
 276:        newBoost += (block.timestamp - lastUpdated[user])
 333:        lastUpdated[user] = block.timestamp;

protocol/contracts/tokenomics/AmmGauge.sol:
  41:        ammLastUpdated = uint48(block.timestamp);
  90:        (block.timestamp - uint256(ammLastUpdated))).scaledDiv(totalStaked);
 146:        uint256 timeElapsed = block.timestamp - uint256(ammLastUpdated);
 150:        ammLastUpdated = uint48(block.timestamp);

protocol/contracts/tokenomics/AmmConvexGauge.sol:
 100:        uint256 timeElapsed = block.timestamp - uint256(ammLastUpdated);
 121:        uint256 timeElapsed = block.timestamp - uint256(ammLastUpdated);
 188:        uint256 timeElapsed = block.timestamp - uint256(ammLastUpdated);
 208:        ammLastUpdated = uint48(block.timestamp);

protocol/contracts/tokenomics/Minter.sol:
 106:        lastEvent = block.timestamp;
 107:        lastInflationDecay = block.timestamp;
 188:        totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
 189:        lastEvent = block.timestamp;
 190:        if (block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD) {
 212:        lastInflationDecay = block.timestamp;
 218:        totalAvailableToNow += ((block.timestamp - lastEvent) * currentTotalInflation);
 222:        lastEvent = block.timestamp;

protocol/contracts/tokenomics/VestedEscrow.sol:
  57:        require(starttime_ >= block.timestamp, "start must be future");
 114:        _claimUntil(msg.sender, block.timestamp);
 126:        return _totalVestedOf(_recipient, block.timestamp);
 130:        return _balanceOf(_recipient, block.timestamp);
 134:        uint256 vested = _totalVestedOf(_recipient, block.timestamp);
 139:        _claimUntil(_recipient, block.timestamp);
 164:        return _computeVestedAmount(initialLockedSupply, block.timestamp);

protocol/contracts/tokenomics/LpGauge.sol:
  70:        (block.timestamp - poolLastUpdate)).scaledDiv(poolTotalStaked);
 115:        poolStakedIntegral += (currentRate * (block.timestamp - poolLastUpdate)).scaledDiv(
 119:        poolLastUpdate = block.timestamp;

protocol/contracts/tokenomics/VestedEscrowRevocable.sol:
  55:        revokedTime[_recipient] = block.timestamp;
  56:        uint256 vested = _totalVestedOf(_recipient, block.timestamp);
  74:        return _totalVestedOf(_recipient, block.timestamp) - _vestedBefore;
  81:        return _totalVestedOf(_recipient, block.timestamp);
  85:        uint256 timestamp = block.timestamp;
  97:        uint256 vested = _totalVestedOf(_recipient, block.timestamp);
 102:        uint256 timestamp = block.timestamp;

protocol/contracts/tokenomics/KeeperGauge.sol:
  49:        lastUpdated = uint48(block.timestamp);
 112:        uint256 timeElapsed = block.timestamp - uint256(lastUpdated);
 115:        lastUpdated = uint48(block.timestamp);

protocol/contracts/utils/Preparable.sol:
  30:        deadlines[key] = block.timestamp + delay;
 110:        require(block.timestamp >= deadline, Error.DEADLINE_NOT_REACHED);

Use modifiers instead of require statements for access roles

Instead of using a require statement to check that msg.sender belongs to a certain role (e.g. msg.sender is owner), consider using modifiers. This would help improve code clarity and prevent accidental mistakes in future code.

For example, to check that msg.sender is owner, a modifier can be written as such:

modifier isOwner() {
   require(msg.sender == owner, "error");
   _;
}

Functions can then use isOwner to validate msg.sender, for example:

function setOwner(address _owner) external {
   require(msg.sender == owner, "error");
   // ...
}

can be rewritten to:

function setOwner(address _owner) external isOwner {
   // ...
}

Other instances of this include:

protocol/contracts/StakerVault.sol:
  99:        require(msg.sender == address(inflationManager), Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/BkdToken.sol:
  31:        require(msg.sender == minter, Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/AmmGauge.sol:
  50:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/Minter.sol:
 132:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/VestedEscrow.sol:
  70:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
  76:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
  81:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
  90:        require(msg.sender == fundAdmin || msg.sender == admin, Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/VestedEscrowRevocable.sol:
  52:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);

protocol/contracts/tokenomics/KeeperGauge.sol:
  40:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

protocol/contracts/tokenomics/AmmConvexGauge.sol:
  38:        event RewardClaimed(
  39:                address indexed beneficiary,
  40:                uint256 bkdAmount,
  41:                uint256 crvAmount,
  42:                uint256 cvxAmount
  43:            );

protocol/contracts/zaps/PoolMigrationZap.sol:
  18:        event Migrated(address user, address oldPool, address newPool, uint256 lpTokenAmount);

protocol/interfaces/vendor/ICvxLocker.sol:
  43:        event Staked(
  44:                address indexed _user,
  45:                uint256 indexed _epoch,
  46:                uint256 _paidAmount,
  47:                uint256 _lockedAmount,
  48:                uint256 _boostedAmount
  49:            );

  51:        event Withdrawn(address indexed _user, uint256 _amount, bool _relocked);
  52:        event KickReward(address indexed _user, address indexed _kicked, uint256 _reward);
  53:        event RewardPaid(address indexed _user, address indexed _rewardsToken, uint256 _reward);

[GalloDaSballo]

Use of block.timestamp

Check your bot

Use modifiers instead of require statements for access roles

More of a gas report

event is missing indexed fields

Disagree with the generalized take

[GalloDaSballo]

Overall pretty low quality submission, formatting is pretty good though