Badger Citadel contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L830

Vulnerability details

The IVesting interface contains a setupVesting function:

IVesting#4

interface IVesting {
    function setupVesting(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external;
}

This function is called to send withdrawals to the vesting contract in StakedCitadel#withdraw:

StakedCitadel#830

        // Send funds to vesting contract and setup vesting
        IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp);
        token.safeTransfer(vesting, _amount);

However, the corresponding function in StakedCitadelVester is named vest, not setupVesting.

StakedCitadelVester#132

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
        require(_amount > 0, "StakedCitadelVester: cannot vest 0");

        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

        emit Vest(
            recipient,
            vesting[recipient].lockedAmounts,
            _unlockBegin,
            vesting[recipient].unlockEnd
        );
    }

This is pretty clearly just a typo in StakedCitadelVester, but as written calls to StakedCitadel#withdraw will revert due to this incorrect interface, blocking users from withdrawing to the vesting contract. (Had this actually been deployed, it would be possible to redeploy StakedCitadelVester with the correct interface and update it on StakedCitadel).

Recommendation: rename StakedCitadelVester#vest to setupVesting. Since this error was the result of an incorrect interface integration, consider an end to end test that exercises the full lifecycle from a deposit to Funding through staking, withdrawal, vesting, and claims.

Test case:

    function test_withdrawal_to_vesting() public {
        uint256 amount = 1000 ether;
        tip(address(citadel), whale, amount);
        vm.startPrank(whale);
        citadel.approve(address(xCitadel), amount);

        xCitadel.deposit(amount);

        // Withdrawal to vesting contract fails due to
        // incorrect interface. The `StakedCitadel` contract
        // calls `setupVesting`, but the vesting function on
        // `StakedCitadelVester` is named `vest`.
        xCitadel.withdraw(xCitadel.balanceOf(whale));
    }

Low

setCitadelAssetPriceBounds can set min bound above max bound

Governance can intentionally or accidentally set a value for minCitadelPriceInAsset that is greater than maxCitadelPriceInAsset. If the parameters are misconfigured in this way, any updated price will trigger the citadelPriceFlag, and oracle prices will not update until the parameters are updated by governance.

Consider validating that minCitadelPriceInAsset is less than or equal to maxCitadelPriceInAsset in setCitadelAssetPriceBounds.

Funding.sol

    function setCitadelAssetPriceBounds(uint256 _minPrice, uint256 _maxPrice)
        external
        gacPausable
        onlyRole(CONTRACT_GOVERNANCE_ROLE)
    {
        minCitadelPriceInAsset = _minPrice;
        maxCitadelPriceInAsset = _maxPrice;

        emit CitadelPriceBoundsSet(_minPrice, _maxPrice);
    }

Test case:

    function test_min_price_set_above_max_price(uint16 n) public {
        vm.assume(n > 0);
        // Governance accidentally sets min > max
        vm.prank(governance);
        fundingCvx.setCitadelAssetPriceBounds(1500, 500);

        // Any reported price triggers the price flag
        vm.prank(eoaOracle);
        fundingCvx.updateCitadelPriceInAsset(n);
        assertTrue(fundingCvx.citadelPriceFlag());
    }

setDiscountLimits can set min discount above max discount

Governance can intentionally or accidentally set a value for funding.minDiscount that is greater than funding.maxDiscount. If values are misconfigured in this way, any call by Policy Ops to setDiscount will revert until these parameters are reset by governance.

Consider validating that funding.minDiscount is less than or equal to funding.MaxDiscount in setDiscountLimits.

Funding.sol#356

    function setDiscountLimits(uint256 _minDiscount, uint256 _maxDiscount)
        external
        gacPausable
        onlyRole(CONTRACT_GOVERNANCE_ROLE)
    {
        require(_maxDiscount < MAX_BPS , "maxDiscount >= MAX_BPS");
        funding.minDiscount = _minDiscount;
        funding.maxDiscount = _maxDiscount;

        emit DiscountLimitsSet(_minDiscount, _maxDiscount);
    }

Test case:

    function test_min_discount_set_above_max_discount(uint16 n) public {
        vm.assume(n > 0);
        // Governance accidentally sets min > max
        vm.prank(governance);
        fundingCvx.setDiscountLimits(5000, 1000);

        // Any call to setDiscount will revert
        vm.startPrank(policyOps);
        try fundingCvx.setDiscount(n) {
            fail();
        } catch Error(string memory message) {}
    }

QA

Unnecessary ABI coder pragma

Solidity versions greater than v0.8.0 use the v2 ABI coder by default. The pragma pragma experimental ABIEncoderV2; is still valid, but has no effect.

Usages:

• CitadelMinter.sol#4
• GlobalAccessControl.sol#4

Typo in natspec comment

There's a minor typo in the natspec comments for CitadelMinter#initialize: expection should be exception:

CitadelMinter#102

     * @dev this contract is intended to be the only way citadel is minted, with the expection of the initial minting event

Unlocked version pragma

Several files use unlocked version pragmas that can be locked to 0.8.12.

• CitadelToken.sol#2
• GlobalAccessControl.sol#3
• GlobalAccessControlManaged.sol#3

Modifier in unconventional location

The onlyCitadelPriceInAssetOracle modifier is defined in an unconventional location within the source file, in between event definitions. Consider moving this modifier to line 145, along with other modifiers in this file.

• Funding.sol#79

Incorrect parameter description in natspec comment

The _minCitadelOut parameter documentation in Funding.sol is incorrect. (It reads "ID of DAO to vote for," which looks like it was copy-pasted from KnightingRound.sol).

• Funding.sol#160

Function can be declared external

Funding#getStakedCitadelAmountOut is not called internally and can be declared external.

• Funding.sol#223

Incorrect description in natspec comment

The @dev annotation on Funding#sweep appears to be incorrect. (Looks like it was copy-pasted from KnightingRound.sol).

• Funding.sol#310

Incorrect natspec comments

The natspec comments at the top of GlobalAccessControl.sol appear to be for another contract.

• GlobalAccessControl#12

Unused variable

The transferFromDisabled state variable in GlobalAccessControl.sol appears to be unused. A code comment says it is set to true in initialize, but it does not appear to be set or updated. Although this variable is exposed in the IGac interface, there is no mechanism to set this variable. Consider whether it can be removed altogether or declared constant.

    // Should the function transferFrom be disabled
    // NOTE: This is enforced at the contract level, the contract just allows the toggling of the bool
    bool public transferFromDisabled; // Set to true in initialize

Gas

Optimize loop in CitadelMinter#getFundingPoolWeights

The loop at CitadelMinter#getFundingPoolWeights()#152 can be optimized by using an unchecked loop counter increment. This is safe since the loop counter will never realistically exceed type(uint256).max.

Current version:

        for (uint256 i = 0; i < numPools; i++) {
            address pool = fundingPools.at(i);
            uint256 weight = fundingPoolWeights[pool];

            pools[i] = pool;
            weights[i] = weight;
        }

Gas test:

[PASS] testGasGetFundingPoolWeights() (gas: 219615)

Optimized:

        for (uint256 i = 0; i < numPools;) {
            address pool = fundingPools.at(i);
            uint256 weight = fundingPoolWeights[pool];

            pools[i] = pool;
            weights[i] = weight;
            unchecked {++i;}
        }

Gas test:

[PASS] testGasGetFundingPoolWeights() (gas: 219465)

Test case:

    function testGasGetFundingPoolWeights() public{
        vm.startPrank(policyOps);
        citadelMinter.setFundingPoolWeight(address(fundingCvx), 8000);
        citadelMinter.setFundingPoolWeight(address(fundingWbtc), 2000);

        citadelMinter.getFundingPoolWeights();
    }

Optimize loop in CitadelMinter#_transferToFundingPools

The loop at CitadelMinter#344 can be optimized by using an unchecked loop counter increment. This is safe since the loop counter will never realistically exceed type(uint256).max.

Additionally, the IERC20Upgradeable interface to citadelToken can be cached outside the loop.

Current version:

        for (uint256 i; i < length; ++i) {
            address pool = fundingPools.at(i);
            uint256 weight = fundingPoolWeights[pool];

            uint256 amount = (_citadelAmount * weight) /
                cachedTotalFundingPoolWeight;

            IERC20Upgradeable(address(citadelToken)).safeTransfer(pool, amount);

            emit CitadelDistributionToFundingPool(
                lastMintTimestamp,
                block.timestamp,
                pool,
                amount
            );
        }

Gas test:

[PASS] testGasMintAndDistribute() (gas: 426247)

Optimized:

        IERC20Upgradeable cachedCitadelToken = IERC20Upgradeable(address(citadelToken));
        for (uint256 i; i < length;) {
            address pool = fundingPools.at(i);
            uint256 weight = fundingPoolWeights[pool];

            uint256 amount = (_citadelAmount * weight) /
                cachedTotalFundingPoolWeight;

            cachedCitadelToken.safeTransfer(pool, amount);

            emit CitadelDistributionToFundingPool(
                lastMintTimestamp,
                block.timestamp,
                pool,
                amount
            );
            unchecked {++i;}
        }

Gas test:

[PASS] testGasMintAndDistribute() (gas: 426046)

Test case:

    function testGasMintAndDistribute() public{
        vm.startPrank(governance);
        schedule.setMintingStart(block.timestamp);
        citadelMinter.initializeLastMintTimestamp();
        vm.stopPrank();

        vm.warp(block.timestamp + 1000);

        vm.startPrank(policyOps);
        citadelMinter.setCitadelDistributionSplit(10000, 0, 0);
        citadelMinter.setFundingPoolWeight(address(fundingCvx), 8000);
        citadelMinter.setFundingPoolWeight(address(fundingWbtc), 2000);
        citadelMinter.mintAndDistribute();
    }