Backd contest - hake's results

LOW

L-01: If any constant address in CvxCrvRewardsLocker changes, they will not be able to be updated.

ConvexStrategyBase.sol

If any constant address in CvxCrvRewardsLocker changes, they will not be able to be updated.

Also valid for(list not exhaustive): -CTokenRegistry -ConvexStrategyBase -StrategySwaper

L-02: No zero address checks

CvxCrvRewardsLocker.setTreasury; L147-L154

Accidentally setting treasury to address zero may lead to losing all funds when withdrawing.

TopUpActionFeeHandler.actionContract#L55

If actionContract is accidentally set to zero the payFees function would become inaccessible because there is no other way to set actionContract.

• Recommendation Implement zero address check

L-03: safeApprove from OpenZeppelin has been deprecated

safeApprove from OpenZeppelin has been deprecated. Instead it is recommended to use increase/decrease allowance.

Please refer to SafeERC20.sol#L38

NON-CRITICAL

N-01: Confusing function naming

CvxCrvRewardsLocker.withdraw#L156-L164 CvxCrvRewardsLocker.withdraw#L217-L225

Suggest changing withdraw#L156-L164 to fullWithdraw to avoid confusion and improve readability.

No-02: Minor typo in CompoundHandler

CompoundHandler#L46 Change tup-up to top-up.

N-04: ERC777 Reentrancy in StakerVault.unstakeFor could bypass allowance.

StakerVault.sol#L360

Hook in ERC777 reenters unstakeFor and bypasses _allowances update which is only calculated after safeTransfer.

Beaware that if LpToken ever uses the ERC777 standard this could be an attack vector.

GAS

G-01: Unnecessary if statement in processExpiredLocks

CvxCrvRewardsLocker.processExpiredLocks; L133-L145

Gas could be saved by changing:

    function processExpiredLocks(bool relock) external override returns (bool) {
        if (relock) {
            require(!prepareWithdrawal, Error.PREPARED_WITHDRAWAL);
        }

        if (relock) {
            ICvxLocker(CVX_LOCKER).processExpiredLocks(relock);
        } else {
            ICvxLocker(CVX_LOCKER).withdrawExpiredLocksTo(treasury);
        }

        return true;
    }

To:

    function processExpiredLocks(bool relock) external override returns (bool) {
        if (relock) {
            require(!prepareWithdrawal, Error.PREPARED_WITHDRAWAL);
            ICvxLocker(CVX_LOCKER).processExpiredLocks(relock);
        } else {
            ICvxLocker(CVX_LOCKER).withdrawExpiredLocksTo(treasury);
        }

        return true;
    }

G-02: Changing parameters order could save some gas

TopUpActionFeeHandler.payFees#L81-L109

If lpToken.safeTransferFrom reverts keeperAmount and treasuryAmount will be unnecessarily calculated.

uint256 keeperAmount = amount.scaledMul(getKeeperFeeFraction());
uint256 treasuryAmount = amount.scaledMul(getTreasuryFeeFraction());
LpToken lpToken = LpToken(lpTokenAddress);

lpToken.safeTransferFrom(msg.sender, address(this), amount);

I suggest setting keeperAmount and treasuryAmount after lpToken.safeTransferFrom as seen below.

LpToken lpToken = LpToken(lpTokenAddress);

lpToken.safeTransferFrom(msg.sender, address(this), amount);

uint256 keeperAmount = amount.scaledMul(getKeeperFeeFraction());
uint256 treasuryAmount = amount.scaledMul(getTreasuryFeeFraction());

Ga-03: For loop in hasAnyRole can be more gas efficient

RoleManager.hasAnyRole#L73-L86

• It is unnecessary to initialize i = 0 as zero is already the default value.
• Caching roles.length would save gas from calculating it every iteration
• Using a prefix ++i is more effecient than using a postfix i++

In summary I suggest changing it from:

for (uint256 i = 0; i < roles.length; i++) {
    if (hasRole(roles[i], account)) {
        return true;
    }
}

To:

size = roles.length;
for (uint256 i; i < size; ++i) {
    if (hasRole(roles[i], account)) {
        return true;
    }
}

Also valid for(list not exhaustive):

• CompoundHandler._getAccountBorrowsAndSupply#L135 -TopUpKeeperHelper#L32 StakerVault - getstakedbyactions