Badger Citadel contest - saian's results

Gas Optimizations

revert strings > 32 bytes

Impact

Revert strings greater than 32 bytes will increase deployment gas as well as when require condition is met

Proof of concept

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

    require(
        keccak256(bytes(roleString)) == role,
        "Role string and role do not match"
    );

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

    require(
        _fundingBps + _stakingBps + _lockingBps == MAX_BPS,
        "CitadelMinter: Sum of propvalues must be 10000 bps"
    );

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

    require(
        lastMintTimestamp == 0,
        "CitadelMinter: last mint timestamp already initialized"
    );

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

    require(
        globalStartTimestamp != 0,
        "CitadelMinter: supply schedule start not initialized"
    );

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

    require(
        fundingPools.remove(_pool),
        "CitadelMinter: funding pool does not exist for removal"
    );

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

    require(
        fundingPools.add(_pool),
        "CitadelMinter: funding pool already exists"
    );

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

    require(
        citadelPriceFlag == false,
        "Funding: citadel price from oracle flagged and pending review"
    );

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

    require(
        _assetCap > funding.assetCumulativeFunded,
        "cannot decrease cap below global sum of assets in"
    );

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

    require(
        _token != address(asset),
        "cannot sweep funding asset, use claimAssetToTreasury()"
    );

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

    require(
        _saleRecipient != address(0),
        "Funding: sale recipient should not be zero"
    );

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

    require(
        _saleStart >= block.timestamp,
        "KnightingRound: start date may not be in the past" 
    );
    require(
        _saleDuration > 0,
        "KnightingRound: the sale duration must not be zero"
    );
    require(
        _tokenOutPrice > 0,
        "KnightingRound: the price must not be zero"
    );
    require(
        _saleRecipient != address(0),
        "KnightingRound: sale recipient should not be zero"
    );

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

    require(
        _saleStart >= block.timestamp,
        "KnightingRound: start date may not be in the past"
    );
    require(!finalized, "KnightingRound: already finalized");

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

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

    require(
        _saleRecipient != address(0),
        "KnightingRound: sale recipient should not be zero"
    );

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

    require(!finalized, "KnightingRound: already finalized");

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

    require(
        _fees <= PERFORMANCE_FEE_HARD_CAP,
        "performanceFeeStrategist too high"
    );

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

    require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
    require(_amount > 0, "StakedCitadelVester: cannot vest 0");

Mitigation

Revert string can be reduced to 32 bytes or can be replaced with custom errors

Initialization with default value

When variables are declared it contains the default values, so explicit initilization of default value is not necessary

Proof of concept

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

    for (uint256 i = 0; i < numPools; i++) { 

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

    uint256 lockingAmount = 0;
    uint256 stakingAmount = 0;  
    uint256 fundingAmount = 0;

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

    // No circuit breaker on price by default
    minCitadelPriceInAsset = 0;

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

    for (uint256 i = 0; i < roles.length; i++) {

Mitigation

Initization with default value can be removed

Avoid storage re-read and save gas

Storage variables can be stored in a local variables and re-used instead of reading from storage to save gas

Proof of concept

citadelToken in CitadelMinter:mintAndDistribute()

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

funding.discount in

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

    if (funding.discount > 0) { 
        citadelAmount_ =
            (citadelAmountWithoutDiscount * MAX_BPS) /
            (MAX_BPS - funding.discount);
    }

asset in

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

    uint256 amount = asset.balanceOf(address(this));
    require(amount > 0, "nothing to claim"); 
    asset.safeTransfer(saleRecipient, amount);

    emit ClaimToTreasury(address(asset), amount);

minCitadelPriceInAsset, maxCitadelPriceInAsset in

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

    if (
        _citadelPriceInAsset < minCitadelPriceInAsset ||
        _citadelPriceInAsset > maxCitadelPriceInAsset
    ) {
        citadelPriceFlag = true;
        emit CitadelPriceFlag(
            _citadelPriceInAsset,
            minCitadelPriceInAsset,
            maxCitadelPriceInAsset
        );

saleStart, guestList in

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

totalTokenin, tokenInLimit in

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

strategy in

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

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

vesting in

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

strategist in

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

Mitigation

storage variable can be cached in a temporary variable and re-used

Postfix increment can be replaced with prefix increment

As return value of postfix i++ is not used, it can be replaced with prefix increment ++i, and unchecked can be added as there is no risk of overflow

Proof of concept

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

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

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

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

Mitigation

postfix increment can be converted to prefix increment

> can be replaced with !=

!= 0 is more efficient than > 0 with uint in require statements

Proof of concept

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

    require(length > 0, "CitadelMinter: no funding pools");

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

        require(_assetAmountIn > 0, "_assetAmountIn must not be 0"); 

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

    require(amount > 0, "nothing to sweep");

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

    require(amount > 0, "nothing to claim");

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

    require(_citadelPriceInAsset > 0, "citadel price must not be zero");

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

    require(
        _saleDuration > 0,
        "KnightingRound: the sale duration must not be zero"
    );
    require(
        _tokenOutPrice > 0,
        "KnightingRound: the price must not be zero"
    );

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

    require(_tokenInAmount > 0, "_tokenInAmount should be > 0");

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

    require(
        _saleDuration > 0,
        "KnightingRound: the sale duration must not be zero" 
    );

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

    require(
        _tokenOutPrice > 0,
        "KnightingRound: the price must not be zero" 
    );

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

    require(amount > 0, "nothing to sweep");

Value can be used directly

Storage values used only once in function can be used directly instead of storing in a temporary variable

Proof of concept

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

    uint256 currentPoolWeight = fundingPoolWeights[_pool];
    totalFundingPoolWeight = totalFundingPoolWeight - currentPoolWeight;

using unchecked can save gas

When expression cant overflow/underflow using unchecked can avoid default overflow/underflow checks introduced in 0.8 and save gas

Proof of concept

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

   if (assetCumulativeFunded < assetCap) {
       limitLeft_ = assetCap - assetCumulativeFunded; 
   }

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

   if (totalTokenIn < tokenInLimit) {
       limitLeft_ = tokenInLimit - totalTokenIn; 
   }

Unused variables

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

    bool public transferFromDisabled; // Set to true in initialize

Mitigation

Unused variable can be removed

Function return value can be cached and reused

Proof of concept

totalSupply() in

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

   if (totalSupply() == 0) {
       return ONE_ETH;
   }
   return (balance() * ONE_ETH) / totalSupply();

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

    if (totalSupply() == 0) {
        shares = _amount;
    } else {
        shares = (_amount * totalSupply()) / _pool; 
    }

balance() in

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