JPEG'd contest - Foundation's results

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).

• FlashEscrow constructor: Use OpenZeppelin Address to bubble up the underlying revert reason if found instead of just emitting a generic FlashEscrow: call_failed.
• LPFarming: newEpoch, add, and set should emit events. This would make it easier for frontends integrating (or subgraphs) to monitor and message significant changes like these.

• JPEGLock: pack owner + unlockAt saving ~21k gas. Could use up to 96 bits and still pack into 1 slot, 96 bits is way more than you need for a date.
• JPEGStaking: This contract upgradeable but the JPEG token is not. It's not common, but with an upgradeable contract you can use immutable variables to save gas. Using a constructor and switching to IERC20Upgradeable public immutable jpeg saves ~2.1k per stake/unstake call since there is no SLOAD required for immutable variables (they have the gas efficiency of a constant, except for the constructor cost itself).
• CryptoPunksHelper / EtherRocksHelper: These are the only use cases for NFTEscrow, suggesting that the nftAddress could be made immutable (similar to the point above), saving ~2.1k per call.
• NFTEscrow: Separate getSalt from precompute to avoid unnecessary calculations anytime _executeTransfer is called.
• JPEGStaking: line 46 _amount <= balanceOf(msg.sender) is redundant, the internal _burn call will validate that as well. Saving ~500 gas.
• Consistency / small gas savings: Cache loop length consistently. e.g. in LPFarming, _massUpdatePools caches but claimAll does not.