Backd contest - horsefacts's results

Originally submitted by warden horsefacts in https://github.com/code-423n4/2022-04-backd-findings/issues/199, duplicate of https://github.com/code-423n4/2022-04-backd-findings/issues/52.

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.

Originally submitted by warden horsefacts in https://github.com/code-423n4/2022-04-backd-findings/issues/199, duplicate of https://github.com/code-423n4/2022-04-backd-findings/issues/17.

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.

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

Gas

Use unchecked decrement in while loop

The while loop inside BkdLocker#executeUnlocks can safely use an unchecked decrement.

Impact: 63 gas per loop iteration.

BkdLocker#executeUnlocks

        require(length > 0, "No entries");
        uint256 i = length;
        while (i > 0) {
            i = i - 1;
            if (stashedWithdraws[i].releaseTime <= block.timestamp) {
                totalAvailableToWithdraw += stashedWithdraws[i].amount;

                stashedWithdraws[i] = stashedWithdraws[stashedWithdraws.length - 1];

                stashedWithdraws.pop();
            }
        }

Using unchecked decrement:

        require(length > 0, "No entries");
        uint256 i = length;
        while (i > 0) {
            unchecked { i = i - 1; }
            if (stashedWithdraws[i].releaseTime <= block.timestamp) {
                totalAvailableToWithdraw += stashedWithdraws[i].amount;

                stashedWithdraws[i] = stashedWithdraws[stashedWithdraws.length - 1];

                stashedWithdraws.pop();
            }
        }

Use unchecked math in StakerVault#transferFrom

Since earlier require statements in StakerVault#transferFrom ensure that startingAllowance and srcTokens are >= the specified transfer amount, subtraction to calculate new allowance and balance can be performed safely using unchecked math.

Impact: 190 gas.

StakerVault#transferFrom:

        require(startingAllowance >= amount, Error.INSUFFICIENT_BALANCE);

        uint256 srcTokens = balances[src];
        require(srcTokens >= amount, Error.INSUFFICIENT_BALANCE);

        address lpGauge = currentAddresses[_LP_GAUGE];
        if (lpGauge != address(0)) {
            ILpGauge(lpGauge).userCheckpoint(src);
            ILpGauge(lpGauge).userCheckpoint(dst);
        }
        ILiquidityPool pool = controller.addressProvider().getPoolForToken(token);
        pool.handleLpTokenTransfer(src, dst, amount);

        uint256 allowanceNew = startingAllowance - amount; // Cannot underflow due to first require on L#152
        uint256 srcTokensNew = srcTokens - amount; // Cannot underflow due to second require on L#155
        uint256 dstTokensNew = balances[dst] + amount;

Using unchecked math:

        require(startingAllowance >= amount, Error.INSUFFICIENT_BALANCE);

        uint256 srcTokens = balances[src];
        require(srcTokens >= amount, Error.INSUFFICIENT_BALANCE);

        address lpGauge = currentAddresses[_LP_GAUGE];
        if (lpGauge != address(0)) {
            ILpGauge(lpGauge).userCheckpoint(src);
            ILpGauge(lpGauge).userCheckpoint(dst);
        }
        ILiquidityPool pool = controller.addressProvider().getPoolForToken(token);
        pool.handleLpTokenTransfer(src, dst, amount);

        unchecked {
            uint256 allowanceNew = startingAllowance - amount;
            uint256 srcTokensNew = srcTokens - amount;
        }
        uint256 dstTokensNew = balances[dst] + amount;