Post Judge QA

[JeeberC4]

🧑‍⚖️ Post-judging QA; deadline: June 6, 2023 10:00AM PST

1. Please post feedback about specific findings on the relevant issue in this repo.
2. More general feedback for the judge can be shared here.

The judge for this contest is @GalloDaSballo

Reminders

• Please keep your comments as fact-based as possible.
• C4 judges have final authority in determining validity and severity.
• You are welcome to start new discussions here!
• Please link to specific issues wherever relevant.

Thank you!

[Co0nan]

Hi @GalloDaSballo, just add more context on #392.

[said017]

Hi @GalloDaSballo. thank you for your reviews and judging sir. I've left a comment on my issue #294

[ToonVanHove]

Hi @GalloDaSballo!

Could you explain why #52 (mine) is marked as OOS? Known issue L-06 only mentions that __Ownable_init() and __ReentrancyGuard_init() are missing in StrategyManager.sol, DelayedWithdrawalRouter.sol, EigenPodManager.sol and EigenPod.sol (which would affect the proxy contract). However it does not mention anything about a missing _disableInitializers() in DelayedWithdrawalRouter.sol, as described in #52 (which would affect to implementation contract).
My knowledge of upgradeable contracts could use some work, so forgive me if this considered to be the same issue.

Thanks in advance!

[GalloDaSballo]

Happy Monday @ToonVanHove

I appreciate the question coming from a place of wanting to learn

I would start by saying that issues related to initializers are limited in their damage as the worst case scenario is the re-deployment of a non-working contract.

Meaning that I would start by recommending you send such a finding as Low Severity and not Medium.

The automated report serves as a catch-all to say that the logics are not initialized

Mitigation would be to add the initializer modifier to the constructor (see N-7)

By doing that, the logics cannot be hijacked.

In terms of front-runs to the proxies, the script is using upgradeAndCall

Which means that the call to initialize is done in the same tx as the deployment (done in the constructor of the proxy)

Happy to explain further, perhaps on office hours, for all to benefit

[sashik-eth]

@GalloDaSballo please check my comments in #188 and #361. Also, it looks like #210 was incorrectly marked as a duplicate of itself.

[GalloDaSballo]

Thank you @sashik-eth for the thorough comments, have reacted accordingly

[d3e4]

@GalloDaSballo Please have a look at #453 and #455, where I have left comments.

I also think #343 and its 11 duplicates have been incorrectly assessed. I think most of them are invalid, including the main issue, for reasons detailed below. Only #445 and #193 describe a valid issue.

Most of the duplicates simply describe the standard conception of an inflation attack where a first depositor inflates the share price to steal from the second depositor. This has already been mitigated against by making the attack 1e9 times more expensive.
What has been overlooked in the mitigation is that the inflation attack is more general than targeting the second depositor. The true issue in the inflation attack is an inflation of the rounding losses which accrue to the first depositor on every subsequent deposit. This implies a leak of value, small on each deposit/withdrawal, but building up over time.
#445 explicitly reports this issue.
#193 focuses on inflating the value of the minimum 1e9 shares left (note that these could be retrieved if all owners cooperated to transfer their shares to one account and withdrew them at once, and that there is no profit for the attacker). But it at least alludes to the issue described in #445: “Because he deposited before the shares were deflated, he still gets the tokens he sent to the Strategy directly back, as well as a small part of subsequent stakers' tokens.” It is unclear whether the report realised that this can be meaningfully exploited.

#445, and perhaps #193, are the only reports that make this observation; that the 1e9 minimum share requirement is an insufficient mitigation which still leaves rooms for a direct compromise of assets.

Invalid issues among #343 and duplicates

I have left these comments on each issue, but include them here as well for easy context.

#343
The issue described in #343 is not a lack of slippage protection. This is rather the suggested mitigation. It is claimed that the lack of slippage protection “will put the user who deposits at risk as anyone can front run his transcation by depositing a large amount and thus when the user transactions proceeds he will receive a very small amount of shares not the amount he was expecting.”. Depositing doesn’t change the share price, so this is obviously false, but I’ll (generously) assume that the warden meant to frontrun with a direct transfer of funds as the first depositor.
It is not an issue in itself that the depositor receives a smaller amount than expected. It is only nominally smaller, but still worth as much. Except of course due to rounding losses. So this describes a normal inflation attack. The 1e9 minimum shares requirement is an adequate mitigation against this type of inflation attack. Therefore #343 fails to describe any real issue that hasn’t already been mitigated.
A slippage check cannot even protect against this; that would entail receiving more than one should. The closest less than one should receive is precisely the amount lost due to inflation of the rounding loss. To minimise rounding losses one would have to adjust the amount in rather than the amount out.

#427
There is no issue here. If priorTokenBalance is 1, then amount*totalShares is the correct amount of shares to mint in order to withdraw the same amount as was deposited. Withdrawing gives _tokenBalance() * amountShares / priorTotalShares which in our case if we withdraw our shares again is (1 + amount) * (amount*totalShares) / (totalShares + amount*totalShares) = amount.

#418
This is not an inflation/donation attack. The donation in the PoC is not necessary. The described vulnerability is based on the strategy’s moving the tokens out of the contract so that withdrawal is calculated on a partial balance. But removing tokens is a functionality only an inheriting contract would have, and it would then have to be assumed that it overrides _tokenBalance() to reflect this new state of affairs.

#330
The 1e9 restriction applies equally to the strategy manager. This is checked in StrategyBase.deposit() so the strategy manager cannot circumvent this.

#249
This describes a standard inflation attack, which the 1e9 minimum shares requirement mitigates against. No attempt is made to explain how this mitigation is insufficient. The attack in the PoC returns only dust profit, which is precisely the intended effect of the mitigation already in place.

#153
Donating just before withdrawing simply splits the donation with all other shares. There is no profit to be made here.

#118
This report only states that funds sent directly to the strategy by mistake may be lost, and that a depositor might not get the nominal exchange rate he desired. These are not valid concerns. The second point doesn’t even imply a loss of funds.

#110
The 1e9 minimum shares requirement makes it 1e9 times harder to inflate the tokens per share ratio. Donating 1000e18 tokens would only inflate a share to 1e13 tokens per share. This is still a very small amount. In fact, this is precisely what is intended by the mitigation of requiring a minimum of 1e9 shares.

#89
This report attempts an inflation attack in spite of the 1e9 minimum shares requirement by simply upping the ante to an entirely unrealistic donation of 1,000,000,001 ethers. To push the required donation up to unrealistic amounts was precisely the intention behind the 1e9 mitigation.

#73
This is invalid for the same reasons as #110. 1e9 shares can be inflated to represent something on the order of 1e13 tokens. This is still a very small amount, which is about as acceptable to have remaining in the contract as 1e9. 1e9 was choses because it is many orders of magnitude greater than 1 but also equally many orders of magnitude smaller than 1e18.

[GalloDaSballo]

I appreciate the time you have spent commenting on the issues, I have written a POC that demonstrates that as you stated, the impact is marginal under all reasonable circumstances

from random import random

INITIAL = 1e9
UNIT = 1e18

"""
    if (totalShares == 0) {
        newShares = amount;
    } else {
        uint256 priorTokenBalance = _tokenBalance() - amount;
        if (priorTokenBalance == 0) {
            newShares = amount;
        } else {
            newShares = (amount * totalShares) / priorTokenBalance;
        }
    }
"""

def get_shares_out(amount_in, token_balance, total_shares):
    if(total_shares == 0):
        return amount_in
    else:
        priorTokenBalance = token_balance - amount_in
        if (priorTokenBalance == 0):
            return amount_in
        else:
            print("get_shares_out")
            print("amount_in", amount_in)
            print("total_shares", total_shares)
            print("priorTokenBalance", priorTokenBalance)
            return amount_in * total_shares // priorTokenBalance

def get_coll_in(amountShares, tokenBalance, priorTotalShares):

        if (priorTotalShares == amountShares):
            return tokenBalance
        else:
            return tokenBalance * amountShares // priorTotalShares


MIN_DEPOSIT = 1e9

GRIEFED_DEPOSIT = 1000e18

def main():
    total_supply = 0
    token_balance = 0

    ## Minimum Deposit
    new_shares = get_shares_out(MIN_DEPOSIT, token_balance, total_supply)

    total_supply += new_shares
    token_balance += MIN_DEPOSIT

    print("total_supply", total_supply)
    print("token_balance", token_balance)

    ATTACK_AMOUNTS = [1e18, 10e18, 100e18, 1000e18, 10000e18, 100_000e18, 1_000_000e18]

    cached_token_balance = token_balance

    ## REBASE with amounts
    for grief_amount in ATTACK_AMOUNTS:
        print("")
        print("")
        print("grief_amount", grief_amount)

        ## Rebase first
        cached_token_balance = token_balance
        cached_token_balance += grief_amount

        print("cached_token_balance", cached_token_balance)

        ## Then perform grief
        cached_token_balance += GRIEFED_DEPOSIT
        griefed_receives = get_shares_out(GRIEFED_DEPOSIT, cached_token_balance, total_supply)
        print("griefed_receives", griefed_receives)
        total_supply += griefed_receives

        grifed_withdraws = get_coll_in(griefed_receives, cached_token_balance, total_supply)
        print("grifed_withdraws", grifed_withdraws)

        print("Loss as %", 100 - (grifed_withdraws / GRIEFED_DEPOSIT * 100))
        print("Loss in ETH", (GRIEFED_DEPOSIT - grifed_withdraws) / 1e18)

Which will output

Running 'scripts/fuzz.py::main'...
total_supply 1000000000.0
token_balance 1000000000.0


grief_amount 1e+18
cached_token_balance 1.000000001e+18
get_shares_out
amount_in 1e+21
total_shares 1000000000.0
priorTokenBalance 1.0000000009999483e+18
griefed_receives 999999999000.0
grifed_withdraws 9.999999999999999e+20
Loss as % 1.4210854715202004e-14
Loss in ETH 1.31072e-13


grief_amount 1e+19
cached_token_balance 1.0000000001e+19
get_shares_out
amount_in 1e+21
total_shares 1000999999000.0
priorTokenBalance 1.0000000000999948e+19
griefed_receives 100099999889990.0
grifed_withdraws 1e+21
Loss as % 0.0
Loss in ETH 0.0


grief_amount 1e+20
cached_token_balance 1.00000000001e+20
get_shares_out
amount_in 1e+21
total_shares 101100999888990.0
priorTokenBalance 1.0000000000099995e+20
griefed_receives 1011009998879790.0
grifed_withdraws 1e+21
Loss as % 0.0
Loss in ETH 0.0


grief_amount 1e+21
cached_token_balance 1.000000000001e+21
get_shares_out
amount_in 1e+21
total_shares 1112110998768780.0
priorTokenBalance 1.0000000000009998e+21
griefed_receives 1112110998767668.0
grifed_withdraws 9.999999999999999e+20
Loss as % 1.4210854715202004e-14
Loss in ETH 1.31072e-13


grief_amount 1e+22
cached_token_balance 1.0000000000001e+22
get_shares_out
amount_in 1e+21
total_shares 2224221997536448.0
priorTokenBalance 1.0000000000001e+22
griefed_receives 222422199753622.0
grifed_withdraws 9.999999999999978e+20
Loss as % 2.2737367544323206e-13
Loss in ETH 2.228224e-12


grief_amount 1e+23
cached_token_balance 1.00000000000001e+23
get_shares_out
amount_in 1e+21
total_shares 2446644197290070.0
priorTokenBalance 1.00000000000001e+23
griefed_receives 24466441972900.0
grifed_withdraws 9.999999999999816e+20
Loss as % 1.8332002582610585e-12
Loss in ETH 1.835008e-11


grief_amount 1e+24
cached_token_balance 1.0000000000000009e+24
get_shares_out
amount_in 1e+21
total_shares 2471110639262970.0
priorTokenBalance 1.0000000000000009e+24
griefed_receives 2471110639262.0
grifed_withdraws 9.999999999996088e+20
Loss as % 3.9122483030951116e

Ultimately the loss is limited to dust amounts, and even in the scenario you have described (.25e17), after a Million Rounding Errors, the marginal loss would be: 2.5e+17 which is .25 ETH

Additionally, every other depositor would get to benefit from the rounding error, and not just the first depositor

For this reason, I believe it's best to downgrade all findings to QA Low Severity as none have demonstrated an impactful loss

@ChaoticWalrus interested in your thoughts

While I would instinctively recommend a Slippage check, the math shows that the impact is heavily limited

[ChaoticWalrus]

@ChaoticWalrus interested in your thoughts

Thanks for the ping on this. I believe I'm in agreement with @d3e4 on this matter -- our team evaluated issue #193 to describe a valid potential griefing issue. Even if it requires very specific circumstances, our implementation was not ideal here. I appreciate your work on this and agree that the other issues seem invalid.

[d3e4]

0.25 ETH is not dust amounts, is it? This is what the attacker would profit. Normal rounding errors would be dust amounts (a few wei), but multiply the dust by a sufficiently large number (e.g. 1000e18/1e9) and the dust turns into pebbles. 0.25 ETH is admittedly not huge on a 1000 ETH investment, but it is a definite something, and it is not a hard upper limit.
Consider also that a 1e12 loss on a 0.01 ETH deposit is 0.1%, which is about a Uniswap fee. It is small, but evidently it is enough for Uniswap pools.

Additionally, every other depositor would get to benefit from the rounding error, and not just the first depositor

But the first depositor will still benefit from his attack. He doesn't care who loses and who gains, except that he gains.

[GalloDaSballo]

@ChaoticWalrus interested in your thoughts

Thanks for the ping on this. I believe I'm in agreement with @d3e4 on this matter -- our team evaluated issue #193 to describe a valid potential griefing issue. Even if it requires very specific circumstances, our implementation was not ideal here. I appreciate your work on this and agree that the other issues seem invalid.

Can you please clarify what makes you think that the other issues are invalid? Don't they all boiled down to the fact that the shares are rebased and that can cause a loss?

[ChaoticWalrus]

Can you please clarify what makes you think that the other issues are invalid? Don't they all boiled down to the fact that the shares are rebased and that can cause a loss?

My apologies, I think I was flippant in my use of the specific term 'invalid'. I believe you've demonstrated the impact is limited in practice and I have argued in specific issues that this should be categorized as known.

[0xJuancito]

Hi @GalloDaSballo! Could you check the comment I made on #207? It can be found here. I believe there was some misunderstanding regarding the judging of the report.

I hope this helps re-evaluate the finding. Thanks!

[GalloDaSballo]

Thank you for the feedback @0xJuancito, I have replied, please lmk

[0xJuancito]

Hi @GalloDaSballo! I added one last comment to clarify my intent on the self-evaluated impact and severity.

I appreciate your time and your attention to looking forward to clarification. Thank you!

[0xJuancito]

Clarification resolved and the judging was made with the provided information. Thanks for taking the time to go through it @GalloDaSballo!

[abarbatei]

Hey, @GalloDaSballo could you please take a look at #138. I think it was wrongly marked as duplicate of #132 because:
#132 is about how a malicious strategy can DoS withdrawals
#138 is how, in case there is a malicious/malfunction with withdrawals from Pods (and only Pods), the protocol doesn't have the granularity to pause only the withdrawal from Pods mechanism and has to pause all withdrawals to halt any issues. Although I could of made a better explanation I guess

[GalloDaSballo]

While I can see the argument for separating the findings, I also believe that paused and malicious are synonymous for "The strategy reverts"

Meaning that it seems appropriate to keep them as duplicates of each other, because the underling issue is the fact that the pending withdrawal must have all strategies work together.

Do you see it differently?

[abarbatei]

There is only the nuance that any issue related strictly to a pod withdraw (say a future exploit that makes it possible for people to over withdraw).
In that case the protocol has no choice but to pause all withdraws.

The pausing of _withdrawBeaconChainETH however, would not stop users for queuing up more withdrawals with the same malfunctioned pod, it would only stop those that have done it, or are doing it, to specifically be blocked, while allowing other users to withdraw from normal strategies.

Yes, it is synonymous with "The strategy reverts", with the added info that "we can somewhat limit to only one specific execution flow of strategy revert while allowing others to work". Thinking of it that way, it does sound like a more elaborate "blacklist mechanism implemented strictly for pod withdraw" though.

As a request, if I may, if concluding this as a duplicate, I would wish to withdraw it from the contest, since I am also the author of #132 it does not provide any value to have an extra duplicate, to me or others.

Thank you for the feedback Alex!

[GalloDaSballo]

Ultimately dedouping from same warden is handled automatically so we can keep both open, will think about your comment and finalize.

Thank you for the insights!

[t-nero]

Hi, @GalloDaSballo

Appreciate your judgment. Could you please take another look at #305? I believe it should not be considered a duplicate of #192. I have already left my comment there.

By the way, #192 shows the threat of address zero being frozen, but there is an analysis in #305 that shows that freezing address zero may be impossible with the Slasher codebase provided in this contest. The reason that the PoC in #192 passed is because it used a mockup Slasher contract that directly marked any given address as frozen without performing sanity checks as the actual Slasher codebase does. Therefore, I believe that #192 should not be considered valid in the first place.

Regards.

[GalloDaSballo]

Replied, please lmk

[GalloDaSballo]

If I did not react to your GH submissions please do link them here so I can check

[Emedudu]

Hello @GalloDaSballo,
Please check #374. I left a comment

[Emedudu]

@GalloDaSballo can you visit this please

[BhHaipls]

Hi, @GalloDaSballo

Thanks for judging

I have left comments in the following submissions:

• Ability to get shares while frozen. The absence of checking is frozen for the withdrawer #127
• EigenPod missed access checks #59
• Initialization of contracts is subject to Front-running attacks #46
• Missing msg.sender field in signature for deposit action #187

I hope they will be useful

Thank you and have a nice day

[GalloDaSballo]

Thank you for your comments, have replied

[Nabeel-javaid]

hey @GalloDaSballo commented on #427 #430

[GalloDaSballo]

Thank you for the comments, at this time, I disagree with them

I recommend you always add more information in your reports (esp #427) as well as a coded POC

For #430 I would be interested in any example of this being the root of an issue

[GalloDaSballo]

QA Changes:
Please give a grade for one Gas finding by warden dharma09: [103]
Please give a grade for one QA finding by warden d3e4: [453, 445] A after all (strategy stuff)
Please give a grade for one QA finding by warden Josiah: [432, 400] B
Please give a grade for one QA finding by warden jasonxiale: [338, 330] B
Please give a grade for one QA finding by warden tonisives: [325] C
Please give a grade for one QA finding by warden Ruhum: [322, 177, 172] A
Please give a grade for one QA finding by warden evmboi32: [318] C
Please give a grade for one QA finding by warden said: [306, 287] B

Multiple QA reports are marked as best
Please give a grade for one QA finding by warden Co0nan: [249, 65, 16] A
Please give a grade for one QA finding by warden santipu_: [208] C
Please give a grade for one QA finding by warden LokiThe5th: [193] C
Please give a grade for one QA finding by warden Haipls: [192, 127, 118, 106, 46] A
Please give a grade for one QA finding by warden rvierdiiev: [185, 183, 180, 178, 108] A
Please give a grade for one QA finding by warden MiloTruck: [123] C
Please give a grade for one QA finding by warden itsmeSTYJ: [110, 109] B
Please give a grade for one QA finding by warden Jigsaw: [89] C
Please give a grade for one QA finding by warden Dug: [73, 67, 66] A
Please give a grade for one QA finding by warden volodya: [63, 54, 39, 38, 37, 23, 22] A BEST

With the intro of bot races I have severely penalized Bot Looking Reports (findings OOS), and have awarded honestly written reports

2 Lows -> B
3 Lows -> A
Volodya sent an exceptional amount of high quality findings so it makes sense to me to award them the best