search

code-423n4 / 2024-06-krystal-defi-findings

0 stars 0 forks source link

The Protocol doesn´t cover all the Liquidity Positions as declared #16

Closed c4-bot-6 closed 2 months ago

c4-bot-6 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L640

Vulnerability details

Impact

Inability and duality in LP management, breaking the business logic

Proof of Concept

The website states that:

While most liquidity management platforms have their own preset strategies that LPs cannot modify, Krystal believes in empowering LPs to customize their own strategies. We aim to be the best liquidity management platform by providing advanced tools and analysis for both passive and active LPs to build customized strategies that suit their needs.

So the declaration is to manage all the liquidity positions.

However, if a user has a position created by pool.mint and later on with a call from nfpm.mint either through Krystal or not, the Krystal only sees and parses the nfpm positions by calling position method.

This is because the positions created by pool.mint and the positions created by NonfungiblePositionManager.mint are tracked separately. The former is an internal record in the pool contract, while the latter is an NFT.

So the protocol is not fulfilling their declaration in their implementation.

E.g.:

  1. Alice has a position of 10_000 DAI where she created her LP with pool.mint
  2. Alice mints another position by calling nfpm.mint and holds 15_000 USDC for a different LP
  3. Alice logs into Krystal Dapp and she only sees her DAI LP.
  4. Alice doesn´t use the Dapp again since it´s not fulfilling the declared feature - LP management center

Tools Used

Manual Review

Recommended Mitigation Steps

Implement the getter function of Uniswap´s Position.sol library too.

Assessed type

Other

c4-judge commented 2 months ago

3docSec marked the issue as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as selected for report

jarvisnn commented 2 months ago

Disputed. It's expected, we only consider normal users, who user Uniswap UI, not interacting directly with the contracts.

3docSec commented 2 months ago

Invalid: missing feature

c4-judge commented 2 months ago

3docSec marked the issue as unsatisfactory: Invalid

0xSorryNotSorry commented 2 months ago

Hi @3docSec,

Thank you for your time and for judging the contest swiftly.

Our sources to audit the codebase are the docs and the contracts. Hence, we didn´t ask for a feature as quoted while the docs clearly state that the protocol is a Liquidity Management Platform. If only the docs stated that the protocol doesn´t govern positions created by the pool.mint method, we wouldn´t visit this issue.

Most importantly, it´s not declared anywhere including the contest page - Scoping Section or Publicly Known Issues Section

We definitely don´t want to push this with all respect to the great dev team and the Judge, to you sir, however, declaring a feature in the docs and partly implementing it in the codebase is a flaw.

We seek your consideration through the above view and accept this submission as Medium.

Thank you for your time again.

3docSec commented 2 months ago

declaring a feature in the docs and partly implementing it in the codebase is a flaw

I agree with this point, but then there is the question of whether the flaw is in the code or in the docs.

Because the functionality was not "faulty" but rather "not coded at all" we don't have a reasonable fit with the C4 severity definition for M, which is the source of truth for what goes where