StakedCitadel depositors can be attacked by the first depositor with depressing of vault token denomination

[code423n4]

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

[GalloDaSballo]

Disagree with the dramatic effect the warden is implying.

Agree with the finding as this is a property of vault based systems

[GalloDaSballo]

Also worth noting that anyone else can still get more deposits in and get their fair share, it's just that the first deposit would now require a deposit of at least vault.balanceOf in order to get the fair amount of shares (which at this point would be rebased to be 1 = prevBalanceOf)

[jack-the-pug]

I believe this is a valid High even though the precondition of this attack is quite strict (the attacker has to be the 1st depositor).

The impact is not just a regular precision loss, but with the pricePerShare of the vault being manipulated to an extreme value, all regular users will lose up to the pricePerShare of the deposited amount due to huge precision loss.