QA Report

[code423n4]

1. getPricePerFullShare is correct only for tokens with 18 decimals (low)

Impact

The price returned by getPricePerFullShare() will have incorrect magnitude if contract token has decimals other than 18.

Keeping it low severity as Citadel and xCitadel have 18 decimals, but such a hard code in the contract that can deal with an arbitrary token isn't correct.

Proof of Concept

If there is no supply getStakedCitadelAmountOut() will return 1e18, if there is supply it will return a value with token's decimals:

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

getStakedCitadelAmountOut() uses getPricePerFullShare(), so can produce results of a wrong magnitude if used with a non-18 decimals token:

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

Recommended Mitigation Steps

Scale the getPricePerFullShare() result

2. vestingDuration can be set to zero breaking claimableBalance logic (low)

Impact

claimableBalance will throw low level error.

Proof of Concept

In order to keep StakedCitadelVester.claimableBalance working the vestingDuration should be kept positive:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L116-L117

But vestingDuration can be set to zero:

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

Recommended Mitigation Steps

Consider introducing min and max duration checks (say 0 and 20 years)

3. Funding CTDL price comments and name are wrong, being inverted (low)

Impact

It is highly misleading at the moment, so it's hard to understand the logic of the contract.

Proof of Concept

The prices are CTDL per ASSET, while the comments state it otherwise (ASSET per CTDL):

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

Prices are CTDL/$ASSET, CTDL per ASSET:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/docs/oracles.md

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/test/BaseFixture.sol#L199

Also, citadelPriceInAsset name in Funding is misleading, it is AssetPriceinCitadel.

Recommended Mitigation Steps

Update the name and comments according to the existing logic.

4. Funding.deposit's _minCitadelOut argument has wrong description (low)

Impact

The end effect is that _minCitadelOut argument has no description.

Proof of Concept

The arg description comment is not correct here:

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

Recommended Mitigation Steps

Replace the line with slippage checking description of the argument

5. SettAccessControl's role setting should be two-step actions (non-critical)

Impact

One step is errror prone, and it's used even for governance.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/SettAccessControl.sol#L35-L54

Recommended Mitigation Steps

Introduce two-step propose-set logic

6. Strategist address check is redundant (non-critical)

Proof of Concept

Strategist address cannot be zero as it is set once on initialization to a non-zero address and can't be modified thereafter:

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

So the address part of the check is redundant:

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

Recommended Mitigation Steps

Remove the check to simplify

7. initializeLastMintTimestamp is redundant (non-critical)

Proof of Concept

getMintable resets input lastMintTimestamp on seeing that it's too low (the only case is lastMintTimestamp being zero when it's not initialized yet), so effectively what initializeLastMintTimestamp doing is already fully achieved on getMintable first run.

In the end of mintAndDistribute, which is the only place getMintable is called, lastMintTimestamp is set to current timestamp, and all the subsequent runs are incrementally using current time only.

Nothing is achieved if initializeLastMintTimestamp be run first instead, so this way initializeLastMintTimestamp isn't needed and can be safely removed.

Recommended Mitigation Steps

Remove the function

8. getMintable breaks if called in the same block as setMintingStart (non-critical)

Impact

When getMintable is called within the same block as cachedGlobalStartTimestamp is set there will be low level subtraction error.

Proof of Concept

getMintable doesn't check that block.timestamp > cachedGlobalStartTimestamp:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L84-L101

Recommended Mitigation Steps

Consider moving L99-101 before L94:

if (lastMintTimestamp < cachedGlobalStartTimestamp) {
   lastMintTimestamp = cachedGlobalStartTimestamp;
}

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L99-L101

to be before block.timestamp > lastMintTimestamp require:

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

9. Debug functions left in the code (non-critical)

Impact

Unused debug functions bloat production code. Using them can bring in various malfunctions. The best way to ensure there is no usage left is to remove them.

Proof of Concept

SupplySchedule's _setEpochRates, getMintableDebug to be removed on release:

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

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

_setEpochRates is used on init:

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

getMintableDebug is referenced by ISupplySchedule:

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

Recommended Mitigation Steps

Consider updating the initialize(), ISupplySchedule and removing _setEpochRates, getMintableDebug

10. Funding, GlobalAccessControl, KnightingRound have open TODOs (non-critical)

Impact

TODOs clutter production code as release fixes the implementation.

Proof of Concept

Funding:

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

Recommended Mitigation Steps

Consider removing TODOs from production code.

11. _onlyAuthorizedActors name is misleading (non-critical)

Impact

Code is less readable and transparent.

Proof of Concept

_onlyAuthorizedActors is an abstract name with GovernanceOrKeeper implementation:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/SettAccessControl.sol#L26

Recommended Mitigation Steps

Consider renaming to _onlyGovernanceOrKeeper in order to match the logic.

12. MedianOracle compiler has to be updated (non-critical)

Impact

Various malfunctions are possible on mixing of compiling versions as the amount of breaking changes is substantial.

For example, an operational mistake can lead it to be accidentally deployed with never version without testing.

Proof of Concept

MedianOracle use 0.4.24 compiler:

https://github.com/ampleforth/market-oracle/blob/master/contracts/MedianOracle.sol#L1

The rest of the system is 0.8.12.

Recommended Mitigation Steps

Consider testing MedianOracle with 0.8.12 compiler and updating it so it be aligned with the rest of the system.