Badger Citadel contest - hyh's results

Lines of code

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

Vulnerability details

Impact

An attacker can become the first depositor for a recently created StakedCitadel contract, providing a tiny amount of Citadel tokens by calling deposit(1) (raw values here, 1 is 1 wei, 1e18 is 1 Citadel as it has 18 decimals). Then the attacker can directly transfer, for example, 10^6*1e18 - 1 Citadel to StakedCitadel, effectively setting the cost of 1 of the vault token to be 10^6 * 1e18 Citadel. The attacker will still own 100% of the StakedCitadel's pool being the only depositor.

All subsequent depositors will have their Citadel token investments rounded to 10^6 * 1e18, due to the lack of precision which initial tiny deposit caused, with the remainder divided between all current depositors, i.e. the subsequent depositors lose value to the attacker.

For example, if the second depositor brings in 1.9*10^6 * 1e18 Citadel, only 1 of new vault to be issued as 1.9*10^6 * 1e18 divided by 10^6 * 1e18 will yield just 1, which means that 2.9*10^6 * 1e18 total Citadel pool will be divided 50/50 between the second depositor and the attacker, as each have 1 wei of the total 2 wei of vault tokens, i.e. the depositor lost and the attacker gained 0.45*10^6 * 1e18 Citadel tokens.

As there are no penalties to exit with StakedCitadel.withdraw(), the attacker can remain staked for an arbitrary time, gathering the share of all new deposits' remainder amounts.

Placing severity to be high as this is principal funds loss scenario for many users (most of depositors), easily executable, albeit only for the new StakedCitadel contract.

Proof of Concept

deposit() -> _depositFor() -> _mintSharesFor() call doesn't require minimum amount and mints according to the provided amount:

deposit:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L309-L311

_depositFor:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L764-L777

_mintSharesFor:

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

When StakedCitadel is new the _pool = balance() is just initially empty contract balance:

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

Any deposit lower than total attacker's stake will be fully stolen from the depositor as 0 vault tokens will be issued in this case.

References

The issue is similar to the TOB-YEARN-003 one of the Trail of Bits audit of Yearn Finance:

https://github.com/yearn/yearn-security/tree/master/audits/20210719_ToB_yearn_vaultsv2

Recommended Mitigation Steps

A minimum for deposit value can drastically reduce the economic viability of the attack. I.e. deposit() -> ... can require each amount to surpass the threshold, and then an attacker would have to provide too big direct investment to capture any meaningful share of the subsequent deposits.

An alternative is to require only the first depositor to freeze big enough initial amount of liquidity. This approach has been used long enough by various projects, for example in Uniswap V2:

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L209-L215

Vulnerability details

Impact

User provided asset funds will be lost, i.e. 100% to be frozen in the contract, as the system will not give away any Citadel in return.

The issue is that when Funding's funding.discount is zero the getAmountOut() will return zero for any given _assetAmountIn so nothing will be deposited on the behalf of msg.sender.

Placing severity to be high as that's clear violation of system logic, while the impact is user funds become frozen in the contract and had to be recovered manually.

Proof of Concept

Whenever funding.discount is zero, the citadelAmount_ will be zero as well by default, as citadelAmount_ is defined only when funding.discount > 0:

uint256 citadelAmountWithoutDiscount = _assetAmountIn * citadelPriceInAsset;

if (funding.discount > 0) {
   citadelAmount_ = (citadelAmountWithoutDiscount * MAX_BPS) / (MAX_BPS - funding.discount);
}

citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L209-L215

This way getAmountOut() will produce nothing for any amount of asset invested:

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

Notice that the issue was introduced lately with discounts, as it didn't existed in the previous version:

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L221-L223

Recommended Mitigation Steps

Consider either replacing citadelAmountWithoutDiscount with citadelAmount_ or adding else to if (funding.discount > 0) {...} else {citadelAmount_ = citadelAmountWithoutDiscount;} logic

1. getPricePerFullShare is correct only for tokens with 18 decimals (low)

Impact

The price returned by getPricePerFullShare() will have incorrect magnitude if contract token has decimals other than 18.

Keeping it low severity as Citadel and xCitadel have 18 decimals, but such a hard code in the contract that can deal with an arbitrary token isn't correct.

Proof of Concept

If there is no supply getStakedCitadelAmountOut() will return 1e18, if there is supply it will return a value with token's decimals:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L281-L289

getStakedCitadelAmountOut() uses getPricePerFullShare(), so can produce results of a wrong magnitude if used with a non-18 decimals token:

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

Recommended Mitigation Steps

Scale the getPricePerFullShare() result

2. vestingDuration can be set to zero breaking claimableBalance logic (low)

Impact

claimableBalance will throw low level error.

Proof of Concept

In order to keep StakedCitadelVester.claimableBalance working the vestingDuration should be kept positive:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L116-L117

But vestingDuration can be set to zero:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L167

Recommended Mitigation Steps

Consider introducing min and max duration checks (say 0 and 20 years)

3. Funding CTDL price comments and name are wrong, being inverted (low)

Impact

It is highly misleading at the moment, so it's hard to understand the logic of the contract.

Proof of Concept

The prices are CTDL per ASSET, while the comments state it otherwise (ASSET per CTDL):

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

Prices are CTDL/$ASSET, CTDL per ASSET:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/docs/oracles.md

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/test/BaseFixture.sol#L199

Also, citadelPriceInAsset name in Funding is misleading, it is AssetPriceinCitadel.

Recommended Mitigation Steps

Update the name and comments according to the existing logic.

4. Funding.deposit's _minCitadelOut argument has wrong description (low)

Impact

The end effect is that _minCitadelOut argument has no description.

Proof of Concept

The arg description comment is not correct here:

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

Recommended Mitigation Steps

Replace the line with slippage checking description of the argument

5. SettAccessControl's role setting should be two-step actions (non-critical)

Impact

One step is errror prone, and it's used even for governance.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/SettAccessControl.sol#L35-L54

Recommended Mitigation Steps

Introduce two-step propose-set logic

6. Strategist address check is redundant (non-critical)

Proof of Concept

Strategist address cannot be zero as it is set once on initialization to a non-zero address and can't be modified thereafter:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L185

So the address part of the check is redundant:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L925-L931

Recommended Mitigation Steps

Remove the check to simplify

7. initializeLastMintTimestamp is redundant (non-critical)

Proof of Concept

getMintable resets input lastMintTimestamp on seeing that it's too low (the only case is lastMintTimestamp being zero when it's not initialized yet), so effectively what initializeLastMintTimestamp doing is already fully achieved on getMintable first run.

In the end of mintAndDistribute, which is the only place getMintable is called, lastMintTimestamp is set to current timestamp, and all the subsequent runs are incrementally using current time only.

Nothing is achieved if initializeLastMintTimestamp be run first instead, so this way initializeLastMintTimestamp isn't needed and can be safely removed.

Recommended Mitigation Steps

Remove the function

8. getMintable breaks if called in the same block as setMintingStart (non-critical)

Impact

When getMintable is called within the same block as cachedGlobalStartTimestamp is set there will be low level subtraction error.

Proof of Concept

getMintable doesn't check that block.timestamp > cachedGlobalStartTimestamp:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L84-L101

Recommended Mitigation Steps

Consider moving L99-101 before L94:

if (lastMintTimestamp < cachedGlobalStartTimestamp) {
   lastMintTimestamp = cachedGlobalStartTimestamp;
}

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L99-L101

to be before block.timestamp > lastMintTimestamp require:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L94

9. Debug functions left in the code (non-critical)

Impact

Unused debug functions bloat production code. Using them can bring in various malfunctions. The best way to ensure there is no usage left is to remove them.

Proof of Concept

SupplySchedule's _setEpochRates, getMintableDebug to be removed on release:

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

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L178

_setEpochRates is used on init:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L45

getMintableDebug is referenced by ISupplySchedule:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/interfaces/citadel/ISupplySchedule.sol#L7

Recommended Mitigation Steps

Consider updating the initialize(), ISupplySchedule and removing _setEpochRates, getMintableDebug

10. Funding, GlobalAccessControl, KnightingRound have open TODOs (non-critical)

Impact

TODOs clutter production code as release fixes the implementation.

Proof of Concept

Funding:

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

Recommended Mitigation Steps

Consider removing TODOs from production code.

11. _onlyAuthorizedActors name is misleading (non-critical)

Impact

Code is less readable and transparent.

Proof of Concept

_onlyAuthorizedActors is an abstract name with GovernanceOrKeeper implementation:

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/SettAccessControl.sol#L26

Recommended Mitigation Steps

Consider renaming to _onlyGovernanceOrKeeper in order to match the logic.

12. MedianOracle compiler has to be updated (non-critical)

Impact

Various malfunctions are possible on mixing of compiling versions as the amount of breaking changes is substantial.

For example, an operational mistake can lead it to be accidentally deployed with never version without testing.

Proof of Concept

MedianOracle use 0.4.24 compiler:

https://github.com/ampleforth/market-oracle/blob/master/contracts/MedianOracle.sol#L1

The rest of the system is 0.8.12.

Recommended Mitigation Steps

Consider testing MedianOracle with 0.8.12 compiler and updating it so it be aligned with the rest of the system.