Badger Citadel contest
Findings & Analysis Report

2022-07-08

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
High Risk Findings (3)
Medium Risk Findings (5)
Low Risk and Non-Critical Issues
Gas Optimizations
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit contest outlined in this document, C4 conducted an analysis of the Badger Citadel smart contract system written in Solidity. The audit contest took place between April 14—April 20 2022.

Wardens

79 Wardens contributed reports to the Badger Citadel contest:

IllIllI
georgypetrov
cmichel
cccz
VAD37
0xDjango
danb
hyh
berndartmueller
reassor
TrungOre
rayn
minhquanym
wuwe1
Ruhum
shenwilly
kyliek
gs8nrv
gzeon
m9800
0xBug
pedroais
Dravee
CertoraInc (egjlmn1, OriDabush, ItayG, and shakedwinder)
horsefacts
scaraven
sorrynotsorry
MaratCerby
joestakey
TomFrenchBlockchain
remora
ilan
csanuragjain
defsec
rfa
TerrierLover
fatherOfBlocks
0xkatana
robee
ellahi
0x1f8b
kenta
securerodd
tchkvsky
Funen
kebabsec (okkothejawa and FlameHorizon)
SolidityScan (cyberboy and zombie)
teryanarmen
z3s
0v3rf10w
jah
oyc_109
delfin454000
Hawkeye (0xwags and 0xmint)
hubble (ksk2345 and shri4net)
AmitN
dipp
p_crypt0
peritoflores
Picodes
Jujic
Yiko
Tomio
saian
0xAsm0d3us
0xNazgul
joshie
slywaters
Cityscape
simon135
bae11
nahnah

This contest was judged by Jack the Pug.

Final report assembled by itsmetechjay.

Summary

The C4 analysis yielded an aggregated total of 8 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 5 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 58 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 48 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Badger Citadel contest repository, and is composed of 8 smart contracts written in the Solidity programming language and includes 2,339 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (3)

[H-01] StakedCitadel doesn’t use correct balance for internal accounting

Submitted by Ruhum, also found by cccz, wuwe1, VAD37, TrungOre, shenwilly, minhquanym, kyliek, danb, gs8nrv, and rayn

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L291-L295

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L772-L776

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881-L893

Impact

The StakedCitadel contract’s balance() function is supposed to return the balance of the vault + the balance of the strategy. But, it only returns the balance of the vault. The balance is used to determine the number of shares that should be minted when depositing funds into the vault and the number of shares that should be burned when withdrawing funds from it.

Since most of the funds will be located in the strategy, the vault’s balance will be very low. Some of the issues that arise from this:

You can’t deposit to a vault that already minted shares but has no balance of the underlying token:

fresh vault with 0 funds and 0 shares
Alice deposits 10 tokens. She receives 10 shares back (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L887-L888)
Vault’s tokens are deposited into the strategy (now balance == 0 and totalSupply == 10)
Bob tries to deposit but the transaction fails because the contract tries to divide by zero: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L890 (pool == balance())

You get more shares than you should

fresh vault with 0 funds and 0 shares
Alice deposits 10 tokens. She receives 10 shares back (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L887-L888)
Vault’s tokens are deposited into the strategy (now balance == 0 and totalSupply == 10)
Bob now first transfers 1 token to the vault so that the balance is now 1 instead of 0.
Bob deposits 5 tokens. He receives 5 * 10 / 1 == 50 shares: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L890

Now, the vault received 15 tokens. 10 from Alice and 5 from Bob. But Alice only has 10 shares while Bob has 50. Thus, Bob can withdraw more tokens than he should be able to.

It simply breaks the whole accounting of the vault.

Proof of Concept

The comment says that it should be vault’s + strategy’s balance: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L291-L295

Here’s another vault from the badger team where the function is implemented correctly: https://github.com/Badger-Finance/badger-vaults-1.5/blob/main/contracts/Vault.sol#L262

Recommended Mitigation Steps

Add the strategy’s balance to the return value of thebalance() function like here.

GalloDaSballo (BadgerDAO) confirmed and commented:

Agree balance must have been changed by mistake or perhaps earn should not transfer to a strategy either would work

[H-02] StakedCitadel: wrong setupVesting function name

Submitted by cccz, also found by TrungOre, wuwe1, reassor, 0xBug, georgypetrov, 0xDjango, scaraven, horsefacts, berndartmueller, CertoraInc, rayn, m9800, pedroais, and VAD37

In the \_withdraw function of the StakedCitadel contract, the setupVesting function of vesting is called, while in the StakedCitadelVester contract, the function name is vest, which will cause the _withdraw function to fail, so that the user cannot withdraw the tokens.

        IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp);
        token.safeTransfer(vesting, _amount);
        ...
    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
        );
    }

Proof of Concept

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

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/interfaces/citadel/IVesting.sol#L5

Recommended Mitigation Steps

Use the correct function name

interface IVesting {
    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external;
}
...
IVesting(vesting).vest(msg.sender, _amount, block.timestamp);
token.safeTransfer(vesting, _amount);

dapp-whisperer (BadgerDAO) confirmed and resolved

[H-03] StakedCitadel depositors can be attacked by the first depositor with depressing of vault token denomination

Submitted by hyh, also found by VAD37, cmichel, 0xDjango, berndartmueller, and danb

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881-L892

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L293-L295

Impact

An attacker can become the first depositor for a recently created StakedCitadel contract, providing a tiny amount of Citadel tokens by calling deposit(1) (raw values here, 1 is 1 wei, 1e18 is 1 Citadel as it has 18 decimals). Then the attacker can directly transfer, for example, 10^6*1e18 - 1 Citadel to StakedCitadel, effectively setting the cost of 1 of the vault token to be 10^6 * 1e18 Citadel. The attacker will still own 100% of the StakedCitadel’s pool being the only depositor.

All subsequent depositors will have their Citadel token investments rounded to 10^6 * 1e18, due to the lack of precision which initial tiny deposit caused, with the remainder divided between all current depositors, i.e. the subsequent depositors lose value to the attacker.

For example, if the second depositor brings in 1.9*10^6 * 1e18 Citadel, only 1 of new vault to be issued as 1.9*10^6 * 1e18 divided by 10^6 * 1e18 will yield just 1, which means that 2.9*10^6 * 1e18 total Citadel pool will be divided 50/50 between the second depositor and the attacker, as each have 1 wei of the total 2 wei of vault tokens, i.e. the depositor lost and the attacker gained 0.45*10^6 * 1e18 Citadel tokens.

As there are no penalties to exit with StakedCitadel.withdraw(), the attacker can remain staked for an arbitrary time, gathering the share of all new deposits’ remainder amounts.

Placing severity to be high as this is principal funds loss scenario for many users (most of depositors), easily executable, albeit only for the new StakedCitadel contract.

Proof of Concept

deposit() -> _depositFor() -> _mintSharesFor() call doesn’t require minimum amount and mints according to the provided amount:

deposit:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L309-L311

_depositFor:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L764-L777

_mintSharesFor:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881-L892

When StakedCitadel is new the _pool = balance() is just initially empty contract balance:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L293-L295

Any deposit lower than total attacker’s stake will be fully stolen from the depositor as 0 vault tokens will be issued in this case.

References

The issue is similar to the TOB-YEARN-003 one of the Trail of Bits audit of Yearn Finance:

https://github.com/yearn/yearn-security/tree/master/audits/20210719_ToB_yearn_vaultsv2

Recommended Mitigation Steps

A minimum for deposit value can drastically reduce the economic viability of the attack. I.e. deposit() -> ... can require each amount to surpass the threshold, and then an attacker would have to provide too big direct investment to capture any meaningful share of the subsequent deposits.

An alternative is to require only the first depositor to freeze big enough initial amount of liquidity. This approach has been used long enough by various projects, for example in Uniswap V2:

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

GalloDaSballo (BadgerDAO) acknowledged, disagreed with severity and commented:

Disagree with the dramatic effect the warden is implying.

Agree with the finding as this is a property of vault based systems

Also worth noting that anyone else can still get more deposits in and get their fair share, it’s just that the first deposit would now require a deposit of at least vault.balanceOf in order to get the fair amount of shares (which at this point would be rebased to be 1 = prevBalanceOf)

jack-the-pug (judge) commented:

I believe this is a valid High even though the precondition of this attack is quite strict (the attacker has to be the 1st depositor).

The impact is not just a regular precision loss, but with the pricePerShare of the vault being manipulated to an extreme value, all regular users will lose up to the pricePerShare of the deposited amount due to huge precision loss.

Medium Risk Findings (5)

[M-01] Guaranteed citadel profit

Submitted by georgypetrov

User can sandwich mintAndDistribute function if mintable is high enough

Deposit before
Withdraw after
Take after 21 days citadels

Proof of Concept

mintAndDistribute increase a price of staking share, that allows to withdraw more than deposited. user takes part of distributed citadels, so different users have smaller profit from distribution

Recommended Mitigation Steps

Call mintAndDistribute through flashbots

GalloDaSballo (BadgerDAO) confirmed, disagreed with severity and commented:

My interpretation of the finding is that there’s no linear vesting in the way more rewards are distributed so they can be frontrun.

I have to disagree in that taking 21 days of exposure to a random token in order to gain a small sub 1% gain is probably not what I’d call a smart move.

That said, I believe the front-running finding to be valid, and while I disagree with High I believe the finding to have validity

jack-the-pug (judge) decreased severity to Medium and commented:

Downgrading to Medium as this attack vector is not economically profitable in practice (because of the 21 days vesting).

[M-02] Funding.deposit() doesn’t work if there is no discount set

Submitted by Ruhum, also found by TrungOre, MaratCerby, 0xBug, minhquanym, shenwilly, 0xDjango, remora, danb, IllIllI, pedroais, m9800, and hyh

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

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

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

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L769

Impact

The Funding contract’s deposit() function uses the getAmountOut() function to determine how many citadel tokens the user should receive for their deposit. But, if no discount is set, the function always returns 0. Now the deposit() function tries to deposit 0 tokens for the user through the StakedCitadel contract. But, that function requires the number of tokens to be != 0. The transaction reverts.

This means, that no deposits are possible. Unless there is a discount.

Proof of Concept

Funding.deposit() calls getAmountOut(): https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L177

Here’s the getAmountOut() function:

    function getAmountOut(uint256 _assetAmountIn)
        public
        view
        returns (uint256 citadelAmount_)
    {
        uint256 citadelAmountWithoutDiscount = _assetAmountIn * citadelPriceInAsset;

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

        // unless the above if block is executed, `citadelAmount_` is 0 when this line is executed.
        // 0 = 0 / x
        citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;
    }

Call to StakedCitadel.depositFor(): https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L184

require statement that makes the whole transaction revert: https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L769

Recommended Mitigation Steps

Change the getAmountOut() function to:

    function getAmountOut(uint256 _assetAmountIn)
        public
        view
        returns (uint256 citadelAmount_)
    {

        uint256 citadelAmount_ = _assetAmountIn * citadelPriceInAsset;

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

        citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;
    }

shuklaayush (BadgerDAO) confirmed

[M-03] KnightingRound tokenOutPrice changes

Submitted by reassor, also found by cccz and cmichel

Function.buy buys the tokens for whatever price is set as tokenOutPrice. This might lead to accidental collisions or front-running attacks when user is trying to buy the tokens and his transaction is being included after the transaction of changing the price of the token via setTokenOutPrice.

Scenario:

User wants to buy tokens and can see price tokenOutPrice
User likes the price and issues a transaction to buy tokens
At the same time CONTRACT_GOVERNANCE_ROLE account is increasing tokenOutPrice through setTokenOutPrice
setTokenOutPrice transaction is included before user’s buy transaction
User buys tokens with the price he was not aware of

Another variation of this attack can be performed using front-running.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add additional parameter uint256 believedPrice to KnightingRound.buy function and check if believedPrice is equal to tokenOutPrice.

GalloDaSballo (BadgerDAO) confirmed

[M-04] New vest reset `unlockBegin` of existing vest without removing vested amount

Submitted by gzeon, also found by cccz, TrungOre, minhquanym, cmichel, 0xDjango, and rayn

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

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

Impact

When vest is called by xCTDL vault, the previous amount will re-lock according to the new vesting timeline. While this is as described in L127, claimableBalance might revert due to underflow if vesting[recipient].claimedAmounts > 0 because the user will need to vest the claimedAmounts again which should not be an expected behavior as it is already vested.

Proof of Concept

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

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

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

        uint256 locked = vesting[recipient].lockedAmounts;
        uint256 claimed = vesting[recipient].claimedAmounts;
        if (block.timestamp >= vesting[recipient].unlockEnd) {
            return locked - claimed;
        }
        return
            ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin)) - claimed;

Recommended Mitigation Steps

Reset claimedAmounts on new vest

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

shuklaayush (BadgerDAO) confirmed and commented:

I think this is valid and was fixed in https://github.com/Citadel-DAO/citadel-contracts/pull/44

jack-the-pug (judge) decreased severity to Medium and commented:

I’m downgrading this to Medium as there are no funds directly at risk, but a malfunction and leak of value. The user will have to wait for a longer than expected time to claim their vested funds.

[M-05] Stale price used when `citadelPriceFlag` is cleared

Submitted by IllIllI

During the video it was explained that the policy operations team was meant to be a nimble group that could change protocol values considered to be safe. Further, it was explained that since pricing comes from an oracle, and there would have to be unusual coordination between the two to affect outcomes, the group was given the ability to clear the pricing flag to get things moving again once the price was determined to be valid

Impact

If an oracle price falls out of the valid min/max range, the citadelPriceFlag is set to true, but the out-of-bounds value is not stored. If the policy operations team calls clearCitadelPriceFlag(), the stale price from before the flag will be used. Not only is it an issue because of stale prices, but this means the policy op team now has a way to affect pricing not under the control of the oracle (i.e. no unusual coordination required to affect an outcome). Incorrect pricing leads to incorrect asset valuations, and loss of funds.

Proof of Concept

The flag is set but the price is not stored File: src/Funding.sol (lines 427-437)

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

Tools Used

Code inspection

Recommended Mitigation Steps

Always set the citadelPriceInAsset

shuklaayush (BadgerDAO) confirmed and commented:

Makes sense. It’s best to update the price even when it’s flagged

jack-the-pug (judge) commented:

This is a very good catch! citadelPriceInAsset is not updated when citadelPriceFlag is set, therefore clearing the flag will not approve the out of range price but continues with a stale price before the out of range price.

Low Risk and Non-Critical Issues

For this contest, 58 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by IllIllI received the top score from the judge.

The following wardens also submitted reports: hyh, sorrynotsorry, berndartmueller, csanuragjain, ilan, kyliek, Ruhum, joestakey, defsec, reassor, TerrierLover, TomFrenchBlockchain, CertoraInc, ellahi, robee, shenwilly, TrungOre, danb, Dravee, fatherOfBlocks, hubble, AmitN, Funen, horsefacts, kebabsec, kenta, scaraven, securerodd, tchkvsky, 0xkatana, rayn, SolidityScan, 0x1f8b, 0xDjango, cmichel, delfin454000, dipp, gs8nrv, gzeon, Hawkeye, jah, m9800, minhquanym, oyc_109, p_crypt0, peritoflores, Picodes, remora, teryanarmen, VAD37, z3s, 0v3rf10w, georgypetrov, Jujic, MaratCerby, rfa, and Yiko.

[L-01] New min/max values should be checked against the current stored value

If citadelPriceInAsset is above the new max or below the new min, the next update will likely have a similar value and immediately cause problems. The code should require that the current value falls within the new range

File: src/Funding.sol (lines 402-403)

        minCitadelPriceInAsset = _minPrice;
        maxCitadelPriceInAsset = _maxPrice;

[L-02] Loss of precision

If tokenOutPrice is less than tokenInNormalizationValue, then the amount will be zero for some amounts. The caller of getAmountOut() should revert if tokenOutAmount ends up being zero

File: src/KnightingRound.sol (lines 239-241)

        tokenOutAmount_ =
            (_tokenInAmount * tokenOutPrice) /
            tokenInNormalizationValue;

[L-03] Unsafe calls to optional ERC20 functions

decimals(), name() and symbol() are optional parts of the ERC20 specification, so there are tokens that do not implement them. It’s not safe to cast arbitrary token addresses in order to call these functions. If IERC20Metadata is to be relied on, that should be the variable type of the token variable, rather than it being address, so the compiler can verify that types correctly match, rather than this being a runtime failure. See this prior instance of this issue which was marked as Low risk. Do this to resolve the issue.

File: src/interfaces/erc20/IERC20.sol (lines 14-18)

    function name() external view returns (string memory);

    function symbol() external view returns (string memory);

    function decimals() external view returns (uint256);

File: src/KnightingRound.sol (line 148)

        tokenInNormalizationValue = 10**tokenIn.decimals();

File: src/StakedCitadel.sol (line 218)

                abi.encodePacked(_defaultNamePrefix, namedToken.name())

File: src/StakedCitadel.sol (line 226)

                abi.encodePacked(_symbolSymbolPrefix, namedToken.symbol())

[L-04] Missing checks for `address(0x0)` when assigning values to `address` state variables

File: external/StakedCitadelLocker.sol (line 186)

        stakingProxy = _staking;

File: src/lib/SettAccessControl.sol (line 39)

        strategist = _strategist;

File: src/lib/SettAccessControl.sol (line 46)

        keeper = _keeper;

File: src/lib/SettAccessControl.sol (line 53)

        governance = _governance;

[L-05] `initialize` functions can be front-run

See this finding from a prior badger-dao contest for details

File: src/CitadelMinter.sol (line 109)

    function initialize(

File: src/KnightingRound.sol (line 119)

    ) external initializer {

File: src/Funding.sol (line 112)

    ) external initializer {

File: src/StakedCitadel.sol (line 179)

    ) public initializer whenNotPaused {

[L-06] `safeApprove()` is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

File: src/CitadelMinter.sol (line 133)

        IERC20Upgradeable(_citadelToken).safeApprove(_xCitadel, type(uint256).max);

File: src/CitadelMinter.sol (line 136)

        IERC20Upgradeable(_xCitadel).safeApprove(_xCitadelLocker, type(uint256).max);

File: src/Funding.sol (line 142)

        IERC20(_citadel).safeApprove(address(_xCitadel), type(uint256).max);

[L-07] Upgradeable contract is missing a `__gap[50]` storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

File: external/StakedCitadelLocker.sol (line 26)

contract StakedCitadelLocker is Initializable, ReentrancyGuardUpgradeable, OwnableUpgradeable {

File: src/CitadelMinter.sol (lines 23-25)

contract CitadelMinter is
    GlobalAccessControlManaged,
    ReentrancyGuardUpgradeable

File: src/CitadelToken.sol (line 8)

contract CitadelToken is GlobalAccessControlManaged, ERC20Upgradeable {

File: src/Funding.sol (line 17)

contract Funding is GlobalAccessControlManaged, ReentrancyGuardUpgradeable {

File: src/GlobalAccessControl.sol (lines 19-21)

contract GlobalAccessControl is
    AccessControlEnumerableUpgradeable,
    PausableUpgradeable

File: src/KnightingRound.sol (line 16)

contract KnightingRound is GlobalAccessControlManaged, ReentrancyGuardUpgradeable {

File: src/lib/GlobalAccessControlManaged.sol (line 12)

contract GlobalAccessControlManaged is PausableUpgradeable {

File: src/StakedCitadel.sol (lines 59-63)

contract StakedCitadel is
    ERC20Upgradeable,
    SettAccessControl,
    PausableUpgradeable,
    ReentrancyGuardUpgradeable

File: src/StakedCitadelVester.sol (lines 14-16)

contract StakedCitadelVester is
    GlobalAccessControlManaged,
    ReentrancyGuardUpgradeable

[L-08] Unbounded loop

If there are too many pools, the function may run out of gas while returning them. It’s best to allow for a start offset and maximum length, so data can be returned in batches that don’t run out of gas

File: src/CitadelMinter.sol (lines 143-147)

    function getFundingPoolWeights()
        external
        view
        returns (address[] memory pools, uint256[] memory weights)
    {

[N-01] Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

File: src/Funding.sol (line 15)

 * TODO: Better revert strings

File: src/Funding.sol (line 61)

    // TODO: we should conform to some interface here

File: src/Funding.sol (line 183)

        // TODO: Check gas costs. How does this relate to market buying if you do want to deposit to xCTDL?

File: src/GlobalAccessControl.sol (line 106)

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

File: src/KnightingRound.sol (line 14)

 * TODO: Better revert strings

File: src/SupplySchedule.sol (line 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] Misleading comment

The value of transferFromDisabled is never updated, let alone in an initialize() function. I don’t see any bugs related to this, but this comment makes it seem as though something was overlooked when branching.

File: src/GlobalAccessControl.sol (line 51)

    bool public transferFromDisabled; // Set to true in initialize

[N-03] Multiple definitions of an interface

These are the only two differences between IEmptyStrategy and IStrategy. IEmptyStrategy should be changed to be is IStrategy and remove the duplicate functions

File: src/interfaces/badger/IEmptyStrategy.sol (lines 12-14)

    function initialize(address vault, address want) external;

    function getName() external view returns (string memory);

[N-04] Unused file

File: src/interfaces/convex/BoringMath.sol (line 1)

// SPDX-License-Identifier: MIT

[N-05] Contract header not updated after branching

File: src/GlobalAccessControl.sol (lines 12-17)

/**
 * @title Badger Geyser
 @dev Tracks stakes and pledged tokens to be distributed, for use with
 @dev BadgerTree merkle distribution system. An arbitrary number of tokens to
 distribute can be specified.
 */

[N-06] Comment not moved when function was moved

File: src/SupplySchedule.sol (lines 52-53)

    // @dev duplicate of getMintable() with debug print added
    // @dev this function is out of scope for reviews and audits

[N-07] Comments not updated after branching

There are a lot of references to the old owner-related code. The comments should be updated to talk about the new RBAC system

File: src/KnightingRound.sol

$ grep owner src/KnightingRound.sol
     * @notice Finalize the sale after sale duration. Can only be called by owner
     * @notice Update the sale start time. Can only be called by owner
     * @notice Update sale duration. Can only be called by owner
     * @notice Modify the tokenOut price in. Can only be called by owner
     * @notice Update the `tokenIn` receipient address. Can only be called by owner
     * @notice Update the guestlist address. Can only be called by owner
     * @notice Modify the max tokenIn that this contract can take. Can only be called by owner
     * @notice Transfers out any tokens accidentally sent to the contract. Can only be called by owner

The price calulation seems inverted since this comment was first written, so it should be updated to reflect the new calculation: 2. File: src/KnightingRound.sol (line 43)

    /// eg. 1 WBTC (8 decimals) = 40,000 CTDL ==> price = 10^8 / 40,000

[N-08] Remove `include` for ds-test

Test code should not be mixed in with production code. The production version should be extended and have its functions overridden for testing purposes

File: src/SupplySchedule.sol (line 4)

import "ds-test/test.sol";

[N-09] The `nonReentrant` `modifier` should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers

File: src/CitadelMinter.sol (line 173)

        nonReentrant

File: src/CitadelMinter.sol (line 254)

        nonReentrant

File: src/CitadelMinter.sol (line 298)

    ) external onlyRole(POLICY_OPERATIONS_ROLE) gacPausable nonReentrant {

File: src/Funding.sol (line 167)

        nonReentrant

File: src/Funding.sol (line 318)

        nonReentrant

File: src/KnightingRound.sol (line 402)

    function sweep(address _token) external gacPausable nonReentrant onlyRole(TREASURY_OPERATIONS_ROLE) {

[N-10] Solidity versions greater than the current version should not be included in the pragma range

The below pragmas should be < 0.9.0, not <=

$ grep '<= 0.9.0' src/*/*/*
src/interfaces/badger/IBadgerGuestlist.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/badger/IBadgerVipGuestlist.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/badger/IEmptyStrategy.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/badger/IStrategy.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/badger/IVault.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/citadel/ICitadelToken.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/citadel/IGac.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/citadel/IStakedCitadelLocker.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/citadel/ISupplySchedule.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/citadel/IVesting.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/convex/BoringMath.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/convex/IRewardStaking.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/convex/IStakingProxy.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/convex/MathUtil.sol:pragma solidity >= 0.5.0 <= 0.9.0;
src/interfaces/erc20/IERC20.sol:pragma solidity >= 0.5.0 <= 0.9.0;

[N-11] Adding a `return` statement when the function defines a named return variable, is redundant

File: external/StakedCitadelLocker.sol (line 272)

        return userRewards;

File: external/StakedCitadelLocker.sol (line 311)

        return amount;

File: external/StakedCitadelLocker.sol (line 338)

        return amount;

File: external/StakedCitadelLocker.sol (line 399)

        return supply;

File: external/StakedCitadelLocker.sol (line 417)

        return supply;

[N-12] `require()`/`revert()` statements should have descriptive reason strings

File: external/MedianOracle.sol (line 68)

        require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);

File: external/MedianOracle.sol (line 69)

        require(minimumProviders_ > 0);

File: external/MedianOracle.sol (line 84)

        require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);

File: external/MedianOracle.sol (line 109)

        require(minimumProviders_ > 0);

File: external/MedianOracle.sol (line 123)

        require(timestamps[0] > 0);

File: external/MedianOracle.sol (line 129)

        require(timestamps[index_recent].add(reportDelaySec) <= now);

File: external/MedianOracle.sol (line 143)

        require (providerReports[providerAddress][0].timestamp > 0);

File: external/MedianOracle.sol (line 211)

        require(providerReports[provider][0].timestamp == 0);

File: external/StakedCitadelLocker.sol (line 126)

        require(_stakingToken != address(0)); // dev: _stakingToken address should not be zero

File: external/StakedCitadelLocker.sol (line 163)

        require(rewardData[_rewardsToken].lastUpdateTime == 0);

File: external/StakedCitadelLocker.sol (line 178)

        require(rewardData[_rewardsToken].lastUpdateTime > 0);

File: external/StakedCitadelLocker.sol (line 812)

        require(rewardDistributors[_rewardsToken][msg.sender]);

File: src/lib/GlobalAccessControlManaged.sol (line 81)

        require(gac.hasRole(PAUSER_ROLE, msg.sender));

File: src/lib/GlobalAccessControlManaged.sol (line 86)

        require(gac.hasRole(UNPAUSER_ROLE, msg.sender));

File: src/StakedCitadel.sol (line 180)

        require(_token != address(0)); // dev: _token address should not be zero

File: src/StakedCitadel.sol (line 181)

        require(_governance != address(0)); // dev: _governance address should not be zero

File: src/StakedCitadel.sol (line 182)

        require(_keeper != address(0)); // dev: _keeper address should not be zero

File: src/StakedCitadel.sol (line 183)

        require(_guardian != address(0)); // dev: _guardian address should not be zero

File: src/StakedCitadel.sol (line 184)

        require(_treasury != address(0)); // dev: _treasury address should not be zero

File: src/StakedCitadel.sol (line 185)

        require(_strategist != address(0)); // dev: _strategist address should not be zero

File: src/StakedCitadel.sol (line 186)

        require(_badgerTree != address(0)); // dev: _badgerTree address should not be zero

File: src/StakedCitadel.sol (line 187)

        require(_vesting != address(0)); // dev: _vesting address should not be zero

[N-13] `public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

File: external/StakedCitadelLocker.sol (lines 121-125)

    function initialize(
        address _stakingToken,
        string calldata name,
        string calldata symbol
    ) public initializer {

File: external/StakedCitadelLocker.sol (line 142)

    function decimals() public view returns (uint8) {

File: external/StakedCitadelLocker.sol (line 145)

    function name() public view returns (string memory) {

File: external/StakedCitadelLocker.sol (line 148)

    function symbol() public view returns (string memory) {

File: external/StakedCitadelLocker.sol (line 151)

    function version() public view returns(uint256){

File: external/StakedCitadelLocker.sol (lines 158-162)

    function addReward(
        address _rewardsToken,
        address _distributor,
        bool _useBoost
    ) public onlyOwner {

File: external/StakedCitadelLocker.sol (line 250)

    function lastTimeRewardApplicable(address _rewardsToken) public view returns(uint256) {

File: src/CitadelToken.sol (lines 22-26)

    function initialize(
        string memory _name,
        string memory _symbol,
        address _gac
    ) public initializer {

File: src/Funding.sol (line 223)

    function getStakedCitadelAmountOut(uint256 _assetAmountIn) public view returns (uint256 xCitadelAmount_) {

File: src/lib/GlobalAccessControlManaged.sol (lines 27-29)

    function __GlobalAccessControlManaged_init(address _globalAccessControl)
        public
        onlyInitializing

File: src/lib/SettAccessControl.sol (line 51)

    function setGovernance(address _governance) public {

File: src/StakedCitadel.sol (lines 167-179)

    function initialize(
        address _token,
        address _governance,
        address _keeper,
        address _guardian,
        address _treasury,
        address _strategist,
        address _badgerTree,
        address _vesting,
        string memory _name,
        string memory _symbol,
        uint256[4] memory _feeConfig
    ) public initializer whenNotPaused {

File: src/StakedCitadel.sol (line 284)

    function getPricePerFullShare() public view returns (uint256) {

File: src/SupplySchedule.sol (line 43)

    function initialize(address _gac) public initializer {

File: src/SupplySchedule.sol (line 79)

    function getEmissionsForCurrentEpoch() public view returns (uint256) {

[N-14] `constant`s should be defined rather than using magic numbers

File: external/StakedCitadelLocker.sol (line 131)

        _decimals = 18;

File: external/StakedCitadelLocker.sol (line 201)

        require(_max < 1500, "over max payment"); //max 15%

File: external/StakedCitadelLocker.sol (line 202)

        require(_rate < 30000, "over max rate"); //max 3x

File: external/StakedCitadelLocker.sol (line 211)

        require(_rate <= 500, "over max rate"); //max 5% per epoch

File: external/StakedCitadelLocker.sol (line 232)

                rewardData[_rewardsToken].rewardRate).mul(1e18).div(rewardData[_rewardsToken].useBoost ? boostedSupply : lockedSupply)

File: external/StakedCitadelLocker.sol (line 243)

        ).div(1e18).add(rewards[_user][_rewardsToken]);

File: external/StakedCitadelLocker.sol (line 428)

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

File: src/CitadelMinter.sol (line 272)

            require(_weight <= 10000, "exceed max funding pool weight");

File: src/StakedCitadel.sol (line 178)

        uint256[4] memory _feeConfig

File: src/StakedCitadel.sol (line 203)

            _feeConfig[3] <= MANAGEMENT_FEE_HARD_CAP,

File: src/StakedCitadel.sol (line 250)

        managementFee = _feeConfig[3];

File: src/StakedCitadel.sol (line 255)

        toEarnBps = 9_500; // initial value of toEarnBps // 95% is invested to the strategy, 5% for cheap withdrawals

File: src/SupplySchedule.sol (line 170)

        epochRate[0] = 593962000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 171)

        epochRate[1] = 591445000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 172)

        epochRate[2] = 585021000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 173)

        epochRate[3] = 574138000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 173)

        epochRate[3] = 574138000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 174)

        epochRate[4] = 558275000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 174)

        epochRate[4] = 558275000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 175)

        epochRate[5] = 536986000000000000000000 / epochLength;

File: src/SupplySchedule.sol (line 175)

        epochRate[5] = 536986000000000000000000 / epochLength;

[N-15] Numeric values having to do with time should use time units for readability

There are units for seconds, minutes, hours, days, and weeks

File: external/StakedCitadelLocker.sol (line 70)

    uint256 public constant rewardsDuration = 86400; // 1 day

File: external/StakedCitadelLocker.sol (line 70)

    uint256 public constant rewardsDuration = 86400; // 1 day

File: src/StakedCitadelVester.sol (line 34)

    uint256 public constant INITIAL_VESTING_DURATION = 86400 * 21; // 21 days of vesting

File: src/StakedCitadelVester.sol (line 34)

    uint256 public constant INITIAL_VESTING_DURATION = 86400 * 21; // 21 days of vesting

[N-16] Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated

File: src/Funding.sol (lines 21-22)

    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
        keccak256("CONTRACT_GOVERNANCE_ROLE");

seen in src/CitadelMinter.sol

File: src/Funding.sol (lines 23-24)

    bytes32 public constant POLICY_OPERATIONS_ROLE =
        keccak256("POLICY_OPERATIONS_ROLE");

seen in src/CitadelMinter.sol

File: src/GlobalAccessControl.sol (lines 25-26)

    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
        keccak256("CONTRACT_GOVERNANCE_ROLE");

seen in src/Funding.sol

File: src/GlobalAccessControl.sol (lines 32-33)

    bytes32 public constant POLICY_OPERATIONS_ROLE =
        keccak256("POLICY_OPERATIONS_ROLE");

seen in src/Funding.sol

File: src/GlobalAccessControl.sol (lines 34-35)

    bytes32 public constant TREASURY_OPERATIONS_ROLE =
        keccak256("TREASURY_OPERATIONS_ROLE");

seen in src/Funding.sol

File: src/GlobalAccessControl.sol (line 37)

    bytes32 public constant KEEPER_ROLE = keccak256("KEEPER_ROLE");

seen in src/Funding.sol

File: src/GlobalAccessControl.sol (lines 46-47)

    bytes32 public constant CITADEL_MINTER_ROLE =
        keccak256("CITADEL_MINTER_ROLE");

seen in src/CitadelToken.sol

File: src/KnightingRound.sol (lines 19-20)

    bytes32 public constant CONTRACT_GOVERNANCE_ROLE =
        keccak256("CONTRACT_GOVERNANCE_ROLE");