QA Report

[code423n4]

Vulnerability details:

Low Risk Issues

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

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

       minCitadelPriceInAsset = _minPrice;
       maxCitadelPriceInAsset = _maxPrice;

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

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

       tokenOutAmount_ =
           (_tokenInAmount * tokenOutPrice) /
           tokenInNormalizationValue;

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.

1. 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);

2. File: src/KnightingRound.sol (line 148)

       tokenInNormalizationValue = 10**tokenIn.decimals();

3. File: src/StakedCitadel.sol (line 218)

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

4. File: src/StakedCitadel.sol (line 226)

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

Missing checks for address(0x0) when assigning values to address state variables

1. File: external/StakedCitadelLocker.sol (line 186)

       stakingProxy = _staking;

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

       strategist = _strategist;

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

       keeper = _keeper;

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

       governance = _governance;

initialize functions can be front-run

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

1. File: src/CitadelMinter.sol (line 109)

   function initialize(

2. File: src/KnightingRound.sol (line 119)

   ) external initializer {

3. File: src/Funding.sol (line 112)

   ) external initializer {

4. File: src/StakedCitadel.sol (line 179)

   ) public initializer whenNotPaused {

now is deprecated

Use block.timestamp instead

1. File: external/MedianOracle.sol (line 129)

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

2. File: external/MedianOracle.sol (line 131)

       reports[index_past].timestamp = now;

3. File: external/MedianOracle.sol (line 134)

       emit ProviderReportPushed(providerAddress, payload, now);

4. File: external/MedianOracle.sol (line 161)

       uint256 minValidTimestamp =  now.sub(reportExpirationTimeSec);

5. File: external/MedianOracle.sol (line 162)

       uint256 maxValidTimestamp =  now.sub(reportDelaySec);

safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

1. File: src/CitadelMinter.sol (line 133)

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

2. File: src/CitadelMinter.sol (line 136)

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

3. File: src/Funding.sol (line 142)

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

Open TODOs

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

1. File: src/Funding.sol (line 15)

   * TODO: Better revert strings

2. File: src/Funding.sol (line 61)

   // TODO: we should conform to some interface here

3. 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?

4. File: src/GlobalAccessControl.sol (line 106)

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

5. File: src/KnightingRound.sol (line 14)

   * TODO: Better revert strings

6. 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)

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.

1. File: external/StakedCitadelLocker.sol (line 26)

   contract StakedCitadelLocker is Initializable, ReentrancyGuardUpgradeable, OwnableUpgradeable {

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

   contract CitadelMinter is
   GlobalAccessControlManaged,
   ReentrancyGuardUpgradeable

3. File: src/CitadelToken.sol (line 8)

   contract CitadelToken is GlobalAccessControlManaged, ERC20Upgradeable {

4. File: src/Funding.sol (line 17)

   contract Funding is GlobalAccessControlManaged, ReentrancyGuardUpgradeable {

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

   contract GlobalAccessControl is
   AccessControlEnumerableUpgradeable,
   PausableUpgradeable

6. File: src/KnightingRound.sol (line 16)

   contract KnightingRound is GlobalAccessControlManaged, ReentrancyGuardUpgradeable {

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

   contract GlobalAccessControlManaged is PausableUpgradeable {

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

   contract StakedCitadel is
   ERC20Upgradeable,
   SettAccessControl,
   PausableUpgradeable,
   ReentrancyGuardUpgradeable

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

   contract StakedCitadelVester is
   GlobalAccessControlManaged,
   ReentrancyGuardUpgradeable

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.

1. File: src/GlobalAccessControl.sol (line 51)

   bool public transferFromDisabled; // Set to true in initialize

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

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

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

Non-critical Issues

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

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

   function initialize(address vault, address want) external;
   
   function getName() external view returns (string memory);

Unused file

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

   // SPDX-License-Identifier: MIT

Contract header not updated after branching

1. 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.
   */

Comment not moved when function was moved

1. 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

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

1. 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

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

1. File: src/SupplySchedule.sol (line 4)

   import "ds-test/test.sol";

The nonReentrant modifier should occur before all other modifiers

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

1. File: src/CitadelMinter.sol (line 173)

       nonReentrant

2. File: src/CitadelMinter.sol (line 254)

       nonReentrant

3. File: src/CitadelMinter.sol (line 298)

   ) external onlyRole(POLICY_OPERATIONS_ROLE) gacPausable nonReentrant {

4. File: src/Funding.sol (line 167)

       nonReentrant

5. File: src/Funding.sol (line 318)

       nonReentrant

6. File: src/KnightingRound.sol (line 402)

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

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;

Adding a return statement when the function defines a named return variable, is redundant

1. File: external/StakedCitadelLocker.sol (line 272)

       return userRewards;

2. File: external/StakedCitadelLocker.sol (line 311)

       return amount;

3. File: external/StakedCitadelLocker.sol (line 338)

       return amount;

4. File: external/StakedCitadelLocker.sol (line 399)

       return supply;

5. File: external/StakedCitadelLocker.sol (line 417)

       return supply;

require()/revert() statements should have descriptive reason strings

1. File: external/MedianOracle.sol (line 68)

       require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);

2. File: external/MedianOracle.sol (line 69)

       require(minimumProviders_ > 0);

3. File: external/MedianOracle.sol (line 84)

       require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);

4. File: external/MedianOracle.sol (line 109)

       require(minimumProviders_ > 0);

5. File: external/MedianOracle.sol (line 123)

       require(timestamps[0] > 0);

6. File: external/MedianOracle.sol (line 129)

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

7. File: external/MedianOracle.sol (line 143)

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

8. File: external/MedianOracle.sol (line 211)

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

9. File: external/StakedCitadelLocker.sol (line 126)

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

10. File: external/StakedCitadelLocker.sol (line 163)

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

11. File: external/StakedCitadelLocker.sol (line 178)

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

12. File: external/StakedCitadelLocker.sol (line 812)

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

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

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

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

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

15. File: src/StakedCitadel.sol (line 180)

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

16. File: src/StakedCitadel.sol (line 181)

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

17. File: src/StakedCitadel.sol (line 182)

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

18. File: src/StakedCitadel.sol (line 183)

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

19. File: src/StakedCitadel.sol (line 184)

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

20. File: src/StakedCitadel.sol (line 185)

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

21. File: src/StakedCitadel.sol (line 186)

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

22. File: src/StakedCitadel.sol (line 187)

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

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.

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

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

2. File: external/StakedCitadelLocker.sol (line 142)

   function decimals() public view returns (uint8) {

3. File: external/StakedCitadelLocker.sol (line 145)

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

4. File: external/StakedCitadelLocker.sol (line 148)

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

5. File: external/StakedCitadelLocker.sol (line 151)

   function version() public view returns(uint256){

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

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

7. File: external/StakedCitadelLocker.sol (line 250)

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

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

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

9. File: src/Funding.sol (line 223)

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

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

    function __GlobalAccessControlManaged_init(address _globalAccessControl)
        public
        onlyInitializing

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

    function setGovernance(address _governance) public {

12. 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 {

13. File: src/StakedCitadel.sol (line 284)

    function getPricePerFullShare() public view returns (uint256) {

14. File: src/SupplySchedule.sol (line 43)

    function initialize(address _gac) public initializer {

15. File: src/SupplySchedule.sol (line 79)

    function getEmissionsForCurrentEpoch() public view returns (uint256) {

constants should be defined rather than using magic numbers

1. File: external/StakedCitadelLocker.sol (line 131)

       _decimals = 18;

2. File: external/StakedCitadelLocker.sol (line 201)

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

3. File: external/StakedCitadelLocker.sol (line 202)

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

4. File: external/StakedCitadelLocker.sol (line 211)

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

5. File: external/StakedCitadelLocker.sol (line 232)

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

6. File: external/StakedCitadelLocker.sol (line 243)

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

7. File: external/StakedCitadelLocker.sol (line 428)

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

8. File: src/CitadelMinter.sol (line 272)

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

9. File: src/StakedCitadel.sol (line 178)

       uint256[4] memory _feeConfig

10. File: src/StakedCitadel.sol (line 203)

            _feeConfig[3] <= MANAGEMENT_FEE_HARD_CAP,

11. File: src/StakedCitadel.sol (line 250)

        managementFee = _feeConfig[3];

12. File: src/StakedCitadel.sol (line 255)

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

13. File: src/SupplySchedule.sol (line 170)

        epochRate[0] = 593962000000000000000000 / epochLength;

14. File: src/SupplySchedule.sol (line 171)

        epochRate[1] = 591445000000000000000000 / epochLength;

15. File: src/SupplySchedule.sol (line 172)

        epochRate[2] = 585021000000000000000000 / epochLength;

16. File: src/SupplySchedule.sol (line 173)

        epochRate[3] = 574138000000000000000000 / epochLength;

17. File: src/SupplySchedule.sol (line 173)

        epochRate[3] = 574138000000000000000000 / epochLength;

18. File: src/SupplySchedule.sol (line 174)

        epochRate[4] = 558275000000000000000000 / epochLength;

19. File: src/SupplySchedule.sol (line 174)

        epochRate[4] = 558275000000000000000000 / epochLength;

20. File: src/SupplySchedule.sol (line 175)

        epochRate[5] = 536986000000000000000000 / epochLength;

21. File: src/SupplySchedule.sol (line 175)

        epochRate[5] = 536986000000000000000000 / epochLength;

Numeric values having to do with time should use time units for readability

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

1. File: external/StakedCitadelLocker.sol (line 70)

   uint256 public constant rewardsDuration = 86400; // 1 day

2. File: external/StakedCitadelLocker.sol (line 70)

   uint256 public constant rewardsDuration = 86400; // 1 day

3. File: src/StakedCitadelVester.sol (line 34)

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

4. File: src/StakedCitadelVester.sol (line 34)

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

Constant redefined elsewhere

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

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

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

   seen in src/CitadelMinter.sol

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

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

   seen in src/CitadelMinter.sol

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

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

   seen in src/Funding.sol

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

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

   seen in src/Funding.sol

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

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

   seen in src/Funding.sol

6. File: src/GlobalAccessControl.sol (line 37)

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

   seen in src/Funding.sol

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

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

   seen in src/CitadelToken.sol

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

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

   seen in src/GlobalAccessControl.sol

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

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

   seen in src/GlobalAccessControl.sol

10. File: src/KnightingRound.sol (lines 24-25)

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

    seen in src/GlobalAccessControl.sol

11. File: src/KnightingRound.sol (lines 26-27)

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

    seen in src/GlobalAccessControl.sol

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

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

    seen in src/GlobalAccessControl.sol

13. File: src/lib/GlobalAccessControlManaged.sol (line 16)

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

    seen in src/GlobalAccessControl.sol

14. File: src/StakedCitadel.sol (line 112)

    uint256 public constant MAX_BPS = 10_000;

    seen in src/Funding.sol

15. File: src/StakedCitadelVester.sol (lines 20-21)

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

    seen in src/KnightingRound.sol

16. File: src/SupplySchedule.sol (lines 22-23)

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

    seen in src/StakedCitadelVester.sol

Non-library/interface files should use fixed compiler versions, not floating ones

1. File: src/CitadelToken.sol (line 2)

   pragma solidity ^0.8.0;

2. File: src/GlobalAccessControl.sol (line 3)

   pragma solidity ^0.8.0;

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

   pragma solidity ^0.8.12;

Typos

1. File: external/StakedCitadelLocker.sol (line 300)

               //stop now as no futher checks are needed

   futher

2. File: src/CitadelMinter.sol (line 102)

    * @dev this contract is intended to be the only way citadel is minted, with the expection of the initial minting event

   expection

3. File: src/Funding.sol (line 289)

    * @param _assetCap New max cumulatiive amountIn

   cumulatiive

4. File: src/Funding.sol (line 333)

   /// @dev We let assets accumulate and batch transfer to treasury (rather than transfer atomically on each deposi)t for user gas savings

   deposi)t

5. File: src/KnightingRound.sol (line 342)

    * @notice Update the `tokenIn` receipient address. Can only be called by owner

   receipient

6. File: src/lib/GlobalAccessControlManaged.sol (line 24)

    * @dev this is assumed to be used in the initializer of the inhereiting contract

   inhereiting

7. File: src/lib/GlobalAccessControlManaged.sol (line 60)

   // @dev used to faciliate extra contract-specific permissioned accounts

   faciliate

8. File: src/StakedCitadel.sol (line 81)

   address public badgerTree; // Address we send tokens too via reportAdditionalTokens

   too -> to

File does not contain an SPDX Identifier

1. File: external/MedianOracle.sol (line 0)

   pragma solidity 0.4.24;

File is missing NatSpec

1. File: external/StakedCitadelLocker.sol (line 0)

   // SPDX-License-Identifier: MIT

2. File: src/interfaces/badger/IBadgerGuestlist.sol (line 0)

   // SPDX-License-Identifier: MIT

3. File: src/interfaces/badger/IBadgerVipGuestlist.sol (line 0)

   // SPDX-License-Identifier: MIT

4. File: src/interfaces/badger/IEmptyStrategy.sol (line 0)

   // SPDX-License-Identifier: MIT

5. File: src/interfaces/badger/IStrategy.sol (line 0)

   // SPDX-License-Identifier: MIT

6. File: src/interfaces/badger/IVault.sol (line 0)

   // SPDX-License-Identifier: MIT

7. File: src/interfaces/citadel/ICitadelToken.sol (line 0)

   // SPDX-License-Identifier: MIT

8. File: src/interfaces/citadel/IGac.sol (line 0)

   /// SPDX-License-Identifier: MIT

9. File: src/interfaces/citadel/IMedianOracle.sol (line 0)

   /// SPDX-License-Identifier: MIT

10. File: src/interfaces/citadel/IStakedCitadelLocker.sol (line 0)

    // SPDX-License-Identifier: MIT

11. File: src/interfaces/citadel/ISupplySchedule.sol (line 0)

    // SPDX-License-Identifier: MIT

12. File: src/interfaces/citadel/IVesting.sol (line 0)

    // SPDX-License-Identifier: MIT

13. File: src/interfaces/convex/IRewardStaking.sol (line 0)

    // SPDX-License-Identifier: MIT

14. File: src/interfaces/convex/IStakingProxy.sol (line 0)

    // SPDX-License-Identifier: MIT

NatSpec is incorrect

Wrong parameter description

1. File: src/Funding.sol (line 160)

    * @param _minCitadelOut ID of DAO to vote for

NatSpec is incomplete

1. File: src/Funding.sol (lines 95-112)

   /**
    * @notice Initializer.
    * @param _gac Global access control
    * @param _citadel The token this contract will return in a trade
    * @param _asset The token this contract will receive in a trade
    * @param _xCitadel Staked citadel, citadel will be granted to funders in this form
    * @param _saleRecipient The address receiving the proceeds of the sale - will be citadel multisig
    * @param _assetCap The max asset that the contract can take
    */
   function initialize(
       address _gac,
       address _citadel,
       address _asset,
       address _xCitadel,
       address _saleRecipient,
       address _citadelPriceInAssetOracle,
       uint256 _assetCap
   ) external initializer {

   Missing: @param _citadelPriceInAssetOracle

2. File: src/KnightingRound.sol (lines 98-119)

   /**
    * @notice Initializer.
    * @param _tokenOut The token this contract will return in a trade (citadel)
    * @param _tokenIn The token this contract will receive in a trade
    * @param _saleStart The time when tokens can be first purchased
    * @param _saleDuration The duration of the token sale
    * @param _tokenOutPrice The tokenOut per tokenIn price
    * @param _saleRecipient The address receiving the proceeds of the sale - will be citadel multisig
    * @param _guestlist Address that will manage auction approvals
    * @param _tokenInLimit The max tokenIn that the contract can take
    */
   function initialize(
       address _globalAccessControl,
       address _tokenOut,
       address _tokenIn,
       uint256 _saleStart,
       uint256 _saleDuration,
       uint256 _tokenOutPrice,
       address _saleRecipient,
       address _guestlist,
       uint256 _tokenInLimit
   ) external initializer {

   Missing: @param _globalAccessControl

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

   /// @notice Initializes the Sett. Can only be called once, ideally when the contract is deployed.
   /// @param _token Address of the token that can be deposited into the sett.
   /// @param _governance Address authorized as governance.
   /// @param _keeper Address authorized as keeper.
   /// @param _guardian Address authorized as guardian.
   /// @param _treasury Address to distribute governance fees/rewards to.
   /// @param _strategist Address authorized as strategist.
   /// @param _badgerTree Address of badgerTree used for emissions.
   /// @param _name Specify a custom sett name. Leave empty for default value.
   /// @param _symbol Specify a custom sett symbol. Leave empty for default value.
   /// @param _feeConfig Values for the 4 different types of fees charges by the sett
   ///         [performanceFeeGovernance, performanceFeeStrategist, withdrawToVault, managementFee]
   ///         Each fee should be less than the constant hard-caps defined above.
   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 {

   Missing: @param _vesting

4. File: src/StakedCitadel.sol (lines 357-367)

   /// @notice Deposits `_amount` tokens, issuing shares to `recipient`.
   ///         Checks the guestlist to verify that `recipient` is authorized to make a deposit for the specified `_amount`.
   ///         Note that deposits are not accepted when the Sett is paused or when `pausedDeposit` is true.
   /// @dev See `_depositForWithAuthorization` for details on guestlist authorization.
   /// @param _recipient Address to issue the Sett shares to.
   /// @param _amount Quantity of tokens to deposit.
   function depositFor(
       address _recipient,
       uint256 _amount,
       bytes32[] memory proof
   ) external whenNotPaused {

   Missing: @param proof

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

1. File: external/MedianOracle.sol (line 35)

   event ProviderAdded(address provider);

2. File: external/MedianOracle.sol (line 36)

   event ProviderRemoved(address provider);

3. File: external/MedianOracle.sol (line 37)

   event ReportTimestampOutOfRange(address provider);

4. File: external/MedianOracle.sol (line 38)

   event ProviderReportPushed(address indexed provider, uint256 payload, uint256 timestamp);

5. File: external/StakedCitadelLocker.sol (line 853)

   event RewardAdded(address indexed _token, uint256 _reward);

6. File: external/StakedCitadelLocker.sol (line 854)

   event Staked(address indexed _user, uint256 indexed _epoch, uint256 _paidAmount, uint256 _lockedAmount, uint256 _boostedAmount);

7. File: external/StakedCitadelLocker.sol (line 855)

   event Withdrawn(address indexed _user, uint256 _amount, bool _relocked);

8. File: external/StakedCitadelLocker.sol (line 856)

   event KickReward(address indexed _user, address indexed _kicked, uint256 _reward);

9. File: external/StakedCitadelLocker.sol (line 857)

   event RewardPaid(address indexed _user, address indexed _rewardsToken, uint256 _reward);

10. File: external/StakedCitadelLocker.sol (line 858)

    event Recovered(address _token, uint256 _amount);

11. File: src/CitadelMinter.sol (lines 56-60)

    event FundingPoolWeightSet(
        address pool,
        uint256 weight,
        uint256 totalFundingPoolWeight
    );

12. File: src/CitadelMinter.sol (lines 61-65)

    event CitadelDistributionSplitSet(
        uint256 fundingBps,
        uint256 stakingBps,
        uint256 lockingBps
    );

13. File: src/CitadelMinter.sol (lines 66-70)

    event CitadelDistribution(
        uint256 fundingAmount,
        uint256 stakingAmount,
        uint256 lockingAmount
    );

14. File: src/CitadelMinter.sol (lines 72-76)

    event CitadelDistributionToFunding(
        uint256 startTime,
        uint256 endTime,
        uint256 citadelAmount
    );

15. File: src/CitadelMinter.sol (lines 77-82)

    event CitadelDistributionToFundingPool(
        uint256 startTime,
        uint256 endTime,
        address pool,
        uint256 citadelAmount
    );

16. File: src/CitadelMinter.sol (lines 83-87)

    event CitadelDistributionToStaking(
        uint256 startTime,
        uint256 endTime,
        uint256 citadelAmount
    );

17. File: src/CitadelMinter.sol (lines 88-93)

    event CitadelDistributionToLocking(
        uint256 startTime,
        uint256 endTime,
        uint256 citadelAmount,
        uint256 xCitadelAmount
    );

18. File: src/Funding.sol (lines 62-66)

    event Deposit(
        address indexed buyer,
        uint256 assetIn,
        uint256 citadelOutValue
    );

19. File: src/Funding.sol (line 68)

    event CitadelPriceInAssetUpdated(uint256 citadelPrice);

20. File: src/Funding.sol (line 70)

    event CitadelPriceBoundsSet(uint256 minPrice, uint256 maxPrice);

21. File: src/Funding.sol (line 71)

    event CitadelPriceFlag(uint256 price, uint256 minPrice, uint256 maxPrice);

22. File: src/Funding.sol (line 74)

    event AssetCapUpdated(uint256 assetCap);

23. File: src/Funding.sol (line 76)

    event Sweep(address indexed token, uint256 amount);

24. File: src/Funding.sol (line 77)

    event ClaimToTreasury(address indexed token, uint256 amount);

25. File: src/Funding.sol (line 87)

    event DiscountLimitsSet(uint256 minDiscount, uint256 maxDiscount);

26. File: src/Funding.sol (line 88)

    event DiscountSet(uint256 discount);

27. File: src/Funding.sol (line 89)

    event DiscountManagerSet(address discountManager);

28. File: src/interfaces/erc20/IERC20.sol (line 85)

    event Transfer(address indexed from, address indexed to, uint256 value);

29. File: src/interfaces/erc20/IERC20.sol (lines 91-95)

    event Approval(
        address indexed owner,
        address indexed spender,
        uint256 value
    );

30. File: src/KnightingRound.sol (lines 76-81)

    event Sale(
        address indexed buyer,
        uint8 indexed daoId,
        uint256 amountIn,
        uint256 amountOut
    );

31. File: src/KnightingRound.sol (line 82)

    event Claim(address indexed claimer, uint256 amount);

32. File: src/KnightingRound.sol (line 85)

    event SaleStartUpdated(uint256 saleStart);

33. File: src/KnightingRound.sol (line 86)

    event SaleDurationUpdated(uint256 saleDuration);

34. File: src/KnightingRound.sol (line 87)

    event TokenOutPriceUpdated(uint256 tokenOutPrice);

35. File: src/KnightingRound.sol (line 90)

    event TokenInLimitUpdated(uint256 tokenInLimit);

36. File: src/KnightingRound.sol (line 92)

    event Sweep(address indexed token, uint256 amount);

37. File: src/StakedCitadel.sol (lines 122-127)

    event TreeDistribution(
        address indexed token,
        uint256 amount,
        uint256 indexed blockNumber,
        uint256 timestamp
    );

38. File: src/StakedCitadel.sol (lines 130-135)

    event Harvested(
        address indexed token,
        uint256 amount,
        uint256 indexed blockNumber,
        uint256 timestamp
    );

39. File: src/StakedCitadel.sol (line 139)

    event SetToEarnBps(uint256 newEarnToBps);

40. File: src/StakedCitadel.sol (line 140)

    event SetMaxWithdrawalFee(uint256 newMaxWithdrawalFee);

41. File: src/StakedCitadel.sol (line 141)

    event SetMaxPerformanceFee(uint256 newMaxPerformanceFee);

42. File: src/StakedCitadel.sol (line 142)

    event SetMaxManagementFee(uint256 newMaxManagementFee);

43. File: src/StakedCitadel.sol (line 146)

    event SetWithdrawalFee(uint256 newWithdrawalFee);

44. File: src/StakedCitadel.sol (line 147)

    event SetPerformanceFeeStrategist(uint256 newPerformanceFeeStrategist);

45. File: src/StakedCitadel.sol (line 148)

    event SetPerformanceFeeGovernance(uint256 newPerformanceFeeGovernance);

46. File: src/StakedCitadel.sol (line 149)

    event SetManagementFee(uint256 newManagementFee);

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

    event Vest(
        address indexed recipient,
        uint256 _amount,
        uint256 _unlockBegin,
        uint256 _unlockEnd
    );

48. File: src/StakedCitadelVester.sol (lines 47-51)

    event Claimed(
        address indexed owner,
        address indexed recipient,
        uint256 amount
    );

49. File: src/StakedCitadelVester.sol (line 53)

    event SetVestingDuration(uint256 duration);

50. File: src/SupplySchedule.sol (line 36)

    event MintingStartTimeSet(uint256 globalStartTimestamp);

51. File: src/SupplySchedule.sol (line 37)

    event EpochSupplyRateSet(uint256 epoch, uint256 rate);

Non-exploitable reentrancies

Reentrancy in CitadelMinter.mintAndDistribute() (src/CitadelMinter.sol#169-243):
External calls:
- citadelToken.mint(address(this),mintable) (src/CitadelMinter.sol#178-180)
- IVault(cachedXCitadel).deposit(lockingAmount) (src/CitadelMinter.sol#195-197)
- xCitadelLocker.notifyRewardAmount(address(cachedXCitadel),xCitadelToLockers) (src/CitadelMinter.sol#201-205)
- IERC20Upgradeable(address(citadelToken)).safeTransfer(address(cachedXCitadel),stakingAmount) (src/CitadelMinter.sol#217-218)
- _transferToFundingPools(fundingAmount) (src/CitadelMinter.sol#230-231)
- returndata = address(token).functionCall(data,SafeERC20: low-level call failed) (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol#93)
- (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137)
- IERC20Upgradeable(address(citadelToken)).safeTransfer(pool,amount) (src/CitadelMinter.sol#351-353)
External calls sending eth:
- _transferToFundingPools(fundingAmount) (src/CitadelMinter.sol#230-231)
- (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137)
State variables written after the call(s):
- lastMintTimestamp = block.timestamp (src/CitadelMinter.sol#240-241)

Reentrancy in StakedCitadel._withdraw(uint256) (src/StakedCitadel.sol#808-840):
External calls:
- IStrategy(strategy).withdraw(_toWithdraw) (src/StakedCitadel.sol#818-819)
- IVesting(vesting).setupVesting(msg.sender,_amount,block.timestamp) (src/StakedCitadel.sol#830-831)
- token.safeTransfer(vesting,_amount) (src/StakedCitadel.sol#831-833)
State variables written after the call(s):
- _mintSharesFor(treasury,_fee,balance() - _fee) (src/StakedCitadel.sol#836-837)
- _balances[account] += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#268)
- _mintSharesFor(treasury,_fee,balance() - _fee) (src/StakedCitadel.sol#836-837)
- _totalSupply += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#267)

Reentrancy in StakedCitadel._depositFor(address,uint256) (src/StakedCitadel.sol#764-779):
External calls:
- token.safeTransferFrom(msg.sender,address(this),_amount) (src/StakedCitadel.sol#774-775)
State variables written after the call(s):
- _mintSharesFor(_recipient,_after - _before,_pool) (src/StakedCitadel.sol#776-777)
- _balances[account] += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#268)
- _mintSharesFor(_recipient,_after - _before,_pool) (src/StakedCitadel.sol#776-777)
- _totalSupply += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#267)
Reentrancy in Funding.updateCitadelPriceInAsset() (src/Funding.sol#414-443):
External calls:
- (_citadelPriceInAsset,_valid) = IMedianOracle(citadelPriceInAssetOracle).getData() (src/Funding.sol#422-423)
State variables written after the call(s):
- citadelPriceFlag = true (src/Funding.sol#431-432)
- citadelPriceInAsset = _citadelPriceInAsset (src/Funding.sol#438-439)

[liveactionllama]

Warden created this issue as a placeholder, because their submission was too large for the contest form. They then emailed their md file to our team. I've updated this issue with their md file content.

[liveactionllama]

Note: my original copy/paste did not capture all of the warden's content. I've updated this issue to now contain the entirety of the original submission. I've also notified the sponsor. Contest has not yet moved to judging.

[jack-the-pug]

Misleading comment, now is deprecated and Open TODOs in the Low Risk Issues section should be Non-critical.