StakedCitadel withdraw burns extra shares on Strategy liquidity issues

[code423n4]

Lines of code

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

Vulnerability details

Impact

If for any reason there is not enough tokens for user withdraw, the StakedCitadel._withdraw will burn extra shares than it's due for the amount actually withdrawn.

The discrepancy can be arbitrary large, up to user losing all the shares and receiving nothing in return, which happens when there are no tokens on StakedCitadel balance (token.balanceOf(address(this)) is 0) and nothing was retrieved from the Strategy (IStrategy(strategy).withdraw(_toWithdraw) gives nothing).

The deficit can happen on any liquidity related issues, say Strategy is lending pool related and has liquidity shortage at the moment.

The correct behavior here is to burn only what corresponds to the amount actually withdrawn.

Placing severity to be high as for user this is principal funds loss scenario as her shares are removed irrevocably and there is no way to compensate for the amount not received, which this way is lost. The size of the loss is determined by current liquidity situation.

Proof of Concept

User requested shares are burned unconditionally in the beginning of the function:

uint256 r = (balance() * _shares) / totalSupply(); _burn(msg.sender, _shares);

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

While later r can be reduced:

if (_diff < _toWithdraw) {r = b + _diff;}

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

There is no compensational mint done, so the _toWithdraw - _diff difference is lost for the user.

Recommended Mitigation Steps

As the function is nonReentrant the order of operations can be adjusted to fit the logic.

Consider burning the shares only when the final amount is determined: if (r > 0 && r_final > 0) {_burn(msg.sender, (r_final * _shares) / r);

[GalloDaSballo]

I must disagree with this one, slippage on withdrawal must be paid by the caller, (yes, at the callers detriment), otherwise the vault is "the sucker"

Anytime the vault takes a loss (socializing losses) the vault system is vulnerable to value extraction.

The warden here seems to be implying that allowing value extraction is a good thing, which I completely disagree, that's how we get on the rekt leaderboard

[GalloDaSballo]

To bring further detail:

The math for withdrawal is properly calculated based on balanceOf which accounts for assets in the vault, the strategy (as want) and in the pool.

If the math says that X shares are worth Y tokens, and then we incur slippage, then that means that in withdrawing from the pool we incurred in slippage.

This slippage MUST be paid by the caller else we're socializing losses and building a vulnerable system.

For that reason, I must disagree categorically with the finding

[jack-the-pug]

Dup #183

[dmitriia]

This was actually targeted to the case of the lack of ability of a Strategy to withdraw full amount requested. Say Strategy invests in Aave pool, which has liquidity shortage. User shouldn't pay huge slippage in this case as the assets are only temporary inaccessible.

Regarding regular slippage it is fair to say that it shouldn't be socialized.

I see a minimum amount argument as a way to go here, so a user can decide whether this slippage good or not, no matter what its origin is.

[dmitriia]

And, as it implies a possibility of user's loss, I don't think the severity should be drastically lowered for the pack.

[dmitriia]

For 'Strategy out of scope' thought: a code that deals with a general case (any token, any strategy, etc), should deal with all significant particulars that can emerge there, it is not a job of downstream code as it by definition is focused on a particular case only.