Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

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

Open
sherlock-admin3 opened this issue Jun 6, 2024 · 41 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Jun 6, 2024

elhaj

high

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

Summary

  • The Claimer contract's claimPrizes function can revert when the prize for the tier (numberOfTiers - 3) is 0. This prevents all winners in this draw from receiving their prizes (through claimer) and stops honest claimers from claiming rewards due to the transaction reverting.

Vulnerability Detail

  • The PrizePool contract holds the prizes and ensures that vault users who contributed to the PrizePool have the chance to win prizes proportional to their contribution and twab balance in every draw. On the other hand, the Claimer contract facilitates the claiming of prizes on behalf of winners so that winners automatically receive their prizes. An honest claimer should always have the ability to claim prizes for correct winners in every draw.
  • The ClaimPrizes function in the Claimer contract allows anyone to call it to claim prizes for winners. The caller (bots) computes the winners off-chain and they are incentivized through an auction mechanism where the reward increases over time if prizes are not claimed promptly.
        function claimPrizes(IClaimable _vault, uint8 _tier, address[] calldata _winners, uint32[][] calldata _prizeIndices, address _feeRecipient, uint256 _minFeePerClaim)
            external
            returns (uint256 totalFees)
        {
        //some code ...
    
            /**
             * If the claimer hasn't specified both a min fee and a fee recipient, we assume that they don't
             * expect a fee and save them some gas on the calculation.
             */
            if (!feeRecipientZeroAddress) {
    >>        feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, _countClaims(_winners, _prizeIndices), prizePool.claimCount()));
                if (feePerClaim < _minFeePerClaim) {
                    revert VrgdaClaimFeeBelowMin(_minFeePerClaim, feePerClaim);
                }
            }
    
            return feePerClaim * _claim(_vault, _tier, _winners, _prizeIndices, _feeRecipient, feePerClaim);
        }
  • Notice that the function calls the internal function _computeFeePerClaim() to compute the fee the claimer bot will receive based on the auction condition at this time. In _computeFeePerClaim(), another internal function _computeDecayConstant is called to compute the decay constant for the Variable Rate Gradual Dutch Auction:
        function _computeFeePerClaim(uint8 _tier, uint256 _claimCount, uint256 _claimedCount) internal view returns (uint256) {
        //some code .. 
    >>    SD59x18 decayConstant = _computeDecayConstant(targetFee, numberOfTiers);
        // more code ...
        }
        function _computeDecayConstant(uint256 _targetFee, uint8 _numberOfTiers) internal view returns (SD59x18) {
    >>    uint256 maximumFee = prizePool.getTierPrizeSize(_numberOfTiers - 3); 
    >>    return LinearVRGDALib.getDecayConstant(LinearVRGDALib.getMaximumPriceDeltaScale(_targetFee, maximumFee, timeToReachMaxFee));
        }
  • In _computeDecayConstant, the maximumFee is obtained as the prize of the tier(_numberOfTiers - 3), which is the last tier before canary tiers. This value is then fed into the LinearVRGDALib.getMaximumPriceDeltaScale() function.
  • The issue arises when the maximumFee is zero, which can happen if there is no liquidity in this tier. With maximumFee = 0, the internal call to LinearVRGDALib.getMaximumPriceDeltaScale() will revert specifically in wadLn function when input is 0, causing the transaction to revert:
        function getMaximumPriceDeltaScale(uint256 _minFee, uint256 _maxFee, uint256 _time) internal pure returns (UD2x18) {
            return ud2x18(SafeCast.toUint64(uint256(wadExp(wadDiv(
                wadLn(wadDiv(SafeCast.toInt256(_maxFee), SafeCast.toInt256(_minFee))),//@audit : this will be zero 
                SafeCast.toInt256(_time)) / 1e18))));
        }

        function wadLn(int256 x) pure returns (int256 r) {
        unchecked {
    >>    require(x > 0, "UNDEFINED");
            // ... more code ..
        }
        }
  • This means that even if a winner wins, the claimer won't be able to claim the prize for them if tier(_numberOfTiers - 3).prize = 0, unless they set fees to 0. However, no claimer will do this since they would be paying gas for another user's prize. This will lead to winners not receiving their prizes even when they have won, and it prevents the honest claimer from performing its expected job due to external conditions.

  • The winner in this case may win prizes from tiers less than _numberOfTiers - 3, which are rarer and higher prizes (they can't win prizes from tier (_numberOfTiers - 3) since it has 0 liquidity).

  • The tier(_numberOfTiers - 3).prize can be 0 in different scenarios (less liquidity that round to zero when devid by prize count,high tierLiquidityUtilizationRate ..ect). Here is a detailed example explain one of the scenarios that can lead to tier(_numberOfTiers - 3).prize= 0 :

  • Example Scenario

  1. Initial Setup:

    • Assume we have 4 tiers [0, 1, 2, 3].
    • Current draw is 5.
    • rewardPerToken = 2.
    1. Awarding Draw 5:
    • rewardPerToken becomes 3.
    • Prizes are claimed, and all canary prizes are claimed due to a lot of liquidity contribution in draw 5.
    • Liquidity for each tier after draw 5:
      Tier Reward Per Token (rpt)
      t0 0
      t1 0
      t2 3
      t3 3
    • Notice that the remaining liquidity in tiers 2 and 3 (canary tiers) is 0 now.
    1. Awarding Draw 6:
    • Time passes, and the next draw is 6.
    • Draw 6 is awarded, but there were no contributions.
    • Since the claim count was high in the previous draw, the number of tiers is incremented to 5.
    • Reclaim liquidity in both previous tiers 2 and 3, but there is no liquidity to reclaim.
    • There is no new liquidity to distribute, so the rewardPerToken remains 3.
    • Liquidity for each tier in draw 6:
      Tier Reward Per Token (rpt)
      t0 0
      t1 0
      t2 3
      t3 3
      t4 3
    1. Claiming Prizes:
    • A claimer computes the winners (consuming some resources) and calls claimPrizes with the winners.
    • The uint256 maximumFee = prizePool.getTierPrizeSize(_numberOfTiers - 3); (tier2) will be 0 since there was no liquidity .
    • This causes wadLn(0) to revert when trying to compute the claimer fees thus tx revert and claimer can't claim any prize.

Impact

  • claimers cannot claim prizes for legitimate winners due to the transaction reverting.
  • all Users who won the prizes in this awarded draw won't receive their prizes (unless they claim for them selfs with 0 fees which unlikely).

Code Snippet

Tool used

Manual Review , Foundry Testing

Recommendation

  • handle the situation where getTierPrizeSize(numberOfTiers - 3) prize is 0, to make sure that the prize for winner still can be claimed by claimers.
@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jun 11, 2024
@nevillehuang nevillehuang added Medium A valid Medium severity issue and removed High A valid High severity issue labels Jun 17, 2024
@sherlock-admin3 sherlock-admin3 changed the title Bubbly Chocolate Wombat - Claimers Cannot Claim Prizes When Last Tier Liquidity is 0, Preventing Winners from Receiving Their Prizes elhaj - Claimers Cannot Claim Prizes When Last Tier Liquidity is 0, Preventing Winners from Receiving Their Prizes Jun 18, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Jun 18, 2024
@elhajin
Copy link
Collaborator

elhajin commented Jun 20, 2024

  • 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.

  • given the lost of prize for winners , and Dos of claiming process , this should be high severity

@sherlock-admin3
Copy link
Author

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
Copy link

0xjuaan commented Jun 20, 2024

Escalate

On behalf of @elhajin

@sherlock-admin3
Copy link
Author

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Jun 20, 2024
@WangSecurity
Copy link

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
Copy link
Collaborator

elhajin commented Jun 24, 2024

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

  • one of the main features of pooltogather is winners will receive prizes automatically through incentivized claimers and average users are not expected to check each draw if they win or not and claim their prizes ..
  • so Dos on claiming will lead to winners not receiving their prizes except for those who claim for themselves which is not the protocol intent

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 24, 2024

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
Copy link
Collaborator

elhajin commented Jun 24, 2024

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

  • so in this case winners of tiers that are less then (numberOfTiers - 3) in this draw will lose their prizes due to lack of incentive for claimers.

  • this is still an issue. Claimers should always be incentivized to claim prizes for winners in each draw regardless of last tier liquidity Since the protocol intent is winners get their prizes automatically.

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

  • the decrease of tiers in the next draw is irrelevant to this issue.. cause winners of previous draw already lost their prizes

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

  • this is not fine winning a prize for a user is the main idea of the protocol .. and if he lost his prize it's high likely he won't win it again in the new awarded draws

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 24, 2024

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
Copy link
Collaborator

elhajin commented Jun 24, 2024

I understand your POV.

  • just to add to my previous comments other tiers (less then numberOfTiers -3) rely on contribution from several draws (up to contribution of a user in last 365 draw for grand prize) to determine the prize size and winner .and my POV is Not incentive claimer to claim those prizes because one of those draws has small contribution is an issue.

To summarize that for the judge:

  • there is no incentives for claimers to claim prizes for winners when last tier liquidity equals 0
  • this will lead to users who rely on the fact that prizes are distributed automatically lose their prizes

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 24, 2024

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
Copy link
Collaborator

elhajin commented Jun 24, 2024

Will never happen???

  • 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

  • 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

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

  • I'd love the sponsor to leave his comment here

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 25, 2024

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
Copy link

WangSecurity commented Jun 26, 2024

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
Copy link
Collaborator

elhajin commented Jun 26, 2024

  • Odds for canary tiers are 100%, and the claimer is getting the whole prize of those tiers. This means it matters if the prize for canary tiers is more than the gas the claimer will spend to claim it. If so, the claimer will claim canary tiers, and it's highly likely that they will find a lot of winners for these tiers (even exceeding the liquidity those canary tiers have when claiming for them all) given the high odds.
  • When can this happen?
    • This can happen when we have a large enough contribution in the previous draw since liquidity for canary tiers is redistributed each draw.
  • What happens when claimers claim all liquidity in canary tiers?
    • The next draw will increase the number of tiers, and the new tier (newNumberOfTiers - 3) will get its liquidity from the contribution of this draw. So if there were no or small contributions in this draw, this tier will get 0 liquidity.
  • Why is it likely to get no contribution in the next draw while we got large enough liquidity in the previous draw?
    • As mentioned in my comment above, vaults don't accumulate yield daily, and it's highly likely if they contributed yesterday, they will have no yield to contribute today.

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
Copy link
Collaborator

0x73696d616f commented Jun 26, 2024

@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
Copy link
Collaborator

elhajin commented Jun 26, 2024

  • Again, I still think it's highly likely to get no contribution in a day.
  • For example, if canary tiers are t2 and t3, each address (user) has chance to win up to : 4^2+ 4^3 = 80 prizes and we have 80*2(UR) = 160 prizes. I'm curious how you calculated the odds for claiming canary tiers given that we don't know how many users contributed to vaults and how many vaults contributed to the PrizePool in this draw?

As the utilization ratio is 50%, the odds of claiming all canary tiers twice are close to 0.0625.

@0x73696d616f
Copy link
Collaborator

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
Copy link
Collaborator

elhajin commented Jun 26, 2024

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

  • that's not true and you can check on-chain the deployed version of the protocol

  • your calculation shows the probability of 6.25%(0.0625) that all canary tiers will be claimed .. which is for me high enough

  • moreover I spoke about that with sponsor during the contest and he confirmed
    Screenshot_2024-06-26-13-11-14-00_572064f74bd5f9fa804b05334aa4f912.jpg

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 26, 2024

@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
Copy link
Collaborator

elhajin commented Jun 26, 2024

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

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 26, 2024

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
Copy link
Collaborator

0x73696d616f commented Jun 26, 2024

@WangSecurity
Copy link

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
Copy link
Collaborator

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

@elhajin
Copy link
Collaborator

elhajin commented Jun 27, 2024

@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
Copy link
Collaborator

0x73696d616f commented Jun 27, 2024

@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
Copy link
Collaborator

elhajin commented Jun 27, 2024

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

  • 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.

  • 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(anyone can deploy a vault).

  • 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 V5 is still new).

@0x73696d616f
Copy link
Collaborator

0x73696d616f commented Jun 27, 2024

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Collaborator

elhajin commented Jun 28, 2024

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

@WangSecurity
Copy link

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
Copy link

There are no dupes of this issue its unique finding.

@WangSecurity WangSecurity removed the Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label label Jun 30, 2024
@WangSecurity
Copy link

WangSecurity commented Jun 30, 2024

Result:
Medium
Unique

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Jun 30, 2024

Escalations have been resolved successfully!

Escalation status:

@WangSecurity WangSecurity reopened this Jun 30, 2024
@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label Jun 30, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label Jun 30, 2024
@sherlock-admin3 sherlock-admin3 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Jul 2, 2024
@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Jul 3, 2024

The protocol team fixed this issue in the following PRs/commits:
GenerationSoftware/pt-v5-claimer#34

@10xhash
Copy link
Collaborator

10xhash commented Jul 11, 2024

Fixed in GenerationSoftware/pt-v5-claimer#33 and GenerationSoftware/pt-v5-claimer#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
Copy link
Contributor

The Lead Senior Watson signed off on the fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

10 participants