QA Report

[code423n4]

Low

Missing freshness validation in ETH price oracle

The ChainlinkUsdWrapper#_ethPrice() function does not check for a nonzero answer or validate that the price was returned in a recent round:

ChainlinkUsdWrapper#_ethPrice

    function _ethPrice() private view returns (int256) {
        (, int256 answer, , , ) = _ethOracle.latestRoundData();
        return answer;
    }

Although callers of ChainlinkUsd#latestRoundData can check for a nonzero price, they can't verify that the ETH oracle price used in this conversion was returned in a recent round. If the ETH oracle returns a stale price, the wrapper may return an inaccurate conversion.

Recommendation: Validate returned ETH price using roundId and answeredInRound.

Avoid payable.transfer

EthPool and EthVault both use payable(address).transfer to transfer ETH.

It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed amount of gas and may revert if future gas costs change. (See the Consensys Diligence article here).

EthPool#_doTransferOut

    function _doTransferOut(address payable to, uint256 amount) internal override {
        to.transfer(amount);
    }

EthVault#_transfer

    function _transfer(address to, uint256 amount) internal override {
        payable(to).transfer(amount);
    }

EthVault#_depositToTreasury

    function _depositToTreasury(uint256 amount) internal override {
        payable(addressProvider.getTreasury()).transfer(amount);
    }

Consider using OpenZeppelin Address.sendValue, but take care to avoid reentrancy. Callers of these internal functions should be protected with a reentrancy guard.

QA/Noncritical

Gas bank depositors can deposit for zero address

Depositors to the GasBank can accidentally make a deposit on behalf of address(0):

GasBank#depositFor

    /**
     * @notice Deposit `msg.value` on behalf of `account`
     */
    function depositFor(address account) external payable override {
        _balances[account] += msg.value;
        emit Deposit(account, msg.value);
    }

Since withdrawals check that msg.sender matches the given withdrawal account, deposits credited to address(0) cannot be recovered. Consider checking that the deposit account is not address(0).

Mismatched error message in StakerVault#transferFrom

The error message on line 152 of StakerVault.sol checks the user's allowance, but returns an "insufficient balance" error message:

StakerVault#transferFrom

        /* Get the allowance, infinite for the account owner */
        uint256 startingAllowance = 0;
        if (spender == src) {
            startingAllowance = type(uint256).max;
        } else {
            startingAllowance = _allowances[src][spender];
        }
        require(startingAllowance >= amount, Error.INSUFFICIENT_BALANCE); // Should this be 'insufficient allowance?'

Although this contract is explicitly not an ERC20 token, consider an "insufficient allowance" error, which is more consistent with user expectations.

Shadowed variable in initializer

The roleManager local variable shadows AuthorizationBase.roleManager() in initialize.

AddressProvider#initialize

    function initialize(address roleManager) external initializer {
        AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, true);
        _addressKeyMetas.set(AddressProviderKeys._ROLE_MANAGER_KEY, meta.toUInt());
        _setConfig(AddressProviderKeys._ROLE_MANAGER_KEY, roleManager);
    }

Shadowed variable in constructor

The roleManager local variable shadows AuthorizationBase.roleManager() in Authorization#constructor.

Authorization#constructor

    constructor(IRoleManager roleManager) {
        __roleManager = roleManager; // shadowed variable in constructor
    }

Unnecessary ternary

SwapperRegistry#swapperExists uses an unnecessary ternary operator:

    /**
     * @notice Check if a swapper implementation exists for a given token pair.
     * @param fromToken Address of token to swap.
     * @param toToken Address of token to receive.
     * @return True if a swapper exists for the token pair.
     */
    function swapperExists(address fromToken, address toToken)
        external
        view
        override
        returns (bool)
    {
        return _swapperImplementations[fromToken][toToken] != address(0) ? true : false;
    }

Unused interface

BkdLocker.govToken is instantiated as an IBkdToken but stored as an IERC20.

BkdLocker#constructor

    IERC20 public immutable govToken;

    constructor(
        address _rewardToken,
        address _govToken,
        IRoleManager roleManager
    ) Authorization(roleManager) {
        rewardToken = _rewardToken;
        govToken = IBkdToken(_govToken);
    }

Missing natspec docs

Functions missing natspec documentation:

• SwapperRegistry#getAllSwappableTokens

Additional events

Consider adding events for:

• CvxCrvRewardsLocker#setDelegate
• CvxCrvRewardsLocker#clearDelegate

Duplicate import

Errors.sol and IController.sol imports are duplicated in StakerVault.sol.

• StakerVault.sol#L10
• StakerVault.sol#L18

[chase-manning]

I consider this report to be of particularly high quality