QA Report

[code423n4]

Title: Check transfer receiver is not 0 to avoid burned money
Severity: Low Risk

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

    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/CitadelMinter.sol#L217
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/CitadelMinter.sol#L351
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/KnightingRound.sol#L220
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L461
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L774

Title: Missing non reentrancy modifier
Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer.
Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    StakedCitadel.sol, depositAll is missing a reentrancy modifier
    StakedCitadel.sol, setPerformanceFeeGovernance is missing a reentrancy modifier
    StakedCitadel.sol, withdrawAll is missing a reentrancy modifier
    StakedCitadel.sol, setMaxManagementFee is missing a reentrancy modifier
    Funding.sol, clearCitadelPriceFlag is missing a reentrancy modifier

Title: Require with not comprehensive message
Severity: Low Risk

The following requires has a non comprehensive messages.
This is very important to add a comprehensive message for any require. Such that the user has enough
information to know the reason of failure:

    Solidity file: StakedCitadel.sol, In line 768 with Require message: Address 0
    Solidity file: StakedCitadel.sol, In line 502 with Require message: Address 0
    Solidity file: StakedCitadel.sol, In line 769 with Require message: Amount 0
    Solidity file: StakedCitadel.sol, In line 700 with Require message: No want
    Solidity file: StakedCitadel.sol, In line 809 with Require message: 0 Shares

Title: Does not validate the input fee parameter
Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    StakedCitadel.initialize (_feeConfig)
    StakedCitadel.setPerformanceFeeGovernance (_performanceFeeGovernance)
    StakedCitadel.setPerformanceFeeStrategist (_performanceFeeStrategist)
    StakedCitadel.setWithdrawalFee (_withdrawalFee)
    StakedCitadel.setMaxManagementFee (_fees)

Title: Missing commenting
Severity: Low Risk

    The following functions are missing commenting as describe below:
    
    StakedCitadel.sol, depositFor (external), parameter proof not commented
    StakedCitadel.sol, _depositForWithAuthorization (internal), parameters _recipient, _amount, proof not commented
    StakedCitadel.sol, _depositWithAuthorization (internal), parameters _amount, proof not commented
    StakedCitadel.sol, initialize (public), parameter _vesting not commented

Title: In the following public update functions no value is returned
Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0.
We assumed that after the update you return the latest new value.
(similar issue here: code-423n4/2021-10-badgerdao-findings#85).

    Funding.sol, updateCitadelPriceInAsset

Title: Treasury may be address(0)
Severity: Low Risk

Make sure the treasury is not address(0).
    
    
    StakedCitadel.sol.setTreasury _treasury
    StakedCitadel.sol.initialize _treasury

Title: Solidity compiler versions mismatch
Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Add a timelock
Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L586
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/Funding.sol#L397
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L521
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L599
    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/SupplySchedule.sol#L150

Title: Anyone can withdraw others
Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still
1) not the desired behavior
2) can be dangerous if the receiver is a smart contract
3) the receiver may not know someone withdraw him

    StakedCitadel.withdrawToVault
    StakedCitadel.setMaxWithdrawalFee
    StakedCitadel.withdraw
    StakedCitadel.setWithdrawalFee
    StakedCitadel.withdrawAll

Title: Init frontrun
Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself.
(details credit to: code-423n4/2021-09-sushimiso-findings#64)
The vulnerable initialization functions in the codebase are:

    Funding.sol, initialize, 104
    StakedCitadelVester.sol, initialize, 59
    CitadelMinter.sol, initialize, 109
    CitadelToken.sol, initialize, 22
    KnightingRound.sol, initialize, 109

Title: Two Steps Verification before Transferring Ownership
Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked.
It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership.
A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    StakedCitadel.sol
    Funding.sol

Title: Open TODOs
Severity: Low Risk

Open TODOs can hint at programming or architectural errors that still need to be fixed.
These files has open TODOs:

Open TODO in GlobalAccessControl.sol line 105 : /// TODO: Add string -> hash EnumerableSet to a new RoleRegistry contract for easy on-chain viewing.

Open TODO in Funding.sol line 14 : * TODO: Better revert strings

Open TODO in Funding.sol line 182 : // TODO: Check gas costs. How does this relate to market buying if you do want to deposit to xCTDL?

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

Open TODO in Funding.sol line 60 : // TODO: we should conform to some interface here

Title: safeApprove of openZeppelin is deprecated
Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base

Deprecated safeApprove in CitadelMinter.sol line 132: IERC20Upgradeable(_citadelToken).safeApprove(_xCitadel, type(uint256).max);

Deprecated safeApprove in Funding.sol line 141: IERC20(_citadel).safeApprove(address(_xCitadel), type(uint256).max);

Deprecated safeApprove in CitadelMinter.sol line 135: IERC20Upgradeable(_xCitadel).safeApprove(_xCitadelLocker, type(uint256).max);

Title: Not verified input
Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    StakedCitadel.sol.sweepExtraToken _token
    StakedCitadel.sol.emitNonProtectedToken _token
    StakedCitadelVester.sol.claim recipient
    StakedCitadel.sol.initialize _guardian
    StakedCitadelVester.sol.vest recipient

Title: Require with empty message
Severity: Low Risk

The following requires are with empty messages.
This is very important to add a message for any require. Such that the user has enough
information to know the reason of failure:

    Solidity file: StakedCitadel.sol, In line 181 with Empty Require message.
    Solidity file: StakedCitadel.sol, In line 186 with Empty Require message.
    Solidity file: StakedCitadel.sol, In line 185 with Empty Require message.
    Solidity file: StakedCitadel.sol, In line 184 with Empty Require message.
    Solidity file: StakedCitadel.sol, In line 180 with Empty Require message.

Title: Must approve 0 first
Severity: Low/Med Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first CitadelMinter.sol, 132, IERC20Upgradeable(_citadelToken).safeApprove(_xCitadel, type(uint256).max);

approve without approving 0 first CitadelMinter.sol, 135, IERC20Upgradeable(_xCitadel).safeApprove(_xCitadelLocker, type(uint256).max);

approve without approving 0 first Funding.sol, 141, IERC20(_citadel).safeApprove(address(_xCitadel), type(uint256).max);

Title: Div by 0
Severity: Low Risk

Division by 0 can lead to accidentally revert,

    https://github.com/code-423n4/2022-04-badger-citadel/tree/main/src/StakedCitadel.sol#L890 _pool might be 0