sherlock-audit / 2023-07-kyber-swap-judging

12 stars 8 forks source link

0x52 - SnipAttack safeguards distribute snipped fees in a way that can still be abused #88

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

0x52

medium

SnipAttack safeguards distribute snipped fees in a way that can still be abused

Summary

When a snip attack is blocked, the rTokens minted to the attacker are burned. This effectively distributes the captured fees to all rToken holders. The problem with this is that the fees should have been distributed to liquidity that is in range. With the right preparation this can actually make it more profitable for the attacker instead of less. If the attacker is able to accrue a large number of rTokens they can still effectively steal fees from in range liquidity.

Vulnerability Detail

Pool.sol#L260-L271

function burnRTokens(uint256 _qty, bool isLogicalBurn)
  external
  override
  lock
  returns (uint256 qty0, uint256 qty1)
{
  if (isLogicalBurn) {
    _burn(msg.sender, _qty);

    emit BurnRTokens(msg.sender, _qty, 0, 0);
    return (0, 0);
}

When rTokens are burned as a result of snip attack safeguards being triggered, the burned rTokens do not qualify for any fees. This maintains the same amount of "liquidity" while decreasing the number of eligible tokens. The result of this is that all burned fees are effectively distributed to all rToken holders. The issue is that the fees should actually belong to in range liquidity. This allows malicious users to still exploit snip attacks in a roundabout manner.

Assume an attacker is able collect a large number of rTokens. This can be accomplished by depositing single tick liquidity then swapping repeatedly in it. Assume they own 75% of the total supply. Now they can profit large amounts from snipping attacks. When the rTokens generated during the attack are burned, they still receive 75% of the profits because they own 75% of the rTokens. Due to this effective monopoly the attacker has they would likely end up netting more money overall. Currently on UniswapV3 these types of attacks are highly competitive which means a large amount of the profits wind up with the validators rather than the attacker. In this scenario the attacker would be the only person capable of conducting this attack (since they own the rTokens) allowing them to keep more the profit and not have to share as much with the validator.

Impact

Snipping safeguards can be effectively bypassed

Code Snippet

AntiSnipAttackPositionManager.sol#L94-L143

Tool used

Manual Review

Recommendation

Distribute fees proportionally to all in range liquidity. To prevent manipulation of in range liquidity, considering using the built in TWAP to determine which liquidity is eligible

sherlock-admin commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, burning tokens due to snip attack uses isLogicalBurn == true. It means it just burns rTokens without accumulating fees (code snippet). Therefore, there is no profit from this burning.

jkoppel commented:

Reinvestment liquidity is active for the entire range of the pool. This is working as intended.

IAm0x52 commented 1 year ago

Escalate

I believe this has been incorrectly excluded.

invalid, burning tokens due to snip attack uses isLogicalBurn == true. It means it just burns rTokens without accumulating fees (code snippet). Therefore, there is no profit from this burning.

Judges comment here is incorrect. Fees still accumulate but the fees are redirected to the rTokens through appreciation. The current implementation doesn't fairly distribute the fees to the correct parties which is the root cause of this issue and why it can be abused. The attacker wouldn't be able to capture 100% of fees but they can still capture enough to make it profitable. I still believe the best solution would be to redistribute to all in range liquidity rather than just rTokens.

sherlock-admin2 commented 1 year ago

Escalate

I believe this has been incorrectly excluded.

invalid, burning tokens due to snip attack uses isLogicalBurn == true. It means it just burns rTokens without accumulating fees (code snippet). Therefore, there is no profit from this burning.

Judges comment here is incorrect. Fees still accumulate but the fees are redirected to the rTokens through appreciation. The current implementation doesn't fairly distribute the fees to the correct parties which is the root cause of this issue and why it can be abused. The attacker wouldn't be able to capture 100% of fees but they can still capture enough to make it profitable. I still believe the best solution would be to redistribute to all in range liquidity rather than just rTokens.

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.

Trumpero commented 1 year ago

@IAm0x52 After reviewing your issues again, I understand your mentions about the distribution of fees during a snip-attack. However, I don't agree that an attacker can profit from snip-attacks even when they hold the majority of the rTokens supply.

This is because their burnable rTokens from the snip-attack will be burned with isLogicalBurn set to true. Therefore, the burnRtokens function will not distribute any liquidity fees for this amount of rTokens. https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L266-L271 As a result, their shares of rTokens will remain unchanged, while the totalSupply of rTokens will increase due to government fees. Consequently, the attacker will receive fewer fee rewards compared to the scenario where they didn't execute a snip-attack. https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L279-L290

Consider the following scenario: The total supply of rTokens is 100 at the current moment: Alice holds 75 rTokens (75%), Bob holds 5 rTokens, and the government holds 20 rTokens as the gov fee. The liquidity value of fee (reinvestL) in the pool is 100 at this time. After that, a swap is executed, increasing the reinvestL (lpFee) to 100, and minting 80 rTokens for the pool contract (which will be distributed as 75 rTokens for Alice and 5 rTokens for Bob when their positions are adjusted), along with 20 rTokens for the government as gov fee. So, if Alice didn't execute a snip-attack, she would receive pool tokens from the 150 lpFee (75% of 200) by burning her rTokens. In the case where Alice did execute a snip-attack, increasing her liquidity in the pool before the swap, almost 80 rTokens from the pool contract during that swap would be distributed to Alice. However, this amount of rTokens would be burned by the logical burn when Alice exits liquidity of the attack immediately after the swap, while reinvestL is still at 200. The supply of rTokens would then be 120 (the old supply plus the government fee), and Alice would still have 75 rTokens. So, when Alice burns her rTokens, she would receive a distribution of 75/120 = 62.5% of reinvestL lpFee, which is less than the case where she didn't execute this attack (62.5% * 200 = 125 < 150).

IAm0x52 commented 1 year ago

(which will be distributed as 75 rTokens for Alice and 5 rTokens for Bob when their positions are adjusted)

This is not true. In the event that the snipping attack doesn't happen, the rTokens will be minted to in range liquidity not to the current rToken holders. In this scenario, Alice wouldn't profit (or only profit slightly) from the large trade but if they do attack then they will gain a much larger percentage. If we assume that rToken liquidity only accounts for 10% of in range liquidity then Alice would profit 10% of the fees without the attack. As calculated in the comment directly above, snip attacking the pool would result in 62.5% profit.

This is the fundamental issue with how the anti-snip profits are distributed. Normally fees are distributed to in-range liquidity but when an attack happens, fees are instead distributed to rToken holders. My submission shows how this difference in distribution methodology can be abused.

Trumpero commented 1 year ago

@IAm0x52 The scenario mentioned above considers that the liquidity of Alice and Bob is within the same liquidity range because they will not receive lpFee distribution if their liquidity is not within the range of the mentioned swap. This example focuses on the calculation of fee distribution, so it need to assume they have the same liquidity range.

Additionally, the result of 62.5% of lpFee mentioned in the previous comment is not a profit from the snip-attack. It represents the lpFee that would be distributed to Alice by burning her rTokens in the case that Alice executed a snip-attack. However, if Alice did not execute snip-attack, she would receive 75% of the lpFee as rewards, so 62.5% is a loss for Alice.

IAm0x52 commented 1 year ago

rTokens are independent of in range liquidity and users can own large amounts of rToken even if their liquidity is not in range.

Also it's not a loss. They own 75% of 100 (75) vs 62.5% of 200 (125) so they have still profited from the attack. Another thing to note is that your calculation assume a 20% governance fee. If there is no fee then Alice has 75% both before and after the attack. Meaning their percentage doesn't decease at all and they fully profit.

Trumpero commented 1 year ago

rTokens are independent of in range liquidity and users can own large amounts of rToken even if their liquidity is not in range.

Also it's not a loss. They own 75% of 100 (75) vs 62.5% of 200 (125) so they have still profited from the attack. Another thing to note is that your calculation assume a 20% governance fee. If there is no fee then Alice has 75% both before and after the attack. Meaning their percentage doesn't decease at all and they fully profit.

Sorry, I made a typographical error in my example. In my scenario, the swap provides 100 lpFee to reinvestL, so the total reinvestL after the swap is 200. That's what I meant. When Alice burns all her rTokens, she will receive 75% of 200 (150) and 62.5% of 200 (125) lpFee in those two cases. Even when the swap provides 200 lpFee, the lpFee that Alice receives in the case where she didn't execute the snip-attack will be 75% of 100 + 75% of 200 = 225, which is more than the case where she executed the attack. And if there is no government fee, then Alice will obtain the same lpFee in both cases.

manhlx3006 commented 1 year ago

Hi, this has been considered when we designed the logic of burning rTokens from snipping attackers.

There were few solutions were proposed to discuss on how to make it fairly: (1) Send that rTokens to DAO/protocol fee recipient. (2) Distribute the rTokens to the active LPs (not include out of range positions). (3) Burn rTokens, benefit all rTokens holders. (4) Some others.

(1) It is not good, as it benefits the DAO, but not the LPs. (2) It is extremely difficult to distribute correctly and fairly to LPs/positions. Take not that the swap and removeLiquidity (then got burnt some rTokens) are 2 separated actions with completely different pool states. Thus, distributing rTokens to active LPs (active positions) is also not accurate.

(3) Burn rTokens, benefit all rTokens holders: This comes as the most easy + gas efficient solution, even though it is not considered as the most fair to LPs (but all solutions are unfair one way or another). (4) Same as (1) or (2), either difficult, costly, or still unfair.

Thus, in our assessments, burning rtokens to benefit all LPs in the pool is still the most feasible solution considering all factors.

IAm0x52 commented 1 year ago

I acknowledge the points above but I have still demonstrated a valid exploit in the design of the snip attack prevention which can lead to abuse. The protocol may not agree that it needs to be fixed but I still believe this qualifies for a medium severity issue in the context of the contest. It was not stated as a known issue and I have shown how abuse can lead to loss of rewards for other LPs

manhlx3006 commented 1 year ago

Hi,

First, as the documentation has stated clearly: "In the case of an early withdrawal, the remaining rewarded fee tokens are burned, which benefits other liquidity providers based on their contribution, also encourages LPs to keep rTokens.", the rTokens from JIT are burnt to benefit all LPs based on their contribution as of holding rTokens, where in this case the attacker is also a liquidity provider with a huge contribution to the pool (by holding a majority of rTokens, it means the attacker has been contributing and earning lots of fees in the pool), which is still in the intention of the JIT mechanism.

Second, the JIT is benefiting LPs compares to non-JIT model, even though the benefit is not fairly distributed with the current pool states, it still helps to increase in value. So in this case, even though the attacker holds a majority of rTokens, if the attacker performs the snipping attack, the JIT mechanism still helps:

IAm0x52 commented 1 year ago

I think it's clear to see I've raised a valid scenario and I see no disagreement that the scenario I've describe does indeed exist. Seems to me the main point of disagreement is the severity based largely on the intention when designing these protections.

I would like to point back to this comment from my original submission as well because I believe that in certain scenarios having this protection leads to larger realized profits for the attacker rather than smaller:

Currently on UniswapV3 these types of attacks are highly competitive which means a large amount of the profits wind up with the validators rather than the attacker. In this scenario the attacker would be the only person capable of conducting this attack (since they own the rTokens) allowing them to keep more the profit and not have to share as much with the validator

Unless there are further concerns from the dev team, I think the only thing left here is for Sherlock to make the final call on severity.

hrishibhat commented 1 year ago

Result: Low Unique Considering the impact of this issue a low based on the above comments and further discussion. In addition to the likelihood of the issue, this seems to be a design choice/limitation with it being considered an acceptable risk by the protocol.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: