Staked Citadel function `balance` and `reportHarvest`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L293 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L898 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L908 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L922 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L927 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L772 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L811 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L288 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L396 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L416 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L904

Vulnerability details

Impact

In the StakedCitadel.sol, the call to balance() (1) which is supposed to represent the total assets (currently available + delegated to strategies) is wrong and only returns the available balance (directly transferrable, sitting idle on the contract).

This has a lot of implications:

• In the functions _handleFees()(2): It makes wrong all computation of management_fee(3) and then the minted shares to strategist (5) and treasury (4).

• In the functions _depositFor() (6) and _withdraw() (7): where the number of shares are computed with balance and in the first function it will return too many shares and too few underlying tokens in the latter one.

• In the function getPricePerFullShare() (8): where one share is supposed to be equal to (balance() * ONE_ETH) / totalSupply(), while this would be correct with the right definition of balance(), this highly undervalue the current price per share.

On a similar topic, in the reportHarvest(harvestedAmount) (9): I am unsure what harvestedAmount represents. If this is only the profits made by the strategy:

• assetsAtLastHarvest (10) doesn't have a real meaning (as set at 0 when there is no gain)
• in _handleFees(...) the management_fee (3) would also be wrong if balance() were the right amount, as we would give a management fee on full capital at each report.

If harvestedAmount represents the current assets handle by strategy, then this would be similar to balance(): performance fees(11) would be wrong as taken on the full assets and therefore too high.

Proof of Concept

(1) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L293 (2) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L898 (3) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L908 (4) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L922 (5) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L927 (6) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L772 (7) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L811 (8) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L288 (9) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L396 (10) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L416 (11) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L904

Tools Used

Testing with mocks contract + hardhat ts

Recommended Mitigation Steps

Modify balance(), by keeping the available balance + a call to the strategy balance (not only on the idle assets but the used assets. On the harvestedAmount this should be only the gain reported

[GalloDaSballo]

@dapp-whisperer any logic behind changing the "default" balance from v1.5?

[jack-the-pug]

Dup #74