Platform: Code4rena
Start Date: 14/04/2022
Pot Size: $75,000 USDC
Total HM: 8
Participants: 72
Period: 7 days
Judge: Jack the Pug
Total Solo HM: 2
Id: 110
League: ETH
Rank: 7/72
Findings: 4
Award: $3,956.28
🌟 Selected for report: 0
🚀 Solo Findings: 0
896.081 USDC - $896.08
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L816
r is the user's part of the contract balance, but is supposed to be the user's part of the total funds, including the strategy funds.
therefore the check at line 816 will always return false because the user's part of the contract balance is smaller than the contract balance.
the user doesn't get their part of the strategy funds.
The user loses funds if they withdraw when there are funds in the strategy.
chnage r to be (total funds) * _shares / totalSupply();
#0 - GalloDaSballo
2022-04-22T23:04:04Z
I believe balance in our contract was improperly changed.
Wdyt @dapp-whisperer
#1 - jack-the-pug
2022-05-30T07:30:28Z
Dup #74
2782.1219 USDC - $2,782.12
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881
in deposit, when the ratio totalSupply / balance is very high, the amount of the minted shares can round down to zero.
Let's say that the token is USDC. Alice is the first one to deposit in StakedCitadel. she deposits 1 basic unit of USDC (10**-6 dollar), therefore minting one share. then she transferred 1 million USDC to the contract. then bob deposits 500,000 USDC. (500000 * 10**6 basic units) the amount of shares he gets is 500000 * 10**6 * 1 / (1000000* 10**6) = 0 therefore the number of shares didn't change but the balance increased by 500000 dollars. Alice can now withdraw her share and receive her funds back together with bob funds, as he doesn't have any shares.
change to:
if (totalSupply == 0) { shares = _amount * 10 ** 18; } else {
#0 - GalloDaSballo
2022-04-22T23:00:55Z
Have to acknowledge the finding but have to disagree with severity as well as mitigation.
Mitigation would now rebase all shares to have 18 decimals more than normal, it would still have the same issue as integer division can always be brought to eat some value.
As for impact, this is mostly avoided by minting 1e6 or 1e18 shares as the first deposit
#1 - jack-the-pug
2022-05-30T08:41:11Z
Dup #217
184.248 USDC - $184.25
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L202
citadelAmount_ is initialized to 0 by default, and then if the discount is greater than zero, it is set to the amount out.
But if the discount is 0, it is not set to the amount out and citadelAmount_ stays 0 and then divided by assetDecimalsNormalizationValue, but still stays zero.
Therefore anyone who will deposit when there is no discount will lose their funds.
remove the if check. this will also work if the discount is zero.
#0 - shuklaayush
2022-05-11T20:43:06Z
#18
#1 - jack-the-pug
2022-05-29T06:48:45Z
Dup #218
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, Hawkeye, Jujic, MaratCerby, Picodes, Ruhum, SolidityScan, TerrierLover, TomFrenchBlockchain, TrungOre, VAD37, Yiko, berndartmueller, cmichel, csanuragjain, danb, defsec, delfin454000, dipp, ellahi, fatherOfBlocks, georgypetrov, gs8nrv, gzeon, horsefacts, hubble, hyh, ilan, jah, joestakey, kebabsec, kenta, kyliek, m9800, minhquanym, oyc_109, p_crypt0, peritoflores, rayn, reassor, remora, rfa, robee, scaraven, securerodd, shenwilly, sorrynotsorry, tchkvsky, teryanarmen, z3s
93.8321 USDC - $93.83
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L717
the function earn can be called multiple times, and each time, more money will go to the strategy.
toEarnBps=50%
the contract balance is 1000.
in the first call to earn, 500 token will be sent to the strategy.
now the contract balance is 500.
in the second call to earn, 250 token will be sent to the strategy.
this can be repeated multiple times until there is only 1 token left (and then available() will return 0).
Keep track of the tokens that were sent to the strtegy, and use them in available calculations.
#0 - GalloDaSballo
2022-04-23T00:57:11Z
I don't think there's any risk in sending the tokens to the strategy, especially if it will be the bricked strategy, The only thing is it will cost some extra gas to withdraw.
For this reason I have to dispute, ultimately the warden is stating a fact, but there is no impact
#1 - jack-the-pug
2022-05-30T07:37:54Z
Downgrading to QA as the warden did not demonstrate how this can lead to loss of funds or other notable damage.