Badger Citadel contest - kyliek's results

Lines of code

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

Vulnerability details

Impact

When earn() is called by authorized actors (keeper or governance), 95% of the balance of CTDL token in the StakedCitadel contract will be transferred to strategy. https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L722

Thus, the balance() will be roughly only 5% of the totalSupply(). At this juncture, if an attacker deposit(amt) for _amt amount of CDTL tokens, he will get _amt * totalSupply()/balance(), which is about 20 times more than he should.

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

This affects also _withdraw calculation where r can be very small, or rounds to 0 when _shares is less than 20. Thus the users won't be able to withdraw right after earn() is called.

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

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Step 1: prepare a big sum of CTDL tokens (as much as one get can)

Step 2: monitor the contract activity and wait for earn() to be called by authorised actors.

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

Step 3: deposit CTDL tokens from Step1 to deposit(uint256) function and get up to 20x staked xCDTL tokens. https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L309-L311

Step 4: monitor the contract activity and wait for withdrawToVault() is triggered https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L676-L679

Step 5: withdraw all staked tokens by calling withdrawAll() to get back CTDL tokens that correspond to the inflated shares.

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

Tools Used

Recommended Mitigation Steps

balance() function needs to keep track of both this contract balance and how much is in strategy contract, which may be compounding as well.

Low

[L1] hardcoded version number cannot be updated in the future

The version number is hardcoded, which cannot be updated in the future. Also the function is not virtual, thus cannot be override in future contract.

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

[L2] Mixing test and production code

Test is really useful in development but not in production.

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

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

[L3] CitadelToken cannot be minted before CITADEL_MINTER_ROLE is assigned

Consider assign minter role in the initialize function to ensure that mint can be called successfully.

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

[L4] Not able to sweep ethers in KnightingRound

ERC20 tokens can be sweeped by the following function. https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L402-L415

In the unlikely event that some contract self-destruct to send ethers into this contract, one may as well implement a function to sweep ethers.

[L5] ABIEncoderV2 is no longer considered experimental

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

Because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2

see https://docs.soliditylang.org/en/v0.8.13/layout-of-source-files.html#abiencoderv2

[L6] uninitialised state variables

There are some state variables that are not initialised in the initialize function. For example, https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L44-L45 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L48-L50 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L40

Notes and Information

[N1] guardian role should be put in SettAccessControl instead of StakedCitadel

Guardian role seems to be an externally owned account and is used the same way as other authorised actors such as governance. Thus, they could be all together in the SettAccessControl contract.

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

This will also put the modifiers related to Guardian and setGuardian to the access control contract. https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L261-L266 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L560-L566

[N2] Not using AccessControlUpgradeable from OpenZepplin

SettAccessControl implements a role-based access control system, which has already had a well-thoughtout implementation from OpenZepplin.

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/lib/openzeppelin-contracts-upgradeable/contracts/access/AccessControlUpgradeable.sol

Also consider using OpenZepplin implementation for GlobalAccessControl and GloablAccessControlManaged. https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/GlobalAccessControl.sol https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol

[N3] Not using abstract contract

Since SettAccessControl is not meant to be deployed on its own, but only to inherit as a base contract, one could mark it abstract. https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/SettAccessControl.sol#L9

Similarly, GloablAccessControlManaged can be abstract too.

[N4] Not using modifier keyword

Many functions with only one require check prior to function execution are not labelled using the modifier keyword. Using the modifier keyword helps with readability.

Instances like this are https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/SettAccessControl.sol#L15-L31

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

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

[N5] Unused Variables

lastAdditionalTokenAmount is defined but not used.

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

[N6] Repeated Code

_pool is the same as _before in https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L772-L773

[N7] uint32 instead of uint256 for efficient storage packing

The 8 fee variables below are meant to be between 0-10000. Thus they can use uint32 instead of uint256 to cover the entire range and get packed together into one single storage slot.

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

[N8] TODOs in code

There are still TODOs in the production code. Instances are

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

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

[N9] Single Line comments instead of multiline comments

There are many instances of comments that are in paragraphs but written with single line comment /// instead of /* */. For example

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

[N10] Constants not using ALL CAPS

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