Backd contest - securerodd's results

QA Report

Low Risks

1. Approve in Stake token is susceptible to front-running

The approve(address, uint256) function in StakerVault.sol does not validate that the previous allowance was already used up before updating the allowance. If a user uses the approve function instead of decreaseAllowance to lower their allowance, they would be subject to a double withdrawal attack (through frontrunning).

Scenario:

If User A has an allowance of 10 stake tokens for User B and they use the `approve(address spender, uint256 amount)` function to change the amount to 5 stake tokens, user B could frontrun that approve transaction to perform a `transferFrom(address src, address dst, uint256 amount)` while the allowance is still 10, setting the allowance to 0. Now when user A’s transaction is processed, this reupdates the allowance to 5. User B could now call the `transferFrom(address src, address dst, uint256 amount)` function once more for an additional 5 stake tokens bringing the the total stake tokens transferred to user B to 15.

Recommendation: Add functions to atomically increase and decrease the _allowances mapping.

Non-Critical

1. OnlyMinter modifier is potentially misleading

Contract: LPToken.sol

The naming convention for the onlyMinter modifier creates some degree of ambiguity as minter roles for ERC-20 tokens are frequently held by EOAs. In this case, the minter is actually a liquidity pool and this is a key distinction as the onlyMinter role has the ability to burn any user's tokens. The modifier would read more clearly if it were, for example onlyPool as opposed to onlyMinter.

2. Typo in Comment in StakerVault

Contract: StakerVault.sol

Code:

/**
 * @notice This contract handles staked tokens from Backd pools
 * However, not that this is NOT an ERC-20 compliant contract and these  //@audit 'not' should be 'note'
 * tokens should never be integrated with any protocol assuming ERC-20 compliant
 * tokens
 * @dev When paused, allows only withdraw/unstake
 */

3. Typo in Comment in LPToken

Contract: StakerVault.sol

Code:

    /**
     * @notice Burn tokens.
     * @param owner Account from which tokens should be burned.
     * @param burnAmount Amount of tokens to burn.
     * @return Aamount of tokens burned.   //@audit 'Aamount' should be 'Amount'
     */

Gas Report

Gas Optimizations

1. General for loop best practices

The for loops used throughout the codebase can benefit from one or more of the following gas saving techniques:

1. Storing the array length in a local variable in memory before comparisons
2. Using a prefix increment instead of postfix
3. Performing unchecked arithmetic on the counter variable
4. Not initializing the counter to 0

Current code in RoleManager.sol:

function hasAnyRole(bytes32[] memory roles, address account)
        external
        view
        virtual
        override
        returns (bool)
    {
        for (uint256 i = 0; i < roles.length; i++) {
            if (hasRole(roles[i], account)) {
                return true;
            }
        }
        return false;
    }

Suggested code:

function hasAnyRole(bytes32[] memory roles, address account)
        external
        view
        virtual
        override
        returns (bool)
    {
        uint256 roleLength = roles.length;
        for (uint256 i; i < roleLength;) {
            if (hasRole(roles[i], account)) {
                return true;
            }
            unchecked {
                ++i;
            }
        }
        return false;
    }

The following locations could also benefit from some or all of the above techniques:

• Controller.sol:117
• ConvexStrategyBase.sol:313
• ConvexStrategyBase.sol:380
• StakerVault.sol:260
• BkdLocker.sol:310
• TopUpKeeperHelper.sol:43
• TopUpKeeperHelper.sol:46
• TopUpKeeperHelper.sol:72
• TopUpKeeperHelper.sol:93
• TopUpKeeperHelper.sol:165
• TopUpAction.sol:188
• TopUpAction.sol:456
• TopUpAction.sol:479
• TopUpAction.sol:506
• TopUpAction.sol:891
• CTokenRegistry.sol:61
• CompoundHandler.sol:135
• VestedEscrow.sol:93
• KeeperGauge.sol:155
• InflationManager.sol:91
• InflationManager.sol:105
• InflationManager.sol:109
• InflationManager.sol:114
• InflationManager.sol:166
• InflationManager.sol:191
• InflationManager.sol:259
• InflationManager.sol:283
• InflationManager.sol:357
• InflationManager.sol:381
• InflationManager.sol:404
• InflationManager.sol:445

2. Nested if and require statements are more efficient than &&

Nested if and require statements are more gas efficient than in-line conditions using the && operator. There were several locations in the codebase that could benefit from using nested statements instead.

Current code in ConvexStrategyBase.sol:

    function addRewardToken(address token_) external onlyGovernance returns (bool) {
        require(
            token_ != address(_CVX) && token_ != address(underlying) && token_ != address(_CRV),
            Error.INVALID_TOKEN_TO_ADD
        );

Suggested code:

    function addRewardToken(address token_) external onlyGovernance returns (bool) {
        require(token_ != address(_CVX));
        require(token_ != address(underlying));
        require(token_ != address(_CRV));

The following locations could also benefit from the above technique:

• Swapper3Crv.sol:59
• Swapper3Crv.sol:76
• Swapper3Crv.sol:128
• RoleManager.sol:95
• RoleManager.sol:97
• RoleManager.sol:99
• StakerVault.sol:386
• BkdLocker.sol:308
• AddressProvider.sol:343
• AddressProvider.sol:405
• SwapperRegistry.sol:36-38
• TopUpAction.sol:360
• TopUpAction.sol:676
• TopUpAction.sol:893
• CTokenRegistry.sol:50
• AmmConvexGauge.sol:74
• AmmConvexGauge.sol:110
• AmmConvexGauge.sol:130
• InflationManager.sol:411
• InflationManager.sol:417
• AmmGauge.sol:88

3. != 0 is More Efficient than > 0 in Require Statements

within require statements, != comparisons are slightly more gas efficient than > and when a uint is being compared to 0, != is effectively the same as > 0.

The following locations could benefit from this technique:

• BkdLocker.sol:90
• BkdLocker.sol:91
• BkdLocker.sol:136
• TopUpAction.sol:210
• TopUpAction.sol:554
• TopUpActionFeeHandler.sol:123
• LiquidityPool.sol:401
• LiquidityPool.sol:471
• LiquidityPool.sol:473
• LiquidityPool.sol:549
• StrategySwapper.sol:111
• AmmConvexGauge.sol:159
• AmmConvexGauge.sol:172
• AmmGauge.sol:104
• AmmGauge.sol:125
• KeeperGauge.sol:138
• VestedEscrow.sol:83
• Vault.sol:164

4. Initializing variables to 0 is less efficient

When initalizing a variable, setting its value to 0 explicitly is slightly less efficient than not setting its value.

The following locations could benefit from this technique:

• Controller.sol:114
• StakerVault.sol:144
• BkdLocker.sol:133
• actionstopupTopUpAction.sol:452
• tokenomicsVestedEscrow.sol:92
• tokenomicsKeeperGauge.sol:154
• poolLiquidityPool.sol:483
• vaultVault.sol:135
• vaultVault.sol:583