Badger Citadel contest - TomFrenchBlockchain's results

List of contents

Low

• Performance fee may be set above maxPerformanceFee
• Funding.sol accepts invalid discount limits
• Funding.sol accepts invalid price bounds
• Funding.sweep sweeps all citadel rather than just excess

Non-Critical

• Incorrect comment on Funding.sweep

Low

Performance fee may be set above maxPerformanceFee

`StakedCitadel defines a max performance fee which may be set at any value up to 30%

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L533-L542

This is broken up into two components, performance fees paid to the strategist and performance fees paid to governance

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L618-L636 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L638-L656

When setting both these components of the performance fee, it's checked that they don't exceed the cap individually but not that they don't exceed the cap together. The real performance fee cap is then 2 * maxPerformanceFee.

Funding.sol accepts invalid discount limits

Funding.sol checks that the provided max and min discount factors don't exceed a maximum discount but does not check that _minDiscount <= _maxDiscount.

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

This results in the discountManager being unable to provide a value which would satisfy these constraints in setDiscount().

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L265-L276

Funding.sol accepts invalid price bounds

Funding.sol accepts minimum and maximum price bounds without any validation that the minimum is less than the maximum.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L397-L406

It's then possible to put the contract in a state such that the price may not be updated until the bounds are fixed.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L427-L430

Funding.sweep sweeps all citadel rather than just excess

The natspec of Funding.sweep suggests that calling sweep(citadel) would be safe in that it would leave enough CTDL to satisfy any outstanding claims. The function implementation however has no special logic for CTDL. https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L310-L312

Non-Critical

Incorrect comment on Funding.sweep

Here we say in a comment that asset builds up on Funding until manually swept to the treasury. https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L333

This is not true as on each sale it is sent directly to saleRecipient. https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L180

List of contents

• Cacheable token decimals in Funding.sol
• FundingParams struct can be packed
• Storage variables in Funding can be packed
• Can use unchecked_at in CitadelMinter.getFundingPoolWeights()
• Return amount of xCitadel minted on deposit calls to avoid needing to query balance
• fundingBps, stakingBps and lockingBps can all be packed in CitadelMinter
• VestingParams can be packed into 2/3 slots
• Report struct can be packed into a single slot
• MedianOracle storage can be packed
• KnightingRound variables boughtAmounts and hasClaimed can be merged into a packed struct
• xCTDL total supply can be cached in _mintSharesFor

Gas Optimisations

Cacheable token decimals in Funding.sol

In Funding.getStakedCitadelAmountOut we may a query to get citadel.decimals() on each call, we can avoid an external call by storing the value of 10**citadel.decimals() on this contract (similar to assetDecimalsNormalizationValue).

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L225

FundingParams struct can be packed

`FundingParams can be packed to reduce the number of SLOADs used when interacting with it.

discount, minDiscount and maxDiscount all fit in uint16s as they're capped at 10000 to fit in the same slot as discountManager. assetCumulativeFunded and assetCap could likely be restricted to uint128s safely.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L46-L53

Storage variables in Funding can be packed

These three variables can be safely packed into a single slot by restricting assetDecimalsNormalizationValue to a uint88 or less. (uint88 will work for all token decimals up to 26)

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/Funding.sol#L39-L43

Can use unchecked_at in CitadelMinter.getFundingPoolWeights()

We we know we won't query past the end of fundingPools there's no need to use the checked version.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/CitadelMinter.sol#L153 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/CitadelMinter.sol#L345

Return amount of xCitadel minted on deposit calls to avoid needing to query balance

Here we need to query the CitadelMinter's balance of xCitadel before and after depositing in order to calculate how much was minted. This would be cheaper if xCitadel returned the amount of shares minted so we can avoid doing these external calls.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/CitadelMinter.sol#L193-L197

fundingBps, stakingBps and lockingBps can all be packed in CitadelMinter

lastMintTimestamp is a timestamp so will easily fit in a uint64. fundingBps, stakingBps and lockingBps all fit in uint16s as they're capped at 10000.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/CitadelMinter.sol#L40 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/CitadelMinter.sol#L48-L50

VestingParams can be packed into 2/3 slots

unlockBegin and unlockEnd are timestamps and so will easily fit in uint64s. lockedAmounts and claimedAmounts could probably be safely made into uint128s as an overflow would require a user to cycle so my Citadel through the contract to be unfeasible.

https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadelVester.sol#L24-L27

Report struct can be packed into a single slot

timestamp is a timestamp (obviously) so will easily fit in a uint64. If payload can be restricted to uint192 then Report will fit in a single struct.

https://github.com/ampleforth/market-oracle/blob/5e7fd1506784f074748ab6bd5df740ca2227b14f/contracts/MedianOracle.sol#L23-L26

MedianOracle storage can be packed

reportExpirationTimeSec, reportDelaySec and minimumProviders will all fit in uint64s and so can be stored in a single slot.

https://github.com/ampleforth/market-oracle/blob/5e7fd1506784f074748ab6bd5df740ca2227b14f/contracts/MedianOracle.sol#L41-L49

KnightingRound variables boughtAmounts and hasClaimed can be merged into a packed struct

The two mappings boughtAmounts and hasClaimed can be merged into a single struct of the form:

struct BuyInfo{
    uint248 boughtAmounts;
    bool hasClaimed;
}

This will save a SLOAD when calling claim().

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

xCTDL total supply can be cached in _mintSharesFor

Here we perform 2 SLOADs for the total supply whereas we could just query once and cache it in storage.

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