Liquidity may be lost when sent to a non-existent token

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L134

Vulnerability details

Impact

Users may lose funds when providing liquidity to a non-existent token. A wrong token ID may be provided mistakenly or due to a software bug.

Proof of Concept

The protocol implements liquidity position NFT: a collection of NFT tokens that are used to identify liquidity providers in pools (Position.sol#L12). When providing liquidity to pools, liquidity providers must specify an NFT token ID (Pool.sol#L114). Provided liquidity will be associated with the token ID, and only the owner of the token or an approved address will be able to withdraw liquidity from pools (Pool.sol#L217).

However, the addLiquidity function of Pool doesn't validate the user-provided token ID: it can be any value from the full range of the values of uint256, no matter whether a token exists or not:

function addLiquidity(
    uint256 tokenId,
    AddLiquidityParams[] calldata params,
    bytes calldata data
) external override returns (uint256 tokenAAmount, uint256 tokenBAmount, BinDelta[] memory binDeltas) {
    ...
    for (uint256 i; i < params.length; i++) {
      ...
        (temp.deltaA, temp.deltaB, temp.deltaLpBalance) = bin.addLiquidity(
            tokenId, // @audit missing tokenId validation
            Math.fromScale(param.deltaA, tokenAScale),
            Math.fromScale(param.deltaB, tokenBScale),
            currentState.activeTick,
            temp.reserveA,
            temp.reserveB
        );

In case a user provides liquidity to a non existent token, it cannot be withdrawn due to the ownership check (Pool.sol#L217):

function removeLiquidity(
    address recipient,
    uint256 tokenId,
    RemoveLiquidityParams[] calldata params
) external override returns (uint256 tokenAOut, uint256 tokenBOut, BinDelta[] memory binDeltas) {
    ...
    checkPositionAccess(tokenId);
    ...
}

function checkPositionAccess(uint256 tokenId) internal view {
    require(msg.sender == position.ownerOf(tokenId) || msg.sender == position.getApproved(tokenId), "P");
}

Thus, when liquidity is provided to a non-existent NFT, there are two scenarios:

1. If the ID can be minted in the future, the minter of the token will be the owner of the liquidity.
2. If it cannot be minted (e.g. the wrong token ID was set to type(uint256).max), the liquidity wil be locked in the contract forever. This is loss of funds.

Here's a proof of concept that demonstrates providing liquidity to a non-existent token ID and the inability to withdraw liquidity afterwards:

// router-v1/test/Router.ts
//   describe("#getOrCreatePoolAndAddLiquidity", () => {
it("allows to add liquidity to non-existent tokens [AUDIT]", async () => {
  let pool: string = await getEthBPool(0.05, 5);
  const prePoolBalance = await balances(pool);
  const nonexistentTokenId = 31337;

  await router.addLiquidityToPool(
    pool,
    nonexistentTokenId,
    [
      {
        kind: 0,
        isDelta: true,
        pos: 0,
        deltaA: floatToFixed(5),
        deltaB: floatToFixed(5),
      },
    ],
    0,
    0,
    maxDeadline
  );

  const postPoolBalance = await balances(pool);
  expect(postPoolBalance.weth9 - prePoolBalance.weth9).to.eq(5);
  expect(postPoolBalance.tokenB - prePoolBalance.tokenB).to.eq(5);

  // Reverts with 'ERC721: invalid token ID'
  await router.removeLiquidity(
    pool,
    await owner.getAddress(),
    nonexistentTokenId,
    [
      {
        binId: 1,
        amount: floatToFixed(5),
        maxDepth: 0,
      },
    ],
    0,
    0,
    maxDeadline
  );
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider disallowing providing liquidity to non-existent NFT IDs.

[hansfriese]

This reminds me of a discussion regarding the NFT standards.

It is not the job of a smart contract to protect against every possible user error (it must protect against invalid actions, not unintended actions). ... Smart contracts don't have the equivalent of a "This seems unsafe, are you sure?" dialog box!

Duplicate of #13

[c4-judge]

kirk-baird marked the issue as duplicate of #13

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[kirk-baird]

For reasons stated in #13 I'm going to downgrade this to QA.

[c4-judge]

kirk-baird marked the issue as grade-b