Badger Citadel contest - Dravee's results

[L-01] Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

src/CitadelMinter.sol:
  109:     function initialize(

src/CitadelToken.sol:
  22:     function initialize(

src/Funding.sol:
  104:     function initialize(

src/GlobalAccessControl.sol:
  61:     function initialize(address _initialContractGovernance)

src/KnightingRound.sol:
  109:     function initialize(

src/StakedCitadel.sol:
  167:     function initialize(

src/StakedCitadelLocker.sol:
  121:     function initialize(

src/StakedCitadelVester.sol:
  59:     function initialize(

src/SupplySchedule.sol:
  43:     function initialize(address _gac) public initializer {

[L-02] Unbounded loop on array can lead to DoS

As this array can grow quite large, the transaction's gas cost could exceed the block gas limit and make it impossible to call this function at all.

src/StakedCitadelLocker.sol:
  325:         for (uint i = locks.length - 1; i + 1 != 0; i--) {
  363:         for (uint i = locks.length - 1; i + 1 != 0; i--) {
  459:         for (uint i = nextUnlockIndex; i < locks.length; i++) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

[L-03] MedianOracle.sol and StakedCitadelLocker.sol should implement a 2-step ownership transfer pattern

These contracts inherit from OpenZeppelin's libraries and the transferOwnership() function is the default one (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. And this role is extensively used accross the solution's contracts. Consider overriding the default transferOwnership() function to first nominate an address as the pending owner and implementing an acceptOwnership() function which is called by the pending owner to confirm the transfer.

[L-04] Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

File: StakedCitadelVester.sol
85:     function claim(address recipient, uint256 amount) external {
86:         uint256 claimable = claimableBalance(msg.sender);
87:         if (amount > claimable) {
88:             amount = claimable;
89:         }
90:         if (amount != 0) {
91:             vesting[msg.sender].claimedAmounts =
92:                 vesting[msg.sender].claimedAmounts +
93:                 amount;
94:             vestingToken.safeTransfer(recipient, amount); //@audit low: recipient should be address(0) checked
95:             emit Claimed(msg.sender, recipient, amount);
96:         }
97:     }

[L-05] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

Consider adding a timelock to:

StakedCitadel.sol:521:    function setMaxWithdrawalFee(uint256 _fees) external {
StakedCitadel.sol:533:    function setMaxPerformanceFee(uint256 _fees) external {
StakedCitadel.sol:548:    function setMaxManagementFee(uint256 _fees) external {
StakedCitadel.sol:611:    function setWithdrawalFee(uint256 _withdrawalFee) external whenNotPaused {
StakedCitadel.sol:625:    function setPerformanceFeeStrategist(uint256 _performanceFeeStrategist)
StakedCitadel.sol:645:    function setPerformanceFeeGovernance(uint256 _performanceFeeGovernance)
StakedCitadel.sol:664:    function setManagementFee(uint256 _fees) external whenNotPaused {

[L-06] Use of deprecated keyword now

Use of deprecated functions/operators should be avoided to prevent unintended errors with newer compiler versions. (see here). Here, use block.timestamp instead of now:

MedianOracle.sol:129:        require(timestamps[index_recent].add(reportDelaySec) <= now);
MedianOracle.sol:131:        reports[index_past].timestamp = now;
MedianOracle.sol:134:        emit ProviderReportPushed(providerAddress, payload, now);
MedianOracle.sol:161:        uint256 minValidTimestamp =  now.sub(reportExpirationTimeSec);
MedianOracle.sol:162:        uint256 maxValidTimestamp =  now.sub(reportDelaySec);

[N-01] Open TODOS

Consider resolving the TODOs before deploying.

src/Funding.sol:
   15:  * TODO: Better revert strings
   61:     // TODO: we should conform to some interface here
  183:         // TODO: Check gas costs. How does this relate to market buying if you do want to deposit to xCTDL?

src/GlobalAccessControl.sol:
  106:     /// TODO: Add string -> hash EnumerableSet to a new RoleRegistry contract for easy on-chain viewing.

src/KnightingRound.sol:
  14:  * TODO: Better revert strings

src/SupplySchedule.sol:
  159:         // TODO: Require this epoch is in the future. What happens if no data is set? (It just fails to mint until set)

[N-02] Related data should be grouped in a struct

The "mappings for balance data" maps should be grouped in a struct.

From:

File: StakedCitadelLocker.sol
88:     mapping(address => Balances) public balances; //@audit NC: should be grouped in a struct with below
89:     mapping(address => LockedBalance[]) public userLocks;//@audit NC: should be grouped in a struct with above 

To

    struct BalanceData {
        Balances balances;
        LockedBalance[] userLocks;
    }
   
    mapping(address => BalanceData) public balanceData;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete balanceData[address].

The same could be said for the following fields:

File: StakedCitadel.sol
94:     mapping(address => uint256) public additionalTokensEarned; //@audit NC: should be grouped in a struct with below
95:     mapping(address => uint256) public lastAdditionalTokenAmount; //@audit NC: should be grouped in a struct with above

Table of Contents:

• CitadelMinter.mintAndDistribute(): L199 should be unchecked due to L193-L197
• Funding.sol: state variables can be tightly packed to save 1 storage slot
• Funding.initialize(): should use memory instead of storage variable
• Funding.onlyWhenPriceNotFlagged(): boolean comparison 147
• Funding.deposit(): funding.assetCumulativeFunded + _assetAmountIn should get cached
• Funding.getRemainingFundable(): L236 should be unchecked due to L235
• Funding.claimAssetToTreasury(): asset should get cached
• KnightingRound.initialize(): should use memory instead of storage variable
• KnightingRound.buy(): saleStart, totalTokenIn and guestlist should get cached
• KnightingRound.getTokenInLimitLeft(): totalTokenIn and tokenInLimit should get cached + L250 should be unchecked due to L249
• StakedCitadel.deposit(): Use calldata instead of memory
• StakedCitadel.depositAll(): Use calldata instead of memory
• StakedCitadel.setStrategy(): strategy should get cached
• StakedCitadel.earn(): strategy should get cached
• StakedCitadel._depositFor(): token should get cached
• StakedCitadel._depositFor(): L776 should be unchecked due to L773-L775
• StakedCitadel._depositForWithAuthorization(): guestList should get cached
• StakedCitadel._withdraw(): token and vesting should get cached
• StakedCitadel._withdraw(): L817 should be unchecked due to L816
• StakedCitadelLocker.sol: state variables can be tightly packed to save 1 storage slot
• StakedCitadelLocker.totalSupplyAtEpoch(): Use a storage variable's reference instead of repeatedly fetching it (epochs[i])
• StakedCitadel._withdraw(): maximumStake, minimumStake and stakingProxy should get cached
• StakedCitadelVester.claimableBalance(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (vesting[recipient])
• StakedCitadelVester.vest(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (vesting[recipient])
• SupplySchedule.getEpochAtTimestamp(): globalStartTimestamp should get cached
• SupplySchedule.getMintable(): L105-L110 should be unchecked due to L95 and L99-L101
• SupplySchedule.getMintableDebug(): globalStartTimestamp should get cached
• SupplySchedule.getMintableDebug(): L200-L205 should be unchecked due to L184 and L188
• No need to explicitly initialize variables with default values
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• >= is cheaper than >
• Shift Right instead of Dividing by 2
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Increments can be unchecked
• Consider making some constants as non-public to save gas
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas

CitadelMinter.mintAndDistribute(): L199 should be unchecked due to L193-L197

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tag):

File: CitadelMinter.sol
169:     function mintAndDistribute()
...
193:             uint256 beforeAmount = cachedXCitadel.balanceOf(address(this));
194: 
195:             IVault(cachedXCitadel).deposit(lockingAmount);
196: 
197:             uint256 afterAmount = cachedXCitadel.balanceOf(address(this));
198: 
199:             uint256 xCitadelToLockers = afterAmount - beforeAmount; //@audit gas: should be unchecked due to L193-L197

Funding.sol: state variables can be tightly packed to save 1 storage slot

From (see @audit tags):

File: Funding.sol
38:     uint256 public maxCitadelPriceInAsset;  //@audit gas: 32 bytes size
39:     bool public citadelPriceFlag; //@audit gas: 1 byte size, can be tightly packed
40: 
41:     uint256 public assetDecimalsNormalizationValue; //@audit gas: 32 bytes size
42: 
43:     address public citadelPriceInAssetOracle; //@audit gas: 20 bytes size
44:     address public saleRecipient; //@audit gas: 20 bytes size
45: 

to:

File: Funding.sol
      uint256 public maxCitadelPriceInAsset;  //@audit gas: 32 bytes size
      bool public citadelPriceFlag; //@audit gas: 1 byte size, can be tightly packed
  
      address public citadelPriceInAssetOracle; //@audit gas: 20 bytes size
      address public saleRecipient; //@audit gas: 20 bytes size

      uint256 public assetDecimalsNormalizationValue; //@audit gas: 32 bytes size

Funding.initialize(): should use memory instead of storage variable

See @audit tag:

File: Funding.sol
104:     function initialize(
...
127:         asset = IERC20(_asset);
...
134:         assetDecimalsNormalizationValue = 10**asset.decimals(); //@audit gas: should use 10**IERC20(_asset).decimals()

Funding.onlyWhenPriceNotFlagged(): boolean comparison 147

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here (see @audit tag):

File: Funding.sol
145:     modifier onlyWhenPriceNotFlagged() {
146:         require(
147:             citadelPriceFlag == false, //@audit gas: instead of comparing to a constant, just use "!citadelPriceFlag"
148:             "Funding: citadel price from oracle flagged and pending review"
149:         );
150:         _;
151:     }

Funding.deposit(): funding.assetCumulativeFunded + _assetAmountIn should get cached

See @audit tags:

File: Funding.sol
163:     function deposit(uint256 _assetAmountIn, uint256 _minCitadelOut)
164:         external
165:         onlyWhenPriceNotFlagged
166:         gacPausable
167:         nonReentrant
168:         returns (uint256 citadelAmount_)
169:     {
170:         require(_assetAmountIn > 0, "_assetAmountIn must not be 0");
171:         require(
172:             funding.assetCumulativeFunded + _assetAmountIn <= funding.assetCap, //@audit gas: should cache "funding.assetCumulativeFunded + _assetAmountIn" (SLOAD 1 + calculation)
173:             "asset funding cap exceeded"
174:         );
175:         funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn; //@audit gas: should use cached "funding.assetCumulativeFunded + _assetAmountIn"  (SLOAD 2 + calculation)

Funding.getRemainingFundable(): L236 should be unchecked due to L235

See @audit tag:

File: Funding.sol
232:     function getRemainingFundable() external view returns (uint256 limitLeft_) {
233:         uint256 assetCumulativeFunded = funding.assetCumulativeFunded;
234:         uint256 assetCap = funding.assetCap;
235:         if (assetCumulativeFunded < assetCap) {
236:             limitLeft_ = assetCap - assetCumulativeFunded; //@audit gas: should be unchecked due to L235
237:         }
238:     }

Funding.claimAssetToTreasury(): asset should get cached

See @audit tag:

File: Funding.sol
334:     function claimAssetToTreasury()
335:         external
336:         gacPausable
337:         onlyRole(TREASURY_OPERATIONS_ROLE)
338:     {
339:         uint256 amount = asset.balanceOf(address(this)); //@audit gas: should cache "asset" (SLOAD 1)
340:         require(amount > 0, "nothing to claim");
341:         asset.safeTransfer(saleRecipient, amount);//@audit gas: should use cached "asset" (SLOAD 2)
342: 
343:         emit ClaimToTreasury(address(asset), amount);//@audit gas: should use cached "asset" (SLOAD 3)
344:     }

KnightingRound.initialize(): should use memory instead of storage variable

See @audit tag:

File: KnightingRound.sol
109:     function initialize(
...
140:         tokenIn = ERC20Upgradeable(_tokenIn);
...
148:         tokenInNormalizationValue = 10**tokenIn.decimals();  //@audit gas: should use 10**ERC20Upgradeable(_tokenIn).decimals()

KnightingRound.buy(): saleStart, totalTokenIn and guestlist should get cached

See @audit tags:

File: KnightingRound.sol
162:     function buy(
...
167:         require(saleStart <= block.timestamp, "KnightingRound: not started"); //@audit gas: should cache saleStart (SLOAD 1)
168:         require(
169:             block.timestamp < saleStart + saleDuration, //@audit gas: should use cached saleStart (SLOAD 2)
...
173:         require(
174:             totalTokenIn + _tokenInAmount <= tokenInLimit, //@audit gas: should cache totalTokenIn (SLOAD 1)
...
178:         if (address(guestlist) != address(0)) { //@audit gas: should cache guestlist (SLOAD 1)
179:             require(guestlist.authorized(msg.sender, _proof), "not authorized"); //@audit gas: should use cached guestlist (SLOAD 2)
180:         }
...
198:         totalTokenIn = totalTokenIn + _tokenInAmount;  //@audit gas: should use cached totalTokenIn (SLOAD 2)

KnightingRound.getTokenInLimitLeft(): totalTokenIn and tokenInLimit should get cached + L250 should be unchecked due to L249

See @audit tags:

File: KnightingRound.sol
248:     function getTokenInLimitLeft() external view returns (uint256 limitLeft_) {
249:         if (totalTokenIn < tokenInLimit) { //@audit gas: should cache totalTokenIn (SLOAD 1) and tokenInLimit (SLOAD 1)
250:             limitLeft_ = tokenInLimit - totalTokenIn; //@audit gas: should be unchecked due to L249  //@audit gas: should use cached totalTokenIn (SLOAD 2) and tokenInLimit (SLOAD 2)
251:         }
252:     }

StakedCitadel.deposit(): Use calldata instead of memory

When arguments are read-only on external functions, the data location should be calldata:

File: StakedCitadel.sol
319:     function deposit(uint256 _amount, bytes32[] memory proof) //@audit gas: should be calldata instead of memory
320:         external
321:         whenNotPaused
322:     {
323:         _depositWithAuthorization(_amount, proof);
324:     }

StakedCitadel.depositAll(): Use calldata instead of memory

See @audit tag:

File: StakedCitadel.sol
319:     function deposit(uint256 _amount, bytes32[] memory proof) //@audit gas: should be calldata instead of memory
320:         external
321:         whenNotPaused
322:     {
323:         _depositWithAuthorization(_amount, proof);
324:     }

StakedCitadel.setStrategy(): strategy should get cached

See @audit tags:

File: StakedCitadel.sol
500:     function setStrategy(address _strategy) external whenNotPaused {
...
505:         if (strategy != address(0)) { //@audit gas: should cache strategy (SLOAD 1)
506:             require(
507:                 IStrategy(strategy).balanceOf() == 0, //@audit gas: should use cached strategy (SLOAD 2)

StakedCitadel.earn(): strategy should get cached

See @audit tags:

File: StakedCitadel.sol
717:     function earn() external {
...
722:         token.safeTransfer(strategy, _bal); //@audit gas: should cache strategy (SLOAD 1)
723:         IStrategy(strategy).earn();//@audit gas: should use cached strategy (SLOAD 2)
724:     }

StakedCitadel._depositFor(): token should get cached

See @audit tags:

File: StakedCitadel.sol
764:     function _depositFor(address _recipient, uint256 _amount)
...
773:         uint256 _before = token.balanceOf(address(this)); //@audit gas: should cache token (SLOAD 1)
774:         token.safeTransferFrom(msg.sender, address(this), _amount); //@audit gas: should use cached token (SLOAD 2)
775:         uint256 _after = token.balanceOf(address(this));//@audit gas: should use cached token (SLOAD 3)

StakedCitadel._depositFor(): L776 should be unchecked due to L773-L775

See @audit tags:

File: StakedCitadel.sol
764:     function _depositFor(address _recipient, uint256 _amount)
...
773:         uint256 _before = token.balanceOf(address(this)); 
774:         token.safeTransferFrom(msg.sender, address(this), _amount); 
775:         uint256 _after = token.balanceOf(address(this));
776:         _mintSharesFor(_recipient, _after - _before, _pool); //@audit gas: should be unchecked due to L773-L775
777:     }

StakedCitadel._depositForWithAuthorization(): guestList should get cached

See @audit tags:

File: StakedCitadel.sol
788:     function _depositForWithAuthorization(
...
793:         if (address(guestList) != address(0)) {//@audit gas: should cache guestList (SLOAD 1)
794:             require(
795:                 guestList.authorized(_recipient, _amount, proof), //@audit gas: should use cached guestList (SLOAD 2)

StakedCitadel._withdraw(): token and vesting should get cached

See @audit tags:

File: StakedCitadel.sol
808:     function _withdraw(uint256 _shares) internal nonReentrant {
...
815:         uint256 b = token.balanceOf(address(this)); //@audit gas: should cache token (SLOAD 1)
...
819:             uint256 _after = token.balanceOf(address(this)); //@audit gas: should use cached token (SLOAD 2)
...
830:         IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp); //@audit gas: should cache vesting (SLOAD 1)
831:         token.safeTransfer(vesting, _amount);  //@audit gas: should use cached token (SLOAD 3) //@audit gas: should use cached vesting (SLOAD 2)

StakedCitadel._withdraw(): L817 should be unchecked due to L816

See @audit tag:

File: StakedCitadel.sol
808:     function _withdraw(uint256 _shares) internal nonReentrant {
...
816:         if (b < r) {
817:             uint256 _toWithdraw = r - b; //@audit gas: should be unchecked due to L816

StakedCitadelLocker.sol: state variables can be tightly packed to save 1 storage slot

From (see @audit tags):

File: StakedCitadelLocker.sol
109:     uint256 public kickRewardEpochDelay = 4;
110: 
111:     //shutdown
112:     bool public isShutdown = false; //@audit gas: can be tightly packed by being moved after uint8
113: 
114:     //erc20-like interface
115:     string private _name;
116:     string private _symbol;
117:     uint8 private _decimals;

to:

   uint256 public kickRewardEpochDelay = 4;

   //erc20-like interface
   string private _name;
   string private _symbol;
   uint8 private _decimals;

   //shutdown
   bool public isShutdown = false;

StakedCitadelLocker.totalSupplyAtEpoch(): Use a storage variable's reference instead of repeatedly fetching it (epochs[i])

See @audit tag:

File: StakedCitadelLocker.sol
403:     function totalSupplyAtEpoch(uint256 _epoch) view external returns(uint256 supply) {
...
409:         for (uint i = _epoch; i + 1 != 0; i--) {
410:             Epoch storage e = epochs[i];
411:             if (uint256(e.date) <= cutoffEpoch) {
412:                 break;
413:             }
414:             supply = supply.add(epochs[i].supply); //@audit gas: use "e.supply"

StakedCitadel._withdraw(): maximumStake, minimumStake and stakingProxy should get cached

See @audit tags:

File: StakedCitadelLocker.sol
747:     function updateStakeRatio(uint256 _offset) internal {
...
760:         uint256 mean = maximumStake.add(minimumStake).div(2); //@audit gas: should cache maximumStake (SLOAD 1) and minimumStake (SLOAD 1)
761:         uint256 max = maximumStake.add(_offset); //@audit gas: should use cached maximumStake (SLOAD 2)
762:         uint256 min = MathUpgradeable.min(minimumStake, minimumStake - _offset); //@audit gas: should use cached minimumStake (SLOAD 2 & 3)
763:         if (ratio > max) {
...
767:         } else if (ratio < min) {
...
770:             stakingToken.safeTransfer(stakingProxy, increase);  //@audit gas: should cache stakingProxy (SLOAD 1)
771:             IStakingProxy(stakingProxy).stake(); //@audit gas: should use cached stakingProxy (SLOAD 2)
772:         }
773:     }

StakedCitadelVester.claimableBalance(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (vesting[recipient])

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

Here, instead of repeatedly calling vesting[recipient], save its reference like this: VestingParams storage _vestingParams = vesting[recipient] and use it.

Impacted lines (see @audit tags):

File: StakedCitadelVester.sol
108:     function claimableBalance(address recipient) public view returns (uint256) {
109:         uint256 locked = vesting[recipient].lockedAmounts; //@audit gas: help the optimizer by declaring VestingParams storage _vestingParams = vesting[recipient];
110:         uint256 claimed = vesting[recipient].claimedAmounts; //@audit gas: use suggested _vestingParams
111:         if (block.timestamp >= vesting[recipient].unlockEnd) { //@audit gas: use suggested _vestingParams
112:             return locked - claimed;
113:         }
114:         return
115:             ((locked * (block.timestamp - vesting[recipient].unlockBegin)) / //@audit gas: use suggested _vestingParams
116:                 (vesting[recipient].unlockEnd - //@audit gas: use suggested _vestingParams
117:                     vesting[recipient].unlockBegin)) - claimed; //@audit gas: use suggested _vestingParams
118:     }

StakedCitadelVester.vest(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (vesting[recipient])

Just like in StakedCitadelVester.claimableBalance() above:

File: StakedCitadelVester.sol
132:     function vest(
...
140:         vesting[recipient].lockedAmounts = //@audit gas: help the optimizer by declaring VestingParams storage _vestingParams = vesting[recipient];
141:             vesting[recipient].lockedAmounts + //@audit gas: use suggested _vestingParams
142:             _amount;
143:         vesting[recipient].unlockBegin = _unlockBegin; //@audit gas: use suggested _vestingParams
144:         vesting[recipient].unlockEnd = _unlockBegin + vestingDuration; //@audit gas: use suggested _vestingParams
145: 
146:         emit Vest(
147:             recipient,
148:             vesting[recipient].lockedAmounts, //@audit gas: use suggested _vestingParams
149:             _unlockBegin,
150:             vesting[recipient].unlockEnd //@audit gas: use suggested _vestingParams
151:         );
152:     }

SupplySchedule.getEpochAtTimestamp(): globalStartTimestamp should get cached

See @audit tags:

File: SupplySchedule.sol
55:     function getEpochAtTimestamp(uint256 _timestamp)
...
60:         require(
61:             globalStartTimestamp > 0, //@audit gas: should cache globalStartTimestamp (SLOAD 1)
...
64:         return (_timestamp - globalStartTimestamp) / epochLength; //@audit gas: should use cached globalStartTimestamp (SLOAD 2)

SupplySchedule.getMintable(): L105-L110 should be unchecked due to L95 and L99-L101

See @audit tags:

File: SupplySchedule.sol
94:         require(
95:             block.timestamp > lastMintTimestamp,
96:             "SupplySchedule: already minted up to current block"
97:         );
...
099:         if (lastMintTimestamp < cachedGlobalStartTimestamp) {
100:             lastMintTimestamp = cachedGlobalStartTimestamp;
101:         }
...
105:         uint256 startingEpoch = (lastMintTimestamp - cachedGlobalStartTimestamp) / //@audit gas: should be unchecked due to L99-L101
106:             epochLength;
107: 
108:         uint256 endingEpoch = (block.timestamp - cachedGlobalStartTimestamp) / //@audit gas: should be unchecked due to L95 and L99-L101
109:             epochLength;
110: 

SupplySchedule.getMintableDebug(): globalStartTimestamp should get cached

See @audit tags:

File: SupplySchedule.sol
178:     function getMintableDebug(uint256 lastMintTimestamp) external {
179:         require(
180:             globalStartTimestamp > 0, //@audit gas: should cache globalStartTimestamp (SLOAD 1)
...
183:         require(
184:             lastMintTimestamp > globalStartTimestamp, //@audit gas: should use cached globalStartTimestamp (SLOAD 2)
...
197:         emit log_named_uint("globalStartTimestamp", globalStartTimestamp);  //@audit gas: should use cached globalStartTimestamp (SLOAD 3)
...
200:         uint256 startingEpoch = (lastMintTimestamp - globalStartTimestamp) / //@audit gas: should use cached globalStartTimestamp (SLOAD 4)
201:             epochLength;
...
204:         uint256 endingEpoch = (block.timestamp - globalStartTimestamp) / //@audit gas: should use cached globalStartTimestamp (SLOAD 5)
...
208:         for (uint256 i = startingEpoch; i <= endingEpoch; i++) {
...
211:             uint256 epochStartTime = globalStartTimestamp + i * epochLength;  //@audit gas: should use cached globalStartTimestamp (SLOAD 6 + iteration)
212:             uint256 epochEndTime = globalStartTimestamp + (i + 1) * epochLength; //@audit gas: should use cached globalStartTimestamp (SLOAD 7 + iteration)

SupplySchedule.getMintableDebug(): L200-L205 should be unchecked due to L184 and L188

File: SupplySchedule.sol
178:     function getMintableDebug(uint256 lastMintTimestamp) external {
...
183:         require(
184:             lastMintTimestamp > globalStartTimestamp, // gas: should use cached globalStartTimestamp (SLOAD 2)
185:             "SupplySchedule: attempting to mint before start block"
186:         );
187:         require(
188:             block.timestamp > lastMintTimestamp,
189:             "SupplySchedule: already minted up to current block"
190:         );
...
200:         uint256 startingEpoch = (lastMintTimestamp - globalStartTimestamp) / // @audit gas: should be unchecked due to L184
201:             epochLength;
202:         emit log_named_uint("startingEpoch", startingEpoch);
203: 
204:         uint256 endingEpoch = (block.timestamp - globalStartTimestamp) / //@audit gas: should be unchecked due to L184 and L188
205:             epochLength;

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

lib/GlobalAccessControlManaged.sol:47:        bool validRoleFound = false;
lib/GlobalAccessControlManaged.sol:48:        for (uint256 i = 0; i < roles.length; i++) {
CitadelMinter.sol:152:        for (uint256 i = 0; i < numPools; i++) {
CitadelMinter.sol:180:        uint256 lockingAmount = 0;
CitadelMinter.sol:181:        uint256 stakingAmount = 0;
CitadelMinter.sol:182:        uint256 fundingAmount = 0;
Funding.sol:283:        citadelPriceFlag = false;
MedianOracle.sol:160:        uint256 size = 0;
MedianOracle.sol:164:        for (uint256 i = 0; i < reportsCount; i++) {
MedianOracle.sol:226:        for (uint256 i = 0; i < providers.length; i++) {
StakedCitadelLocker.sol:93:    address public boostPayment = address(0);
StakedCitadelLocker.sol:94:    uint256 public maximumBoostPayment = 0;
StakedCitadelLocker.sol:96:    uint256 public nextMaximumBoostPayment = 0;
StakedCitadelLocker.sol:104:    address public stakingProxy = address(0);
StakedCitadelLocker.sol:112:    bool public isShutdown = false;
StakedCitadelLocker.sol:267:        for (uint256 i = 0; i < userRewards.length; i++) {
StakedCitadelLocker.sol:423:        uint256 min = 0;
StakedCitadelLocker.sol:428:        for (uint256 i = 0; i < 128; i++) {
StakedCitadelLocker.sol:634:        uint256 reward = 0;
StakedCitadelLocker.sol:838:            for (uint i = 0; i < rewardTokens.length; i++) {
SupplySchedule.sol:103:        uint256 mintable = 0;
SupplySchedule.sol:192:        uint256 mintable = 0;

I suggest removing explicit initializations for default values.

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

interfaces/convex/BoringMath.sol:20:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:102:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:122:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:142:        require(b > 0, "BoringMath: division by zero");
CitadelMinter.sol:343:        require(length > 0, "CitadelMinter: no funding pools");
Funding.sol:170:        require(_assetAmountIn > 0, "_assetAmountIn must not be 0");
Funding.sol:322:        require(amount > 0, "nothing to sweep");
Funding.sol:340:        require(amount > 0, "nothing to claim");
Funding.sol:424:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
Funding.sol:452:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
KnightingRound.sol:125:            _saleDuration > 0,
KnightingRound.sol:129:            _tokenOutPrice > 0,
KnightingRound.sol:172:        require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
KnightingRound.sol:215:        require(tokenOutAmount_ > 0, "nothing to claim");
KnightingRound.sol:313:            _saleDuration > 0,
KnightingRound.sol:332:            _tokenOutPrice > 0,
KnightingRound.sol:411:        require(amount > 0, "nothing to sweep");
MedianOracle.sol:69:        require(minimumProviders_ > 0);
MedianOracle.sol:109:        require(minimumProviders_ > 0);
MedianOracle.sol:123:        require(timestamps[0] > 0);
MedianOracle.sol:143:        require (providerReports[providerAddress][0].timestamp > 0);
StakedCitadelLocker.sol:178:        require(rewardData[_rewardsToken].lastUpdateTime > 0);
StakedCitadelLocker.sol:526:        require(_amount > 0, "Cannot stake 0");
StakedCitadelLocker.sol:681:        require(locked > 0, "no exp locks");
StakedCitadelLocker.sol:813:        require(_reward > 0, "No reward");
StakedCitadelVester.sol:138:        require(_amount > 0, "StakedCitadelVester: cannot vest 0");
SupplySchedule.sol:61:            globalStartTimestamp > 0,
SupplySchedule.sol:91:            cachedGlobalStartTimestamp > 0,
SupplySchedule.sol:180:            globalStartTimestamp > 0,

Also, please enable the Optimizer.

>= is cheaper than >

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

I suggest using >= instead of > to avoid some opcodes here:

interfaces/convex/MathUtil.sol:12:        return a < b ? a : b;

Shift Right instead of Dividing by 2

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

StakedCitadelLocker.sol:431:            uint256 mid = (min + max + 1) / 2;

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

lib/GlobalAccessControlManaged.sol:48:        for (uint256 i = 0; i < roles.length; i++) {
StakedCitadelLocker.sol:267:        for (uint256 i = 0; i < userRewards.length; i++) {
StakedCitadelLocker.sol:459:        for (uint i = nextUnlockIndex; i < locks.length; i++) {
StakedCitadelLocker.sol:777:        for (uint i; i < rewardTokens.length; i++) {
StakedCitadelLocker.sol:838:            for (uint i = 0; i < rewardTokens.length; i++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

lib/GlobalAccessControlManaged.sol:48:        for (uint256 i = 0; i < roles.length; i++) {
CitadelMinter.sol:152:        for (uint256 i = 0; i < numPools; i++) {
MedianOracle.sol:164:        for (uint256 i = 0; i < reportsCount; i++) {
MedianOracle.sol:226:        for (uint256 i = 0; i < providers.length; i++) {
StakedCitadelLocker.sol:267:        for (uint256 i = 0; i < userRewards.length; i++) {
StakedCitadelLocker.sol:296:        for (uint i = nextUnlockIndex; i < locksLength; i++) {
StakedCitadelLocker.sol:428:        for (uint256 i = 0; i < 128; i++) {
StakedCitadelLocker.sol:459:        for (uint i = nextUnlockIndex; i < locks.length; i++) {
StakedCitadelLocker.sol:465:                idx++;
StakedCitadelLocker.sol:659:            for (uint i = nextUnlockIndex; i < length; i++) {
StakedCitadelLocker.sol:676:                nextUnlockIndex++;
StakedCitadelLocker.sol:777:        for (uint i; i < rewardTokens.length; i++) {
StakedCitadelLocker.sol:838:            for (uint i = 0; i < rewardTokens.length; i++) {
SupplySchedule.sol:208:        for (uint256 i = startingEpoch; i <= endingEpoch; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

This is already done here:

CitadelMinter.sol:344:        for (uint256 i; i < length; ++i) {

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

lib/GlobalAccessControlManaged.sol:48:        for (uint256 i = 0; i < roles.length; i++) {
CitadelMinter.sol:152:        for (uint256 i = 0; i < numPools; i++) {
CitadelMinter.sol:344:        for (uint256 i; i < length; ++i) {
SupplySchedule.sol:208:        for (uint256 i = startingEpoch; i <= endingEpoch; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

This is already done here:

SupplySchedule.sol:122:            unchecked { ++i; }

Consider making some constants as non-public to save gas

Reducing from public to private or internal can save gas when a constant isn't used outside of its contract. I suggest changing the visibility from public to internal or private here:

lib/GlobalAccessControlManaged.sol:15:    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
lib/GlobalAccessControlManaged.sol:16:    bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE");
CitadelMinter.sol:30:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
CitadelMinter.sol:32:    bytes32 public constant POLICY_OPERATIONS_ROLE =
CitadelToken.sol:9:    bytes32 public constant CITADEL_MINTER_ROLE =
Funding.sol:21:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
Funding.sol:23:    bytes32 public constant POLICY_OPERATIONS_ROLE =
Funding.sol:25:    bytes32 public constant TREASURY_OPERATIONS_ROLE = keccak256("TREASURY_OPERATIONS_ROLE");
Funding.sol:26:    bytes32 public constant TREASURY_VAULT_ROLE =
Funding.sol:28:    bytes32 public constant KEEPER_ROLE = keccak256("KEEPER_ROLE");
Funding.sol:30:    uint256 public constant MAX_BPS = 10000;
GlobalAccessControl.sol:25:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
GlobalAccessControl.sol:27:    bytes32 public constant TREASURY_GOVERNANCE_ROLE =
GlobalAccessControl.sol:30:    bytes32 public constant TECH_OPERATIONS_ROLE =
GlobalAccessControl.sol:32:    bytes32 public constant POLICY_OPERATIONS_ROLE =
GlobalAccessControl.sol:34:    bytes32 public constant TREASURY_OPERATIONS_ROLE =
GlobalAccessControl.sol:37:    bytes32 public constant KEEPER_ROLE = keccak256("KEEPER_ROLE");
GlobalAccessControl.sol:39:    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
GlobalAccessControl.sol:40:    bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE");
GlobalAccessControl.sol:42:    bytes32 public constant BLOCKLIST_MANAGER_ROLE =
GlobalAccessControl.sol:44:    bytes32 public constant BLOCKLISTED_ROLE = keccak256("BLOCKLISTED_ROLE");
GlobalAccessControl.sol:46:    bytes32 public constant CITADEL_MINTER_ROLE =
KnightingRound.sol:19:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
KnightingRound.sol:21:    bytes32 public constant TREASURY_GOVERNANCE_ROLE =
KnightingRound.sol:24:    bytes32 public constant TECH_OPERATIONS_ROLE =
KnightingRound.sol:26:    bytes32 public constant TREASURY_OPERATIONS_ROLE =
MedianOracle.sol:53:    uint256 private constant MAX_REPORT_EXPIRATION_TIME = 520 weeks;
StakedCitadel.sol:112:    uint256 public constant MAX_BPS = 10_000;
StakedCitadel.sol:113:    uint256 public constant SECS_PER_YEAR = 31_556_952; // 365.2425 days
StakedCitadel.sol:115:    uint256 public constant WITHDRAWAL_FEE_HARD_CAP = 200; // Never higher than 2%
StakedCitadel.sol:116:    uint256 public constant PERFORMANCE_FEE_HARD_CAP = 3_000; // Never higher than 30% // 30% maximum performance fee // We usually do 20, so this is insanely high already
StakedCitadel.sol:117:    uint256 public constant MANAGEMENT_FEE_HARD_CAP = 200; // Never higher than 2%
StakedCitadelLocker.sol:70:    uint256 public constant rewardsDuration = 86400; // 1 day
StakedCitadelLocker.sol:73:    uint256 public constant lockDuration = rewardsDuration * 7 * 21; // 21 weeks
StakedCitadelLocker.sol:98:    uint256 public constant denominator = 10000;
StakedCitadelLocker.sol:105:    uint256 public constant stakeOffsetOnLock = 500; //allow broader range for staking when depositing
StakedCitadelVester.sol:20:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
StakedCitadelVester.sol:34:    uint256 public constant INITIAL_VESTING_DURATION = 86400 * 21; // 21 days of vesting
SupplySchedule.sol:22:    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
SupplySchedule.sol:25:    uint256 public constant epochLength = 21 days;

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

lib/GlobalAccessControlManaged.sol:64:            "GAC: invalid-caller-role-or-address"
lib/SafeERC20.sol:57:            "SafeERC20: approve from non-zero to non-zero allowance"
lib/SafeERC20.sol:78:            require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
lib/SafeERC20.sol:98:            require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
CitadelMinter.sol:301:            "CitadelMinter: Sum of propvalues must be 10000 bps"
CitadelMinter.sol:321:            "CitadelMinter: last mint timestamp already initialized"
CitadelMinter.sol:328:            "CitadelMinter: supply schedule start not initialized"
CitadelMinter.sol:370:            "CitadelMinter: funding pool does not exist for removal"
CitadelMinter.sol:377:            "CitadelMinter: funding pool already exists"
Funding.sol:148:            "Funding: citadel price from oracle flagged and pending review"
Funding.sol:298:            "cannot decrease cap below global sum of assets in"
Funding.sol:325:            "cannot sweep funding asset, use claimAssetToTreasury()"
Funding.sol:390:            "Funding: sale recipient should not be zero"
GlobalAccessControl.sol:118:            "Role string and role do not match"
KnightingRound.sol:122:            "KnightingRound: start date may not be in the past"
KnightingRound.sol:126:            "KnightingRound: the sale duration must not be zero"
KnightingRound.sol:130:            "KnightingRound: the price must not be zero"
KnightingRound.sol:134:            "KnightingRound: sale recipient should not be zero"
KnightingRound.sol:273:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:277:            "KnightingRound: not enough balance"
KnightingRound.sol:295:            "KnightingRound: start date may not be in the past"
KnightingRound.sol:297:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:314:            "KnightingRound: the sale duration must not be zero"
KnightingRound.sol:316:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:333:            "KnightingRound: the price must not be zero"
KnightingRound.sol:351:            "KnightingRound: sale recipient should not be zero"
KnightingRound.sol:384:        require(!finalized, "KnightingRound: already finalized");
StakedCitadel.sol:192:            "performanceFeeGovernance too high"
StakedCitadel.sol:196:            "performanceFeeStrategist too high"
StakedCitadel.sol:508:                "Please withdrawToVault before changing strat"
StakedCitadel.sol:537:            "performanceFeeStrategist too high"
StakedCitadel.sol:632:            "Excessive strategist performance fee"
StakedCitadel.sol:652:            "Excessive governance performance fee"
StakedCitadelVester.sol:137:        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
StakedCitadelVester.sol:138:        require(_amount > 0, "StakedCitadelVester: cannot vest 0");
SupplySchedule.sol:62:            "SupplySchedule: minting not started"
SupplySchedule.sol:92:            "SupplySchedule: minting not started"
SupplySchedule.sol:96:            "SupplySchedule: already minted up to current block"
SupplySchedule.sol:139:            "SupplySchedule: minting already started"
SupplySchedule.sol:143:            "SupplySchedule: minting must start at or after current time"
SupplySchedule.sol:157:            "SupplySchedule: rate already set for given epoch"
SupplySchedule.sol:181:            "SupplySchedule: minting not started"
SupplySchedule.sol:185:            "SupplySchedule: attempting to mint before start block"
SupplySchedule.sol:189:            "SupplySchedule: already minted up to current block"
SupplySchedule.sol:227:                "total mintable after this iteration", 

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

interfaces/convex/BoringMath.sol:8:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:12:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:16:        require(b == 0 || (c = a * b) / b == a, "BoringMath: Mul Overflow");
interfaces/convex/BoringMath.sol:20:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:25:        require(a <= type(uint128).max, "BoringMath: uint128 Overflow");
interfaces/convex/BoringMath.sol:30:        require(a <= type(uint64).max, "BoringMath: uint64 Overflow");
interfaces/convex/BoringMath.sol:35:        require(a <= type(uint32).max, "BoringMath: uint32 Overflow");
interfaces/convex/BoringMath.sol:40:        require(a <= type(uint40).max, "BoringMath: uint40 Overflow");
interfaces/convex/BoringMath.sol:45:        require(a <= type(uint112).max, "BoringMath: uint112 Overflow");
interfaces/convex/BoringMath.sol:50:        require(a <= type(uint224).max, "BoringMath: uint224 Overflow");
interfaces/convex/BoringMath.sol:55:        require(a <= type(uint208).max, "BoringMath: uint208 Overflow");
interfaces/convex/BoringMath.sol:60:        require(a <= type(uint216).max, "BoringMath: uint216 Overflow");
interfaces/convex/BoringMath.sol:68:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:72:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:79:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:83:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:90:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:94:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:98:        require(b == 0 || (c = a * b) / b == a, "BoringMath: Mul Overflow");
interfaces/convex/BoringMath.sol:102:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:110:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:114:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:118:        require(b == 0 || (c = a * b) / b == a, "BoringMath: Mul Overflow");
interfaces/convex/BoringMath.sol:122:        require(b > 0, "BoringMath: division by zero");
interfaces/convex/BoringMath.sol:130:        require((c = a + b) >= b, "BoringMath: Add Overflow");
interfaces/convex/BoringMath.sol:134:        require((c = a - b) <= a, "BoringMath: Underflow");
interfaces/convex/BoringMath.sol:138:        require(b == 0 || (c = a * b) / b == a, "BoringMath: Mul Overflow");
interfaces/convex/BoringMath.sol:142:        require(b > 0, "BoringMath: division by zero");
lib/GlobalAccessControlManaged.sol:41:        require(gac.hasRole(role, msg.sender), "GAC: invalid-caller-role");
lib/GlobalAccessControlManaged.sol:55:        require(validRoleFound, "GAC: invalid-caller-role");
lib/GlobalAccessControlManaged.sol:62:        require(
lib/GlobalAccessControlManaged.sol:71:        require(!gac.paused(), "global-paused");
lib/GlobalAccessControlManaged.sol:72:        require(!paused(), "local-paused");
lib/GlobalAccessControlManaged.sol:81:        require(gac.hasRole(PAUSER_ROLE, msg.sender));
lib/GlobalAccessControlManaged.sol:86:        require(gac.hasRole(UNPAUSER_ROLE, msg.sender));
lib/SafeERC20.sol:55:        require(
lib/SafeERC20.sol:78:            require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
lib/SafeERC20.sol:98:            require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
lib/SettAccessControl.sol:16:        require(msg.sender == governance, "onlyGovernance");
lib/SettAccessControl.sol:20:        require(
lib/SettAccessControl.sol:27:        require(
CitadelMinter.sol:116:        require(_gac != address(0), "address 0 invalid");
CitadelMinter.sol:117:        require(_citadelToken != address(0), "address 0 invalid");
CitadelMinter.sol:118:        require(_xCitadel != address(0), "address 0 invalid");
CitadelMinter.sol:119:        require(_xCitadelLocker != address(0), "address 0 invalid");
CitadelMinter.sol:120:        require(_supplySchedule != address(0), "address 0 invalid");
CitadelMinter.sol:256:        require(
CitadelMinter.sol:272:            require(_weight <= 10000, "exceed max funding pool weight");
CitadelMinter.sol:299:        require(
CitadelMinter.sol:319:        require(
CitadelMinter.sol:326:        require(
CitadelMinter.sol:343:        require(length > 0, "CitadelMinter: no funding pools");
CitadelMinter.sol:368:        require(
CitadelMinter.sol:375:        require(
Funding.sol:80:        require(
Funding.sol:113:        require(
Funding.sol:117:        require(
Funding.sol:146:        require(
Funding.sol:170:        require(_assetAmountIn > 0, "_assetAmountIn must not be 0");
Funding.sol:171:        require(
Funding.sol:178:        require(citadelAmount_ >= _minCitadelOut, "minCitadelOut");
Funding.sol:270:        require(_discount >= funding.minDiscount, "discount < minDiscount");
Funding.sol:271:        require(_discount <= funding.maxDiscount, "discount > maxDiscount");
Funding.sol:296:        require(
Funding.sol:322:        require(amount > 0, "nothing to sweep");
Funding.sol:323:        require(
Funding.sol:340:        require(amount > 0, "nothing to claim");
Funding.sol:361:        require(_maxDiscount < MAX_BPS , "maxDiscount >= MAX_BPS");
Funding.sol:388:        require(
Funding.sol:424:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
Funding.sol:425:        require(_valid, "oracle data must be valid");
Funding.sol:452:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
GlobalAccessControl.sol:95:        require(hasRole(PAUSER_ROLE, msg.sender), "PAUSER_ROLE");
GlobalAccessControl.sol:100:        require(hasRole(UNPAUSER_ROLE, msg.sender), "UNPAUSER_ROLE");
GlobalAccessControl.sol:112:        require(
GlobalAccessControl.sol:116:        require(
KnightingRound.sol:120:        require(
KnightingRound.sol:124:        require(
KnightingRound.sol:128:        require(
KnightingRound.sol:132:        require(
KnightingRound.sol:167:        require(saleStart <= block.timestamp, "KnightingRound: not started");
KnightingRound.sol:168:        require(
KnightingRound.sol:172:        require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
KnightingRound.sol:173:        require(
KnightingRound.sol:179:            require(guestlist.authorized(msg.sender, _proof), "not authorized");
KnightingRound.sol:185:            require(
KnightingRound.sol:210:        require(finalized, "sale not finalized");
KnightingRound.sol:211:        require(!hasClaimed[msg.sender], "already claimed");
KnightingRound.sol:215:        require(tokenOutAmount_ > 0, "nothing to claim");
KnightingRound.sol:273:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:274:        require(saleEnded(), "KnightingRound: not finished");
KnightingRound.sol:275:        require(
KnightingRound.sol:293:        require(
KnightingRound.sol:297:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:312:        require(
KnightingRound.sol:316:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:331:        require(
KnightingRound.sol:349:        require(
KnightingRound.sol:384:        require(!finalized, "KnightingRound: already finalized");
KnightingRound.sol:411:        require(amount > 0, "nothing to sweep");
StakedCitadel.sol:180:        require(_token != address(0)); // dev: _token address should not be zero
StakedCitadel.sol:181:        require(_governance != address(0)); // dev: _governance address should not be zero
StakedCitadel.sol:182:        require(_keeper != address(0)); // dev: _keeper address should not be zero
StakedCitadel.sol:183:        require(_guardian != address(0)); // dev: _guardian address should not be zero
StakedCitadel.sol:184:        require(_treasury != address(0)); // dev: _treasury address should not be zero
StakedCitadel.sol:185:        require(_strategist != address(0)); // dev: _strategist address should not be zero
StakedCitadel.sol:186:        require(_badgerTree != address(0)); // dev: _badgerTree address should not be zero
StakedCitadel.sol:187:        require(_vesting != address(0)); // dev: _vesting address should not be zero
StakedCitadel.sol:190:        require(
StakedCitadel.sol:194:        require(
StakedCitadel.sol:198:        require(
StakedCitadel.sol:202:        require(
StakedCitadel.sol:262:        require(
StakedCitadel.sol:270:        require(msg.sender == strategy, "onlyStrategy");
StakedCitadel.sol:441:        require(address(token) != _token, "No want");
StakedCitadel.sol:487:        require(_treasury != address(0), "Address 0");
StakedCitadel.sol:502:        require(_strategy != address(0), "Address 0");
StakedCitadel.sol:506:            require(
StakedCitadel.sol:523:        require(_fees <= WITHDRAWAL_FEE_HARD_CAP, "withdrawalFee too high");
StakedCitadel.sol:535:        require(
StakedCitadel.sol:550:        require(_fees <= MANAGEMENT_FEE_HARD_CAP, "managementFee too high");
StakedCitadel.sol:562:        require(_guardian != address(0), "Address cannot be 0x0");
StakedCitadel.sol:574:        require(_vesting != address(0), "Address cannot be 0x0");
StakedCitadel.sol:588:        require(_newToEarnBps <= MAX_BPS, "toEarnBps should be <= MAX_BPS");
StakedCitadel.sol:613:        require(_withdrawalFee <= maxWithdrawalFee, "Excessive withdrawal fee");
StakedCitadel.sol:630:        require(
StakedCitadel.sol:650:        require(
StakedCitadel.sol:666:        require(_fees <= maxManagementFee, "Excessive management fee");
StakedCitadel.sol:700:        require(address(token) != _token, "No want");
StakedCitadel.sol:718:        require(!pausedDeposit, "pausedDeposit"); // dev: deposits are paused, we don't earn as well
StakedCitadel.sol:768:        require(_recipient != address(0), "Address 0");
StakedCitadel.sol:769:        require(_amount != 0, "Amount 0");
StakedCitadel.sol:770:        require(!pausedDeposit, "pausedDeposit"); // dev: deposits are paused
StakedCitadel.sol:794:            require(
StakedCitadel.sol:809:        require(_shares != 0, "0 Shares");
StakedCitadelVester.sol:64:        require(_vestingToken != address(0), "Address zero invalid");
StakedCitadelVester.sol:65:        require(_vault != address(0), "Address zero invalid");
StakedCitadelVester.sol:137:        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
StakedCitadelVester.sol:138:        require(_amount > 0, "StakedCitadelVester: cannot vest 0");
SupplySchedule.sol:60:        require(
SupplySchedule.sol:90:        require(
SupplySchedule.sol:94:        require(
SupplySchedule.sol:137:        require(
SupplySchedule.sol:141:        require(
SupplySchedule.sol:155:        require(
SupplySchedule.sol:179:        require(
SupplySchedule.sol:183:        require(
SupplySchedule.sol:187:        require(

I suggest replacing revert strings with custom errors.