Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #105

Open
code423n4 opened this issue Jun 3, 2022 · 2 comments
Open

QA Report #105

code423n4 opened this issue Jun 3, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

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);
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@chase-manning chase-manning added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

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
Copy link
Collaborator

Overall pretty low quality submission, formatting is pretty good though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants