Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

position can be opened without premium #38

Closed
c4-bot-5 opened this issue Dec 21, 2023 · 13 comments
Closed

position can be opened without premium #38

c4-bot-5 opened this issue Dec 21, 2023 · 13 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-27 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-5
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L240-L243

Vulnerability details

Description

Premium in ParticlePositionManager is used to cover trading fees accrued for the liquidity borrowed. When liquidating, a portion of the premium is also used for the liquidation reward.

The issue is that a borrower can open a position without any premium at all:

ParticlePositionManager::openPosition:

File: contracts/protocol/ParticlePositionManager.sol

240:            if (
241:                cache.token0PremiumPortion < params.tokenToPremiumPortionMin ||
242:                cache.token1PremiumPortion < params.tokenFromPremiumPortionMin
243:            ) revert Errors.InsufficientPremium();

params.tokenFrom/ToPremiumPortionMin are supplied by the borrower hence can be 0.

This removes any liquidation reward, since closeCache.tokenFrom/ToPremium is 0:
ParticlePositionManager::liquidatePosition:

File: contracts/protocol/ParticlePositionManager.sol

349:        liquidateCache.liquidationRewardFrom =
350:            ((closeCache.tokenFromPremium) * LIQUIDATION_REWARD_FACTOR) /
351:            uint128(Base.BASIS_POINT);
352:        liquidateCache.liquidationRewardTo =
353:            ((closeCache.tokenToPremium) * LIQUIDATION_REWARD_FACTOR) /
354:            uint128(Base.BASIS_POINT);

Thus any liquidation incentives are removed. Even if the position would immediately be liquidatable there would be no incentive to liquidate it. Other than possibly for the LP as they would get their liquidity back.

0 premium also robs the liquidity provider of any fees that would have been accrued for their borrowed liquidity:
ParticlePositionManager::_closePosition (same for the other branch of zeroForOne):

File: contracts/protocol/ParticlePositionManager.sol

461:            cache.token0Owed = cache.token0Owed < cache.tokenToPremium ? cache.token0Owed : cache.tokenToPremium;
462:            cache.token1Owed = cache.token1Owed < cache.tokenFromPremium ? cache.token1Owed : cache.tokenFromPremium;

Here, min(tokenOwed,tokenPremium) is taken, since tokenPremium would be 0 no fees would be owed.

Impact

A borrower can open a position without any premium. This removes any incentive to liquidate the position and also robs the liquidity provider of fees that would have been accrued.

In the extreme, if you for example open a long position above the price (thus with no leverage), a borrower will not need to provide any funds at all. Thus can spam positions like this locking liquidity providers liquidity and denying them fees (if the trading moves in their direction).

Proof of Concept

Actually, the basic test testBaseOpenLongPosition in OpenPosition.t.sol already reproduces this:

    function testOpenPositionWithoutPremium() public {
        testBaseOpenLongPosition();

        (,, uint128 token0Premium, uint128 token1Premium,,) = particleInfoReader.getOwedInfo(SWAPPER, 0);
        assertEq(0,token0Premium);
        assertEq(0,token1Premium);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider implementing a minimum enforced premium portion. Any premium portion would guarantee a reward to the liquidator.

Assessed type

Invalid Validation

@c4-bot-5 c4-bot-5 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 21, 2023
c4-bot-6 added a commit that referenced this issue Dec 21, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 21, 2023
@wukong-particle
Copy link

wukong-particle commented Dec 23, 2023

Good suggestion, will add a minimum premium parameter. Not agreeing with the severity though. In the worst case, the LP is still willing to close the position to reclaim liquidity. But will let the judge to ultimately decide. Thanks!

@0xleastwood
Copy link

This seems like a DoS on the LP's liquidity. It will still be possible for them to reclaim if they liquidate a position and subsequently withdraw their liquidity but this is still not ideal.

I guess my final question would be do trader's lose any funds when creating positions with no premium? Is there a financial disincentive to do this? This would be the difference between QA and medium severity imo. For the time-being I will downgrade to medium.

@c4-judge
Copy link
Contributor

0xleastwood changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Dec 23, 2023
@0xleastwood
Copy link

@wukong-particle I forgot to tag you in the above comment.

@wukong-particle
Copy link

Well, trader will lose their initial margin if the premium is 0 and got immediately liquidated. If the trader believes immediate liquidation won't happen (because there is no reward from premium), logically the trader will go with 0 premium because this would be the least amount of cost. We will add a minimum premium parameter here so we confirm the issue.

@c4-sponsor
Copy link

wukong-particle (sponsor) confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Dec 24, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Dec 24, 2023
@romeroadrian
Copy link

@0xleastwood Isn't this a duplicate of #27? I think both describe the same core issue with different examples/scenarios

@c4-judge
Copy link
Contributor

0xleastwood marked the issue as not selected for report

@c4-judge c4-judge removed the selected for report This submission will be included/highlighted in the audit report label Dec 31, 2023
@c4-judge c4-judge added duplicate-27 and removed primary issue Highest quality submission among a set of duplicates labels Dec 31, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as duplicate of #27

@0xleastwood
Copy link

@0xleastwood Isn't this a duplicate of #27? I think both describe the same core issue with different examples/scenarios

correct, it should be a duplicate of #27. Thanks

@c4-judge
Copy link
Contributor

0xleastwood marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-27 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

6 participants