Badger Citadel contest - rfa's results

Low

Title: Noe check that _minPrice < _maxPrice

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L397-L406 Can't find any vulnerabilities if _maxPrice < _minPrice. But I think it necessary if max value should always > min value. Its also prevent wrong parameter input by governance.

Gas

Title: Using -= To store decrement calculation

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L364 Change:

        totalFundingPoolWeight = totalFundingPoolWeight - currentPoolWeight;

To:

        totalFundingPoolWeight -= currentPoolWeight;

Occurence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L408

Title: Using delete statement to change mapping value to zero

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L366 Instead of setting mapping value to zero, Using delete statement can cost less gas:

        delete fundingPoolWeights[_pool];

Title: No need to initialize var with default value

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L180-L182 By just declaring the var without set its value if the value = 0 (default) can save gas:

        uint256 lockingAmount;
        uint256 stakingAmount;
        uint256 fundingAmount;

Occurrence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L103 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L192 i in for(): https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152

Title: Prefix increment and unchecked on i in for()

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152 Change:

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

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

To:

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

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

Title: Tight variable packing in Funding.sol

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L21-L44 By placing bool citadelPriceFlag near any address can save 1 slot Change to:

    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
        keccak256("CONTRACT_GOVERNANCE_ROLE");
    bytes32 public constant POLICY_OPERATIONS_ROLE =
        keccak256("POLICY_OPERATIONS_ROLE");
    bytes32 public constant TREASURY_OPERATIONS_ROLE = keccak256("TREASURY_OPERATIONS_ROLE");
    bytes32 public constant TREASURY_VAULT_ROLE =
        keccak256("TREASURY_VAULT_ROLE");
    bytes32 public constant KEEPER_ROLE = keccak256("KEEPER_ROLE");

    uint256 public constant MAX_BPS = 10000;

    IERC20 public citadel; /// token to distribute (in vested xCitadel form)
    IVault public xCitadel; /// wrapped citadel form that is actually distributed
    IERC20 public asset; /// token to take in WBTC / bibbtc LP / CVX / bveCVX

    uint256 public citadelPriceInAsset; /// asset per citadel price eg. 1 WBTC (8 decimals) = 40,000 CTDL ==> price = 10^8 / 40,000
    uint256 public minCitadelPriceInAsset; /// Lower bound on expected citadel price in asset terms. Used as circuit breaker oracle.
    uint256 public maxCitadelPriceInAsset; /// Upper bound on expected citadel price in asset terms. Used as circuit breaker oracle.

    uint256 public assetDecimalsNormalizationValue;

    address public citadelPriceInAssetOracle;
    address public saleRecipient;
    bool public citadelPriceFlag; /// Flag citadel price for review by guardian if it exceeds min and max bounds;

Title: Using != is more efficient

Occurrence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L170 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L209 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L322 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L340 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L424 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L452 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L172 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L313 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L332 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L835 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L138 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L61 Using != is more gas efficient for validating that value isn't zero

Title: Title: Using += To store increment calculation

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L175 Change:

        funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;

To:

        funding.assetCumulativeFunded += _assetAmountIn;

Occurence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L198-L199 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L195-L196 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L218 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L446-L448

Title: Using calldata to store read only parameters

Occurence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelToken.sol#L23-L24 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/GlobalAccessControl.sol#L109 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L176-L177 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L319 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L366 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L791 Using calldata to store parameter is more effective than using memory.

Title: Using unchecked to calculate totalTokenIn in buy()

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L198 The function has validated that final totalTokenIn won't > tokenInLimit so it won't overflow. Therefore, using unchecked can save gas for the calculation. Change to:

        Unchecked{totalTokenIn = totalTokenIn + _tokenInAmount;}

Title: Unnecessary afterAmount MSTORE

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L197 By directly passing cachedXCitadel.balanceOf(address(this)) value to xCitadelToLockers calculation can save gas. The afterAmount var is just called once in the function. Change:

            uint256 afterAmount = cachedXCitadel.balanceOf(address(this));

            uint256 xCitadelToLockers = afterAmount - beforeAmount;

To:

            uint256 xCitadelToLockers = cachedXCitadel.balanceOf(address(this)) - beforeAmount;

Title: Using if instead of else if statement

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L270 Its impossible that the condition at L266 and L270 will pass at the same time. Replacing else if at L270 with if statement can save gas Change to:

        if (_weight == 0 && poolExists) {
            _removeFundingPool(_pool);

            emit FundingPoolWeightSet(_pool, _weight, totalFundingPoolWeight);
        } if (_weight > 0) {

Title: Using custom error

Occurrence: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L116-L120 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L256 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L272 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L299-L302 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L319-L328 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L343 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L368-L371 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L375-L378 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L170-L174 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L270-L271 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L298 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L322-L326 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/GlobalAccessControl.sol#L112-L119 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L120-L135

Defined by using error statement, and using if(condition)revert() to check the condition. It can be implemented for all require() statement for gas opt.

Title: Prevent unnecessary SLOAD and math operation by MSTORE

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L171-L175 By caching the calculation of funding.assetCumulativeFunded + _assetAmountIn can saving deposit() execution gas fee Change:

        require(
            funding.assetCumulativeFunded + _assetAmountIn <= funding.assetCap,
            "asset funding cap exceeded"
        );
        funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;

To:

	uint _chacedVar = funding.assetCumulativeFunded + _assetAmountIn;
        require(
            _chacedVar <= funding.assetCap,
            "asset funding cap exceeded"
        );
        funding.assetCumulativeFunded = _chacedVar;