Badger Citadel contest - kebabsec's results

1. No checks regarding denominator being 0 in _withdraw of StakedCitadel.sol In line 811, no checks regarding if totalSupply() being 0. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L811) For reference, these LOC have such checks (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L285 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L887). We recommend implementing similar checks.

2. Miscellaneous issues I can't see any reason why assetCumulativeFunded cannot be equal to assetCap in this LOC (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L235), consider using "<=" instead of "<". Error code states that "cannot decrease cap below global sum of assets in", if this is the intended effect, "=>" should be used instead of ">", as now cap also can't be equal to sum of assets. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L297) Either change the error code or the behavior, related to above issue.

3. Comment issues Change "See '_depositFor'" part to "See '_depositForWithAuthorization' as that function is called first. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L347) Change the "See _depositWithAuthorization" part to "See '_depositForWithAuthorization'" as this form instructs the readers to check the function itself for reference. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L779) Add a @param proof comment here. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L362) Add a @param _globalAccessControl here. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L110) Add a @param _citadelPriceInAssetOracle for this parameter. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L110) All of the "should be less than" comments should be "should be less than or equal to " to match with the function behavior in these comments (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L519 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L531 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L546 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L607 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L621 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L641 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L660) _minCitadelOut param is wrongly described in this comment, change the description. (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L160) Description of contract seemingly describes another contract titled BadgerGeyser which tracks states and pledged tokens to be distributed, while the contract is about AccessControl. To my knowledge, these two contracts are unrelated, so change the description in these comments.(https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/GlobalAccessControl.sol#L14) The comment here about the duration of vesting period being 21 days (https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L82) could be obsolete in case if function setVestingDuration in StakedCitadelVester.sol (https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadelVester.sol#L163) is used to alter the vesting duration. You might consider adding this detail to the said comment to prevent future confusion.

4. Syntaxing style These variables in _handleFees function of StakedCitadel.sol (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L904) have confusing syntaxing. Might consider changing the syntaxing style.

5. Open TODO's These lines contain TODO statements,( https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L15 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/GlobalAccessControl.sol#L106 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/SupplySchedule.sol#L159 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/KnightingRound.sol#L14), you might consider to remove them.

6. Typos Typo in this line, (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L333), parantheses should came after the word "deposit". "deposi)t" -> "deposit)"

Typo in (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L289), "cumulatiive" should be "cumulative".

Typo in (https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/GlobalAccessControlManaged.sol#L24) "inhereiting" should be "inheriting".

1. Prefix increments cost less than postfix increments. ( https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/GlobalAccessControlManaged.sol#L48 ) Correct implementation can be seen in L344 of CitadelMinter.sol.

2. Use != 0 instead of >0 as these are equivalent in uint and != is cheaper gas-wise. ( https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L908 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L138 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L172 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L184 ) Correct implementation can be seen in L921 of StakedCitadel.sol.