QA Report

[code423n4]

L01: funding.sol: setCitadelAssetPriceBounds does not check that the bounds are valid with the current CitadelAssetPrice

Calling setCitadelAssetPriceBounds with _minPrice, _maxPrice where citadelPriceInAsset is not in that range should not be allowed. This can result in a weird state, even though it will not cause a severe vulnerability.

Proof Of Concept

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

Recommended Mitigation Steps

Add require(citadelPriceInAsset >= _minPrice && citadelPriceInAsset <= _maxPrice) to the function

L02 StakedCitadelVester.vest does not check that _unlockBegin is valid.

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

If an invalid _unlockBegin is given such that _unlockBegin + vestingDuration < block.timestamp then the user will be able to claim vestingToken straight away.

Recommended Mitigation Steps

require(_unlockBegin >= block.timestamp)

L03: missing zero address check in functions

Functions in some of the contracts are missing checks that address != 0. Using such addresses can brake the protocol.

StakedCitadelLocker.addReward StakedCitadelLocker.approveRewardDistributor StakedCitadelLocker.setStakingContract StakedCitadelVester.vest Funding.setDiscountManager

Recommended Mitigation Steps

require(_var_name != address(0), "address 0 invalid");

NC01: KnightingRound.sol: should check tokenInLimit > 0

should require tokenInLimit > 0, when tokenInLimit = 0 the knigting round isn't functional.

add require(_tokenInLimit > 0) in initialize and setTokenInLimit

NC02: StakedCitadel._calculateFee unecessary if (feeBps == 0)

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

if feeBps = 0 then (amount * feeBps) / MAX_BPS; will be 0 anyway, no need for the if statement

NC03: StakedCitadelVester.claim, use require(amount != 0) instead of if (amount != 0)

If there is no claimable balance or the requested amount is 0, then claim should fail on a require. replace if (amount != 0) with require(amount != 0, "error msg")

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

NC04: Missing documentation

Many functions and complex logic are missing informative documentation and comments.

CitadelMinter.getFundingPoolWeights CitadelMinter.initializeLastMintTimestamp CitadelMinter._transferToFundingPools CitadelMinter._removeFundingPool CitadelMinter._addFundingPool Funding.clearCitadelPriceFlag Funding.setSaleRecipient Funding.setCitadelAssetPriceBounds StakedCitadelVester.initialize SupplySchedule.initialize SupplySchedule.getCurrentEpoch SupplySchedule.getEmissionsForEpoch SupplySchedule.getEmissionsForCurrentEpoch SupplySchedule.getMintable SupplySchedule.setMintingStart SupplySchedule.setEpochRate

NC05: StakedCitadel.ONE_ETH visibility should be explicit

default visibility is internal however it is good practice to specify it explicitly. change ONE_ETH to

uint256 internal constant ONE_ETH = 1e18;

NC06: StakedCitadel._withdraw: calling with shares > 0, totalSupply() = 0, will cause zero division error

trying to withdraw while the total supply of the token is 0, will cause a zero division error. Instead should add require(totalSupply() != 0), "msg" with an informative message, to make the error clear to the caller.

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

NC07: Funding.sweep: emit after transfer

It is good practice and more readable to emit at the end.

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

NC08: SupplySchedule.getEpochAtTimestamp wrong documentation,

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

this comment is meant for getMintableDebug and not for getEpochAtTimestamp. Can be confusing while reading the code.

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

NC09: SupplySchedule.getEmissionsForEpoch should check that _epoch is valid

require that the _epoch is valid and has a rate, this is better than returning 0.

change getEmissionsForEpoch to:

rate = epochRate[_epoch];
require(rate > 0, "invalid epoch");
return rate * epochLength;

NC10: simplify logic in CitadelMinter.setFundingPoolWeight

this logic:

uint256 _newTotalWeight = totalFundingPoolWeight;
_newTotalWeight = _newTotalWeight - fundingPoolWeights[_pool];
fundingPoolWeights[_pool] = _weight;
_newTotalWeight = _newTotalWeight + _weight;
totalFundingPoolWeight = _newTotalWeight;

emit FundingPoolWeightSet(_pool, _weight, _newTotalWeight);

is unecessarily complex, can simplify to:

totalFundingPoolWeight = totalFundingPoolWeight - fundingPoolWeights[_pool] + _weight;
fundingPoolWeights[_pool] = _weight;

emit FundingPoolWeightSet(_pool, _weight, totalFundingPoolWeight)

This is much more readable and saves gas.

NC11: Add error message to require in StakedCitadelLocker.sol

Most require statements in this contract are missing an error message. One should be added.

Example: require(_stakingToken != address(0)) -> require(_stakingToken != address(0), "zero adress")

NC12: Funding.sol: use unit256 consistantly , replace unit

use uint256 or unit consistently, preferably uint256

funding.sol 224

funding.sol 419

NC13: a += b rather than a = a + b

a += b is cleaner than a = a + b.

Example:

totalTokenIn = totalTokenIn + _tokenInAmount; -> totalTokenIn += _tokenInAmount;

example in repo