Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L375 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L59

Vulnerability details

Impact

If a user calls NFTVault.finalizePendingNFTValueETH a second time without first calling JPEGLock.unlock to recover their previous lockup, their balance will be overwritten leaving the previous lockup balance unrecoverable.

Proof of Concept

POC by adding the following to NFTValue.ts line 640:

    // Repeat the previous few lines to set up another value change
    await nftVault
      .connect(dao)
      .setPendingNFTValueETH(index, units(50000));
    await jpeg.transfer(user.address, units(12000000));
    await jpeg.connect(user).approve(locker.address, units(12000000));
    await nftVault.connect(user).finalizePendingNFTValueETH(index);

    // Locker is holding both deposits
    expect(await jpeg.balanceOf(locker.address)).to.equal(units(12000000 * 2));

    // But only one is attributed to a user
    expect((await locker.positions(index)).lockAmount).to.eq(units(12000000))

Tools Used

Manual review and modifying existing tests.

Recommended Mitigation Steps

JPEGLock's lockFor could require positions[_nftIndex].lockAmount == 0 to avoid this issue. If the scenario is desirable (allowing the same user to finalize a value change for the same NFT) then maybe you could find a reasonable way to merge them or otherwise account for the original value locked; such as only calling jpeg.safeTransferFrom for the delta amount (possibly refunding a surplus).

code-423n4 / 2022-04-jpegd-findings