NFTXHookCore.sol

The NFTXHookCore is the shared base for both the canonical and flex hooks. It sets the dynamic swap fee, takes the AMM fee and donates the LP share of any listing fees that have been queued against the pool.

[MED] Listing fee donations can be captured by just-in-time liquidity

The LP share of each listing tax is queued against the pool by depositFees and then paid out in a single poolManager.donate call the next time that a hook callback is fired. A donation is shared pro rata between whatever liquidity is in range at that exact moment, with no time weighting.

delete _pendingDonations[_poolId];

poolManager.donate(_poolKey, pending.amount0, pending.amount1, '');

As _distributeFees is called from _beforeAddLiquidity, _beforeRemoveLiquidity and _beforeSwap, the moment of the flush is controlled by the caller. This means that, in a single transaction, anyone can:

1. Add a tightly concentrated position around the current tick
2. Call a permissionless Listings function that realises a fee, such as filling an expired listing at floor, or cancelling their own listing
3. Remove the position, which flushes the donation whilst their liquidity is still in range

No swap takes place between the add and the remove, so the tick does not move and the position carries no price risk. As every seeded position is full range, a single tick-spacing position is around 650x more concentrated than the incumbent LPs. In our tests a position worth 1.5% of the pool TVL captured 90.9% of the donation, and 10% of TVL captured 98.5%. When the add, fill and remove were performed inside a single unlock, the attack captured over 99.99% of the donation with just 10 wei of flETH.

This also allows a lister to wrap the cancellation of their own listing and recover the LP share of their own tax. With a 90/10 LP to protocol split, the lister recovered 81.8% of the tax that they had paid, which undermines the tax that is intended to discipline listing price and duration.

If the pool has no liquidity in range then _distributeFees will defer the donation, so fees will accumulate. A dust position that is added and then immediately removed will receive the full amount that has built up.

LP principal is never at risk, but the listing fees are the main incentive for LPs and these can be repeatedly diverted for just the cost of gas.

We would recommend that the donation is streamed rather than paid out in a single call. By recording a lastFlush timestamp against each pool and only releasing pending * elapsed / STREAM_PERIOD, a position that exists for a single block would only earn a single block's share. It would also be beneficial to record the block that a position was added in _afterAddLiquidity and to skip the flush in _beforeRemoveLiquidity if the position was added in the same block.

As Locker.setImplementation can only be written to once, this fix would require a new hook to be deployed.

[INFO] tx.origin is used to determine fee exemptions

The swap fee is looked up against tx.origin, rather than the _sender of the swap.

// Always set the dynamic fee, even on no-op swaps
swapFee_ = getFee(poolId, tx.origin) | LPFeeLibrary.OVERRIDE_FEE_FLAG;

We understand that this is to allow an exemption to persist when swapping through a router. However, it does mean that any contract called by an exempt address will also inherit their fee exemption, and that smart contract wallets and bundled transactions will not be able to benefit from an exemption in the same way. This should be considered when approving an address.

[INFO] CurrencySettler is imported from the v4-core test directory

The CurrencySettler library is imported from the test directory of v4-core. This is also the case in NFTXFlexHook.sol and NFTXZap.sol.

import {CurrencySettler} from '@uniswap/v4-core/test/utils/CurrencySettler.sol';

Test utilities are not covered by the audits of the core Uniswap contracts and can change without notice between versions. We would recommend moving a copy of this library into the repository so that it is versioned and reviewed alongside the production code.

[INFO] renounceOwnership has not been disabled

The ProtocolFeeReceiver overrides renounceOwnership to prevent it from being called, but the hooks do not. If ownership of a hook was renounced then the fee configuration, fee exemptions, AMM beneficiary and oracle parameters would all be permanently locked. We would recommend applying the same override to the hooks.