Badger Citadel contest - robee's results

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: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/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: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/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

Title: Unnecessary cast Severity: Gas

    address Funding.sol.initialize - unnecessary casting address(_xCitadel)
    address CitadelMinter.sol.setFundingPoolWeight - unnecessary casting address(_pool)

Title: Storage double reading. Could save SLOAD Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    SupplySchedule.sol: globalStartTimestamp is read twice in getMintableDebug

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: SupplySchedule.sol, In line 90, Require message length to shorten: 35, The message: SupplySchedule: minting not started
    Solidity file: KnightingRound.sol, In line 275, Require message length to shorten: 34, The message: KnightingRound: not enough balance
    Solidity file: StakedCitadelVester.sol, In line 138, Require message length to shorten: 34, The message: StakedCitadelVester: cannot vest 0
    Solidity file: KnightingRound.sol, In line 273, Require message length to shorten: 33, The message: KnightingRound: already finalized
    Solidity file: KnightingRound.sol, In line 384, Require message length to shorten: 33, The message: KnightingRound: already finalized

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    KnightingRound.sol (L#259) - hasEnded_ = (block.timestamp >= saleStart + saleDuration) ||
    KnightingRound.sol (L#168) - require( block.timestamp < saleStart + saleDuration, "KnightingRound: already ended" );

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In Funding.sol,rearranging the storage fields can optimize to: 16 slots from: 17 slots. The new order of types (you choose the actual variables): 1. bytes32 2. bytes32 3. bytes32 4. bytes32 5. bytes32 6. uint256 7. IERC20 8. IVault 9. IERC20 10. uint256 11. uint256 12. uint256 13. uint256 14. FundingParams 15. address 16. bool 17. address

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    CitadelMinter.sol, 270: change '_weight > 0' to '_weight != 0'
    KnightingRound.sol, 125: change 'saleDuration > 0' to 'saleDuration != 0'
    Funding.sol, 452: change 'citadelPriceInAsset > 0' to 'citadelPriceInAsset != 0'
    Funding.sol, 170: change '_assetAmountIn > 0' to '_assetAmountIn != 0'
    KnightingRound.sol, 276: change 'balance > 0' to 'balance != 0'

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    CitadelToken.initialize (_symbol)
    CitadelToken.initialize (_name)

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    GlobalAccessControl.sol, transferFromDisabled

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    assetDecimalsNormalizationValue in Funding.sol
    xCitadel in Funding.sol
    funding in Funding.sol
    supplySchedule in CitadelMinter.sol
    tokenInNormalizationValue in KnightingRound.sol

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    StakedCitadel.sol, _depositFor
    SupplySchedule.sol, _setEpochRates
    StakedCitadel.sol, _handleFees
    CitadelMinter.sol, _addFundingPool
    CitadelMinter.sol, _transferToFundingPools

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    StakedCitadel.sol (L85), string internal constant _defaultNamePrefix = "Staked ";
    StakedCitadel.sol (L86), string internal constant _symbolSymbolPrefix = "x"; 

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: CitadelMinter.sol, i, 152
    just change to unchecked: CitadelMinter.sol, i, 344
    change to prefix increment and unchecked: SupplySchedule.sol, i, 208

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    KnightingRound.sol, saleEnded
    StakedCitadel.sol, getPricePerFullShare
    SupplySchedule.sol, getEmissionsForCurrentEpoch
    SupplySchedule.sol, initialize
    CitadelToken.sol, initialize

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    StakedCitadel.sol, _onlyStrategy
    StakedCitadel.sol, _depositFor
    SupplySchedule.sol, _setEpochRates
    StakedCitadel.sol, _withdraw
    StakedCitadel.sol, _handleFees

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    CitadelMinter.sol, 152