Badger Citadel contest - gs8nrv's results

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

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L822 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#L830

Vulnerability details

Impact

In the internal _withdraw(uint256 _shares), if the contract can't withdraw enough funds from the strategy, then users lose part of its shares for nothing. Indeed the function first burn the shares wanted to be withdrawn (2). Then it tries to have a large enough balance to pay the user, but if there is not enough sitting idle in the contract, it will asks the strategy for a pay back (4). If the amount withdrawn is not enough then the contract take it into account in the amount paid back to the users (3). So amount can be less than the one computed by the shares amount, but these shares are already burnt, while a smaller amount < shares should have been burnt as the amount send to the vesting was actually less than expected.

Proof of Concept

Tested with hardhat ts

Tools Used

(1) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L822 (2) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L811 (3) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L830 (4) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L818

Recommended Mitigation Steps

I advise to first see how much can be withdrawn and then burn the shares amount corresponding to the final vested amount.

(1)https://github.com/ampleforth/market-oracle/blob/5e7fd1506784f074748ab6bd5df740ca2227b14f/contracts/MedianOracle.sol#L200 (2) https://cs.stackexchange.com/questions/1914/find-median-of-unsorted-array-in-on-time

Search of the median is done in O(n^2) while this can be reduce to O(n), this is a high optimisation as price computation is called for each oracle update.