c4-bot-6 commented 11 months ago

Lines of code

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

Vulnerability details

Impact

Providing marginTo when opening position will not increase premium but be stolen by MeV.

This can be mitigated by providing amountOutMin in swap params but the protocol should guarantee proper swap.

Proof of Concept

When opening a position a borrower can supply marginTo/From to increase their premium. So that their position doesn't immediately go under water when any trading happens.

marginTo doesn't work as intended though, it is used to calculate amountToMinimum when swapping (underflow handled in other issue):

ParticlePositionManager::openPosition:

File: contracts/protocol/ParticlePositionManager.sol

212:            collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement

Which is used to guarantee that enough tokens are received when swapping:

Base::swap:

File: contracts/libraries/Base.sol

65:        if (amountReceived < amountToMinimum) revert Errors.InsufficientSwap();

The issue is that the more marginTo a user provides, the less the protocol expects out of the swap. This will be picked up by bots and exploited. The marginTo will get "eaten" and only amountToMinimum will be left after the swap. Hence any premium the user thought they'd get from providing marginTo is lost to MeV.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider not counting marginTo towards expected output from the swap. As shown in another issue, there are further problems with this. marginTo is the premium for the "to"-side of the position. Hence should not be part of the expected output of the swap as it is the safety for the position.

Assessed type

MEV

c4-judge commented 11 months ago

0xleastwood marked the issue as primary issue

wukong-particle commented 11 months ago

Hmm the slippage should be protected by the minimum/maximum aftermath price inside the data that is passed to Base.swap. We are not primarily counting on collateralTo - cache.amountToBorrowed - params.marginTo for slippage protection.

It's probably hard to remove marginTo from this equation because it's quite essential to determine the minimum swap amount. We can keep the discussion in https://github.com/code-423n4/2023-12-particle-findings/issues/44 about this.

romeroadrian commented 11 months ago

Right, the slippage of the swap should be factored in the data sent to the aggregator. The min check there is to ensure swap output covers the required amount to get to the collateral.

c4-sponsor commented 11 months ago

wukong-particle (sponsor) disputed

0xleastwood commented 11 months ago

Seems like slippage can already be handled within the swap data itself. Invalidating this issue.