sherlock-audit / 2024-05-pooltogether-judging

2 stars 0 forks source link

elhaj - Claimers Cannot Claim Prizes When Last Tier Liquidity is 0, Preventing Winners from Receiving Their Prizes #84

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

elhaj

high

Claimers Cannot Claim Prizes When Last Tier Liquidity is 0, Preventing Winners from Receiving Their Prizes

Summary

elhajin commented 3 weeks ago
sherlock-admin3 commented 3 weeks ago

Escalate @nevillehuang This issue is distinct from #112 . In #112 , the issue is that having more winners in a specific tier than the prize count for that tier would lead to insufficient liquidity for the excess winners in case the reserve isn't enough to cover that . However, this is not true, as the protocol addresses this using the tierUtilizationRatio to manage such situations, indicating that this is a known limitation. Otherwise, there would be no point in having the tierUtilizationRatio variable .

  • This issue is completely different. It addresses the scenario where the lastTier before the canary tiers has 0 liquidity. In this situation, the claimer won't be able to claim any prize for all winners in this draw. As a result, winners in this draw won't receive their prizes.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

0xjuaan commented 3 weeks ago

Escalate

On behalf of @elhajin

sherlock-admin3 commented 3 weeks ago

Escalate

On behalf of @elhajin

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

WangSecurity commented 3 weeks ago

To clarify, the users indeed can receive rewards, the problem is that they have to do that by themselves and without claimers? But claimers are DOSed?

elhajin commented 3 weeks ago

Yes .. and the argument here is this is not the expected behaviour

0x73696d616f commented 3 weeks ago

This is a design choice. They cap the fee to the the prize pool of the last non canary tier. If this is 0, then fine, new draws have to be awarded so there is enough fee (if users still want to claim, they can call it). If prizes are not claimed, the number of tiers will decrease, which will increase the fee for the next draw, as the number of tiers will be less, so each tier has more liquidity and the number of prizes for the fee tier (number - 3) decreases, so the fee increases. So basically the transaction reverting or not does not matter because the max fee is 0, so if they want it to go through, they need to set the fee to 0.

Think about it this way, if there are no contributions, or small contributions in the last draw (which is the requirement for this), the fee should also be 0 or small.

elhajin commented 3 weeks ago

Good point .. but i disagree that this is a designe choice

Here a quote from the docs :

Prizes expire every Draw. For daily prizes, that means they expire every 24 hours. Incentivizing claimers is critical to ensure that users get their prizes

If this is 0, then fine, new draws have to be awarded so there is enough fee

0x73696d616f commented 3 weeks ago

Claimers can not be incentivized if there was no liquidity provided, which was the design choice made by picking the last non canary tier prize size. The docs still match the code because the incentive is a % of something, if that something is 0, than the incentive is also 0.

What I meant by it's fine, is that users can claim prizes themselves, sure the protocol prefers that bots claim it, but this does not mean users can not claim them, even more so when there was no liquidity contributed. Bots will have incentives in the next draw.

Also, the fact that the transaction reverts when the prize is 0 has nothing to do with this, the issue itself is in how the protocol calculates the max fee, which is based on the liquidity contributed in the previous draws. When the max fee is 0, bots should claim with feeRecipientZeroAddress set to 0 to save gas, if they wish to do.

We can see that it is a design choice because the fix would be a different design of the fee, such as adding a fixed component, which is not a valid issue.

elhajin commented 3 weeks ago

I understand your POV.

To summarize that for the judge:

0x73696d616f commented 3 weeks ago

It is a design choice and another proof is that the issue will never happen due to 1) utilization ratio, 2) requirement of claiming all prizes in the past draw in the canary tiers (and in some cases the last non canary tier, tier 4 to tier 4 and tier 5 to tier 4), 3) not having contributions for a whole draw 4) the shares of the last non canary tier are much bigger than the canary ties, so whatever liquidity is left is mostly sent to it.

Starting state, draw 5 4 tiers, canary tiers 1 and 2 are claimed 50% due to utilization ratio

Final state, draw 6 Example1, tiers move from 4 to 5 even if 0 contributions in this draw, the previous canary tiers were only claimed 50%, so there is liquidity to distribute to tier 2 (the one that leads to the max fee), and most of it is sent there.

Example2, tiers stay at 4 even if 0 contributions, the last canary tiers will be distributed, and tier 1 (the max fee), even if the expected prizes were claimed for tier 1, still remains 50% liquidity + most of the amount from the canaries due to having more shares.

Starting state, draw 5 5 tiers, tier2, canary tiers 1 and 2 are claimed 50% due to utilization ratio

Final state, draw 6 tiers move down to 4 tier2 and the canaries get redistributed, they were only claimed 50% due to the utilization ratio, so tier 1 (the max fee) will always have liquidity, even if no contributions occur and it receives most of the liquidity.

So utilization ratio + requirement to claim all prizes + not having contributions in the whole draw + last non canary tier having much more shares than canary tiers make this issue impossible to happen

elhajin commented 3 weeks ago

Will never happen???

0x73696d616f commented 3 weeks ago

I think you mixed things up; utilization ratio doesn't prevent the claiming of all the liquidity of a tier since we can have more winners than the prize count of a tier

It decreases the likelihood a lot as it needs to outperform the expected claim counts by a big margin (double) on both canary tiers and the last non canary tier in some cases (tier 4 to tier 4 and tier 5 to tier 4). Assume 2 users, each 0.5 contribution to prize, probably of claiming both canaries twice is 0.5*0.5*0.5*0.5 = 0.0625 (simplifying 1 prize count per canary tier, doesnt matter having more in terms of expected probabilities, they always have to overperform by twice the amount).

utilization ratio and last tiers before canary have more prizes can also be a helping factors to this issue to be presented due to high liquidity fragmentation over all prizes then scaling down by utilisation ratio that rounds the prize to zero

This doesnt matter, liquidity ratio just decreases 50% here, and the prize count does not get big enough to send to 0, even if we are in tier 11 (highly unlikely), 4^8 == 65536. The prize token is WETH, which has 18 decimals, 65536 / 1e18 / 0.5 == 1.31072e-13 WETH, which is a dust contribution, enough for this to not happen.

not having contribution in a draw is high likely as vaults doesn't accumulate yeild daily to contribute it

It would require all vaults to not contribute in a day, which is highly unlikely.

So if we multiply all these chances, as they all need to happen simultaneously (and the fact that the last non canary tier has more shares, so it will absorve most liquidity), this will never happen. And it is a design choice regardless, but wanted to get this straight.

WangSecurity commented 2 weeks ago

Firstly, I can confirm that it's not a design decision.

Secondly, I agree that it's not a duplicate of #112.

Thirdly, based on the discussion above, this scenario indeed has significant requirements and constraints (based on this and this comments), moreover, it only DOSes the claimers, but the prizes can still be received. I understand that the goal is to claimers to claim prizes, still the prizes are not locked completely and can be claimed.

Planning to accept the escalation and de-dup the report.

elhajin commented 2 weeks ago

It would require all vaults to not contribute in a day, which is highly unlikely.

It's highly likely. We're talking about two vaults in production at the current time (even with 10 vaults, it's highly likely this will happen). Whether a design choice or not, let's let the judge decide.

0x73696d616f commented 2 weeks ago

@elhajin as the utilization ratio is 50%, the odds of claiming all canary tiers twice are close to 0.0625. 2 vaults is a really small number, it is expected to be more. Each vault has its own token, and more than 1 vault can exist for each token. So this number is likely to grow a lot. And keep in mind that these 2 events must happen at the same time, so the chance becomes really really small.

If we think there is a 1% chance no contributions happen in a day (this number seems too big, I think the chance is even smaller, in normal operating conditions, as bots are incentivized to liquidate daily, if they don't, this is irregular), the chance would be 0.0625*0.01 = 0.0625%, very small.

And even if this happens (highly unlikely), users can still claim prizes for themselves, or even the protocol may choose to sponsor it.

elhajin commented 2 weeks ago
0x73696d616f commented 2 weeks ago

Again, I still think it's highly likely to get no contribution in a day.

It's not the expected, regular behaviour. The protocol incentivizes liquidating daily, so anything other than this is unlikely. In this case, there are multiple vaults, even more unlikely. It would require having unexpected behaviour (not liquidating daily) for all vaults, which is very unlikely.

About the odds, here is the calculation. Assuming 1 prize for the canary tiers, we need at least 2 users to get 4 prizes (if there are 2 users, they at most get 2 prizes, so this issue does not exist). If the users have each 50% of the vault contribution, than their chance to win is 50%, as canary tiers have 100% odds. So as each user has a 50% chance of winning, and we would need them to win both prizes each, which is 0.5*0.5*0.5*0.5. The calculations just get more complicated math wise with more prizes, key is they have to overperform the system, which is very unlikely. Using a claim count of 1 gives a good idea of the likelihood.

elhajin commented 2 weeks ago

It's not the expected, regular behaviour. The protocol incentivizes liquidating daily, so anything other than this is unlikely. In this case, there are multiple vaults, even more unlikely. It would require having unexpected behaviour (not liquidating daily) for all vaults, which is very unlikely

0x73696d616f commented 2 weeks ago

@elhajin, the sponsor said it's possible to claim all prizes, which I agree with, but very unlikely, given the utilization ratio. But the fact that the vaults have to not contribute in the next day, further reduces the likelihood. The 2 chances multiplied are very low for this to be high, estimated at 0.0625% or less.

elhajin commented 2 weeks ago

I think we made our points clear .. let's let the judge decide that ser🤝

0x73696d616f commented 2 weeks ago

And the expected behaviour part is true. Ok they only have 2 deployed vaults right now. But still the expected behaviour is them contributing daily. So the chances go from very low to non existent, considering winning all prizes by double the amount (0.0625) and not contributing in a single day. The actual chance is closer to 0.0625% or lower due to the 2 conditions, not just 6.25% as you mentioned.

0x73696d616f commented 2 weeks ago

Also, they have a lot of vaults deployed, 19, 12 and 11, respectively. https://optimistic.etherscan.io/address/0x0C379e9b71ba7079084aDa0D1c1Aeb85d24dFD39 https://arbiscan.io/address/0x44be003e55e7ce8a2e0ecc3266f8a9a9de2c07bc https://basescan.org/address/0xe32f6344875494ca3643198d87524519dc396ddf

WangSecurity commented 2 weeks ago

To clarify, the chances of this happening is ~0.0625% and not 6.25%, so when @0x73696d616f wrote just 0.0625 it meant 0.0625%, correct?

0x73696d616f commented 2 weeks ago

Yes 0.0625%, after seeing that there are so many deployed vaults, it's even lower than this.

elhajin commented 2 weeks ago

@WangSecurity how so 😐? it's 6%.. 50*50*50*50/(100*100*100*100) = 625/100 which is 6.25% !! It's basic math

0x73696d616f commented 2 weeks ago

@elhajin you are not multiplying by the chance of not liquidating in a whole day, which is very low as explained before. There are at least 12 vaults, so all vaults would have to behave unexpectedly and not liquidate in a day for this to happen. I attributed a chance of 1% to this scenario, which is very reasonable, in reality it will be lower.

@WangSecurity to be clear, for this issue to happen all the canary prizes have to be claimed, which is a 6.25% chance AND no contributions from all vaults can happen in a whole day, so we need to multiply both probabilities. The protocol expects vaults to liquidate daily and has a tpda mechanism in place to ensure this, it would be extremely unlikely for all vaults to not liquidate in a single day. Thus, we can use an upper bound chance of 1% for this event, but we can see how it is extremely unlikely. So 0.0625*0.01 = 0.000625 = 0.0625%

elhajin commented 2 weeks ago
0x73696d616f commented 2 weeks ago

First, we are not talking about that; we are talking about the probability that Canary tiers will be claimed, which is indeed 6.25%.

This is false, we need to multiply by the chance of all vaults not contributing in a day, @WangSecurity if u need any further explanation let me know but this is factual.

Second, you're stating some of your thoughts as facts (at least 12 vaults, contributing daily is the expected behavior , 1% no contribution), which I believe are not true.

The chance of all vaults behaving in a way they are not incentivized to is extremely unlikely. We can use 1%, but it is likely lower than this. If we say each vault has 10% chance of not contributing in a day, the chance would be 0.1^12=1e-12, with 12 vaults. With 2 vaults, it's 0.1^2 = 0.01 = 1%. So the chance goes from very low to impossible. Also, if we have less vaults, the likelihood of not contributing in a day will decrease for each because there is way more liquidity to liquidate, so the incentive is huge.

Third, we don't know which vaults will be active (depending on users adopting them) and which will not. So, even with 100 vaults deployed, it's irrelevant if they are not active.

With regular user operation it can be expected a decent number of vaults will be active, anything other than this is extremely unlikely and it's not what the protocol expects.

When I was talking about on-chain activity, I meant how many vaults are contributing, not how many are deployed. And as you can see here, we have only 1 contribution in 69 days (I understand that the protocol is still new).

We need to talk about regular conditions, not some bootstrap phase. If you want to consider weird activity, than we need to consider the prize may be very small, this phase has a low likelihood of happening, bots may choose not claim all prizes due to low liquidity, so on and so forth. In phases like this bots may not even be setup so this issue will not happen anyway.

This issue is an extremely edge case which will never happen, even medium is questionable. The stars would have to align for this to happen under normal operations.

Hash01011122 commented 2 weeks ago

The chance of all vaults behaving in a way they are not incentivized to is extremely unlikely. We can use 1%, but it is likely lower than this. If we say each vault has 10% chance of not contributing in a day, the chance would be 0.1^12=1e-12, with 12 vaults. With 2 vaults, it's 0.1^2 = 0.01 = 1%. So the chance goes from very low to impossible. Also, if we have less vaults, the likelihood of not contributing in a day will decrease for each because there is way more liquidity to liquidate, so the incentive is huge.

H/M severity don't take likelihood, only impact and constraints. REFERENCE If invalidation reason for this is likelihood then issue #88 should be invalid too

WangSecurity commented 2 weeks ago

Firstly, I believe the constraints for this scenario to happen are extremely high (all the canary prizes have to be claimed, which is a 6.25% chance AND no contributions from all vaults can happen in a whole day). Secondly, the prizes are not lost per se, it's just the claimers who are DOSed. I understand that's an important part of the protocol, but still the funds are not locked, and winners can claim the prizes themselves.

I believe based on these two points together, low severity is indeed more appropriate. The decision remains the same, planning to accept the escalation and de-dup this issue as invalid.

Hash01011122 commented 2 weeks ago

Wrapping around my head on how this is not breakage of core functionality.

the prizes are not lost per se, it's just the claimers who are DOSed. I understand that's an important part of the protocol, but still the funds are not locked, and winners can claim the prizes themselves.

WangSecurity commented 2 weeks ago

I agree this breaks the core contract functionality, but it has to have impact besides just breaking the core contract functionality:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds

As I've said there are no funds lost, since the users can still claim the prizes. But, on the other hand, the idea that just came to my mind is that, claimers have their own contract Claimer and in the case of this issue, the entire contract is useless, correct?

elhajin commented 2 weeks ago

@WangSecurity That's the idea . And claimer contract also in scope.

WangSecurity commented 2 weeks ago

With that, I still believe there is no loss of funds per se, because the prizes still can be claimed by the winners themselves. But, the core contract Claimer would useless in that case, since bots' attempts to claim prizes will revert. Hence, I believe it qualifies for the "Breaks core contract functionality, rendering the contract useless". If it's wrong then please correct me.

Planning to accept the escalation and make a new family of medium severity. Are there any duplicates?

Hash01011122 commented 2 weeks ago

There are no dupes of this issue its unique finding.

WangSecurity commented 2 weeks ago

Result: Medium Unique

sherlock-admin2 commented 2 weeks ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin2 commented 1 week ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-claimer/pull/34

10xhash commented 4 days ago

Fixed in https://github.com/GenerationSoftware/pt-v5-claimer/pull/33 and https://github.com/GenerationSoftware/pt-v5-claimer/pull/34 Now the VRGDA is run with 100% maxFee as the auction's high price. In case the targetFee is 0, it is set to 1% of maxFee

sherlock-admin2 commented 4 days ago

The Lead Senior Watson signed off on the fix.