search

code-423n4 / 2022-04-badger-citadel-findings

0 stars 1 forks source link

StakedCitadel withdraw when available balance is not sufficient --> rekt some of the capital of the user #183

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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.

GalloDaSballo commented 2 years ago

See discussion on #210 Categorically disagree

jack-the-pug commented 2 years ago

The validity of this issue highly depends on how the strategy is designed and implemented, as the strategy is out of scope, I will downgrade this to QA.