code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

When adding liqudity or transfer liqudity, the code does not check if the NFT token exists.

User lose the fund if the user transfer or add liquidity to a token that does not exist

Proof of Concept

This problem exists in Maverick-v1 / Pool#addLiquidity and Pool#transferLiquidity

function addLiquidity(
    uint256 tokenId,
    AddLiquidityParams[] calldata params,
    bytes calldata data
) external override returns (uint256 tokenAAmount, uint256 tokenBAmount, BinDelta[] memory binDeltas) {

and

function transferLiquidity(uint256 fromTokenId, uint256 toTokenId, RemoveLiquidityParams[] calldata params) external override {
    checkReentrancy(false, true);

    // ************************ reentrancy ************************

    checkPositionAccess(fromTokenId);
    for (uint256 i; i < params.length; i++) {
        RemoveLiquidityParams memory param = params[i];
        Bin.Instance storage bin = bins[param.binId];

        // no explicit requires to save gas; will revert if we transfer
        // more than we have.
        bin.balances[toTokenId] += param.amount;
        bin.balances[fromTokenId] -= param.amount;
    }
    emit TransferLiquidity(fromTokenId, toTokenId, params);

    // ************************ reentrancy ************************
    state.status &= ~LOCKED;
}

the POC demonstrate that user can lose their liqudity when they transfer liqudity to a not exist NFT token.

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/test/models/Pool.ts#L470

  describe.only("#transferLiquidity POC", async () => {
    beforeEach(async () => {
      await position.mint(ownerAddress);
      await testPool.addLiquidity(1, [
        {
          kind: 0,
          isDelta: true,
          pos: 0,
          deltaA: floatToFixed(500),
          deltaB: floatToFixed(500),
        },
      ]);
    });

    it("transfers a binLp position", async () => {
      let tokenId = 1;
      let binId = 1;
      let startLpBalance = await pool.balanceOf(tokenId, binId);
      expect(startLpBalance.gt(0)).to.be.true;

      await position.mint(ownerAddress);

      // let newTokenId = 2;
      let newTokenId = 3;
      await pool.transferLiquidity(tokenId, newTokenId, [
        { binId: binId, amount: startLpBalance },
      ]);

      let newBalance = await pool.balanceOf(tokenId, binId);
      let newBalance2 = await pool.balanceOf(newTokenId, binId);

      expect(newBalance.eq(0)).to.be.true;

      expect(newBalance2).to.eq(startLpBalance);
    });

  })

If we run the test under maverick-v1 folder.

 npx hardhat test

the output:

PS D:\Security\2022-12-Stealth-Project\maverick-v1> npx hardhat test
No need to generate any newer typings.

  Pool
    #transferLiquidity POC
      √ transfers a binLp position

note that we change:

await position.mint(ownerAddress);

// let newTokenId = 2;
let newTokenId = 3;

position.mint mint another NFT token Id to user and the newTokenId is 2, and token id 3 does not exist.

but user transfer the liqudity from token 1 and token 3 and succeed,

Given that there is no access control in Position mint function.

function mint(address to) external returns (uint256 tokenId) {
    tokenId = _tokenIdCounter.current();
    _tokenIdCounter.increment();
    _safeMint(to, tokenId);
}

anyone mint the nft with token id 3 can claim the liqudity.

Tools Used

Manual Review, Hardhat test

Recommended Mitigation Steps

We recommend the project check the exists of the token and check if the owner of the token id is msg.sender before adding liqudity and transfer liqudity and add access control to the position mint function.