Farseen0/scar-pairs huggingface dataset:
HIGH severity: Denial of service for swaps on `MultiRangeHook` pools. **Description:** When a new range is created in `MultiRangeHook`, a new `IncentivizedERC20` LP token is registered and added to the array of tokens corresponding to the specific pool:

Since the action is permissionless, this means that anyone can create an unlimited number of ranges to make the...

Vulnerable code:
function createRange(PoolKey calldata key, RangeKey calldata rangeKey) external {
        ...
        address lpToken = address(new IncentivizedERC20(tokenSymbol, tokenSymbol, poolId));

        // Store the Range and LP token infos
        rangeLpToken[rangeId] = lpToken;
        validLpToken[lpToken] = true;
@>      poolLpTokens[poolId].push(lpToken);
        ...
    }

function afterSwap(address, PoolKey calldata key, IPoolManager.SwapParams calldata, BalanceDelta, bytes calldata)
    external
    override
    onlyPoolManager
    returns (bytes4, int128)
{
    PoolId poolId = key.toId();

    if (address(incentiveManager) != address(0)) {
        incentiveManager.notifySwap(poolId, poolLpTokens[poolId]);
    }

    return (MultiRangeHook.afterSwap.selector, 0);
}

function notifySwap(PoolId id, address[] calldata lpTokens) external onlyAllowedHooks {
    // @gas cheaper not to cache length for calldata array input
    for (uint256 i; i < lpTokens.length;) {
        _notifySwap(id, lpTokens[i]);
        unchecked {
            ++i;
        }
    }
}

function _notifySwap(PoolId id, address lpToken) internal {
    // Convert PoolId to IncentivizedPoolId
    IncentivizedPoolId _id = IncentivizedPoolKey({ id: id, lpToken: lpToken }).toId();
    if (poolLinkedHook[_id] != msg.sender) revert NotLinkedHook();
    // Get the list of Incentive Systems linked to the pool
    uint256[] memory _systems = _getPoolIncentiveSystemIndexes(_id);

    uint256 length = _systems.length;
    if (length > 0) {
        for (uint256 i; i < length;) {
            // @gas cheaper to read them both at same time using this style
            IncentiveSystem storage sRef = incentiveSystems[_systems[i]];
            (address system, bool updateOnSwap) = (sRef.system, sRef.updateOnSwap);
            if (updateOnSwap) {
                // Notify the Incentive Logic of the swap
                IIncentiveLogic(system).notifySwap(_id);
            }
            unchecked {
                ++i;
            }
        }
    }
}
HIGH severity: Rewards can be stolen when `IncentivizedER` tokens are recursively provided as liquidity. **Description:** By virtue of the composable nature of DeFi, it is common to have wrapped liquidity tokens that encapsulate some form of yield or other tokenized incentives in a manner similar to Valkyrie. With the ubiquity of LRTs and other forms of re-hypothecation, such as that enabled by...

Vulnerable code:
function settle(Currency currency, IPoolManager manager, address payer, uint256 amount, bool burn) internal {
    ...
    } else {
        manager.sync(currency);
        if (payer != address(this)) {
            IERC20Minimal(Currency.unwrap(currency)).transferFrom(payer, address(manager), amount);
        } else {
            IERC20Minimal(Currency.unwrap(currency)).transfer(address(manager), amount);
        }
        manager.settle();
    }
}

function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
    ITokenizedHook(hook).notifyTransfer(poolId, from, to, amount);

    return super.transferFrom(from, to, amount);
}

poolUserLiquidity[_id][from] -= amount;
poolUserLiquidity[_id][to] += amount;
HIGH severity: `IncentivizedER` should not be hardcoded to 18 decimals. **Description:** Whenever a new `IncentivizedERC20` is created to represent the liquidity of a given pool, its decimals are hardcoded to 18:

This is fine when both tokens in the Uniswap v4 pool have 18 decimals since the output liquidity will also have the same decimal precision. However, when...

Vulnerable code:
contract IncentivizedERC20 is ERC20, Owned {
    ...
    constructor(string memory name, string memory symbol, PoolId id) ERC20(name, symbol, 18) Owned(msg.sender) {
        hook = msg.sender;
        poolId = id;
    }
    ...
}

function test_LessThan18Decimals() public {
    MockERC20 token0WithLessDecimals = new MockERC20("TEST1", "TEST1", 18);
    token0WithLessDecimals.mint(address(this), 1_000_000 * 10 ** 18);
    MockERC20 token1WithLessDecimals = new MockERC20("TEST2", "TEST2", 6);
    token1WithLessDecimals.mint(address(this), 1_000_000 * 10 ** 6);

    PoolKey memory newKey = createPoolKey(address(token0WithLessDecimals), address(token1WithLessDecimals), 3000, hookAddress);
    PoolId newId = key.toId();

    token0WithLessDecimals.approve(address(fullRange), type(uint256).max);
    token1WithLessDecimals.approve(address(fullRange), type(uint256).max);

    initPool(newKey.currency0, newKey.currency1, IHooks(hookAddress), 3000, SQRT_PRICE_1_1);

    uint128 liquidityMinted = fullRange.addLiquidity(
        FullRangeHook.AddLiquidityParams(
            newKey.currency0,
            newKey.currency1,
            3000,
            100 * 10 ** 18,
            100 * 10 ** 6,
            0,
            0,
            address(this),
            MAX_DEADLINE
        )
    );

    assertEq(liquidityMinted, 100 * 10 ** 6);
}

function beforeInitialize(address, PoolKey calldata key, uint160)
        external
        override
        onlyPoolManager
        returns (bytes4)
    {
        if (key.tickSpacing != 60) revert TickSpacingNotDefault();

        PoolId poolId = key.toId();

        // Prepare the symbol for the LP token to be deployed
        string memory symbol0 =
            key.currency0.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency0)).symbol();
        string memory symbol1 =
            key.currency1.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency1)).symbol();

        string memory tokenSymbol =
            string(abi.encodePacked("UniV4", "-", symbol0, "-", symbol1, "-", Strings.toString(uint256(key.fee))));
        // Deploy the LP token for the Pool
++      uint8 token0Decimals = IERC20Metadata(Currency.unwrap(key.currency0)).decimals();
++      uint8 token1Decimals = IERC20Metadata(Currency.unwrap(key.currency1)).decimals();
++      uint8 liquidityDecimals = token0Decimals < token1Decimals ? token0Decimals : token1Decimals;
--      address poolToken = address(new IncentivizedERC20(tokenSymbol, tokenSymbol, poolId));
++      address poolToken = address(new IncentivizedERC20(tokenSymbol, tokenSymbol, poolId, liquidityDecimals));
        ...
    }
HIGH severity: ERtokens that do not implement `symbol()` are incompatible despite being accepted by Uniswap v4. **Description:** The protocol intends to support any ERC-20 token accepted by Uniswap v4; however, Uniswap v4 accepts tokens that do not implement the optional `symbol()` method from the ERC-20 standard.

In the following test, a Uniswap V4 pool is created with custom mocks that do not implement...

Vulnerable code:
function test_PoolCreationWithTokensWithoutSymbols() public {
    MockERC20WithoutSymbol token0WithoutSymbol = new MockERC20WithoutSymbol("TEST1", 18);
    token0WithoutSymbol.mint(address(this), 1_000_000 * 10 ** 18);
    MockERC20WithoutSymbol token1WithoutSymbol = new MockERC20WithoutSymbol("TEST2", 18);
    token1WithoutSymbol.mint(address(this), 1_000_000 * 10 ** 18);

    vm.expectRevert();
    IERC20Metadata(address(token0WithoutSymbol)).symbol();
    vm.expectRevert();
    IERC20Metadata(address(token1WithoutSymbol)).symbol();

    PoolKey memory newKey = createPoolKey(address(token0WithoutSymbol), address(token1WithoutSymbol), 3000, hookAddress);
    PoolId newId = key.toId();

    token0WithoutSymbol.approve(address(fullRange), type(uint256).max);
    token1WithoutSymbol.approve(address(fullRange), type(uint256).max);

    // address(0) is passed as the hook to prevent the revert from the actual FullRangeHook initialization
    initPool(newKey.currency0, newKey.currency1, IHooks(address(0)), 3000, SQRT_PRICE_1_1);
}

function beforeInitialize(address, PoolKey calldata key, uint160)
    external
    override
    onlyPoolManager
    returns (bytes4)
{
    ...
    // Prepare the symbol for the LP token to be deployed
    string memory symbol0 =
        key.currency0.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency0)).symbol();
    string memory symbol1 =
        key.currency1.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency1)).symbol();

    string memory tokenSymbol =
        string(abi.encodePacked("UniV4", "-", symbol0, "-", symbol1, "-", Strings.toString(uint256(key.fee))));
    // Deploy the LP token for the Pool
    address poolToken = address(new IncentivizedERC20(tokenSymbol, tokenSymbol, poolId));
    ...
}

function createRange(PoolKey calldata key, RangeKey calldata rangeKey) external {
    ...
    // Prepare the symbol for the LP token to be deployed
    string memory symbol0 =
        key.currency0.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency0)).symbol();
    string memory symbol1 =
        key.currency1.isAddressZero() ? NATIVE_CURRENCY_SYMBOL : IERC20Metadata(Currency.unwrap(key.currency1)).symbol();

    string memory tokenSymbol =
        string(abi.encodePacked("UniV4", "-", symbol0, "-", symbol1, "-", Strings.toString(uint256(key.fee))));
    // Deploy the LP token for the Pool
    address lpToken = address(new IncentivizedERC20(tokenSymbol, tokenSymbol, poolId));
    ...
}
MEDIUM severity: Donations can be made directly to the Uniswap v4 pool due to missing overrides. **Description:** Currently, the donation hooks are not activated:

Given the following implementations of `PoolManager::donate` and `Hooks::beforeDonate` in Uniswap v4, donations will still be permitted due to an absence of the default behavior being overridden:

The hooks don’t intend to have...

Vulnerable code:
function getHookPermissions() public pure virtual override returns (Hooks.Permissions memory) {
    return Hooks.Permissions({
        ...
        beforeAddLiquidity: true,
        beforeRemoveLiquidity: false,
        afterAddLiquidity: false,
        afterRemoveLiquidity: false,
        beforeDonate: false,
        afterDonate: false,
        ...
    });
}

function donate(PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData)
    external
    onlyWhenUnlocked
    noDelegateCall
    returns (BalanceDelta delta)
{
    PoolId poolId = key.toId();
    Pool.State storage pool = _getPool(poolId);
    pool.checkPoolInitialized();

    key.hooks.beforeDonate(key, amount0, amount1, hookData);

    delta = pool.donate(amount0, amount1);

    _accountPoolBalanceDelta(key, delta, msg.sender);

    // event is emitted before the afterDonate call to ensure events are always emitted in order
    emit Donate(poolId, msg.sender, amount0, amount1);

    key.hooks.afterDonate(key, amount0, amount1, hookData);
}

function beforeDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData)
    internal
    noSelfCall(self)
{
    if (self.hasPermission(BEFORE_DONATE_FLAG)) {
        self.callHook(abi.encodeCall(IHooks.beforeDonate, (msg.sender, key, amount0, amount1, hookData)));
    }
}
MEDIUM severity: Deposited rewards get stuck in incentive logic contracts when there is no pool liquidity. **Description:** For an interval of time during which there is zero liquidity in the pool and a reward has been actively distributed, the proportional amount of rewards that correspond to this interval when the liquidity has been zero will not be withdrawable by anyone.

**Impact:** While it is...

Vulnerable code:
function _updateRewardState(IncentivizedPoolId id, address token, address account) internal virtual {
        // Sync pool total liquidity if not already done
        if(!poolSynced[id]) {
            _syncPoolLiquidity(id);
            poolSynced[id] = true;
        }

        RewardData storage _state = poolRewardData[id][token];
        uint96 newRewardPerToken = _newRewardPerToken(id, token).toUint96();
        _state.rewardPerTokenStored = newRewardPerToken;

++      uint256 lastUpdateTimeApplicable = block.timestamp < _state.endTimestamp ? block.timestamp : _state.endTimestamp;
++      if (poolStates[id].totalLiquidity == 0) {
++          accumulatedFees[token] += (lastUpdateTimeApplicable - _state.lastUpdateTime) * _state.ratePerSec;
++      }
++      _state.lastUpdateTime = uint32(lastUpdateTimeApplicable);

--      uint32 endTimestampCache = _state.endTimestamp;
--      _state.lastUpdateTime = block.timestamp < endTimestampCache ? block.timestamp.toUint32() : endTimestampCache;

        // Update user state if an account is provided
        if (account != address(0)) {
            if(!userLiquiditySynced[id][account]) {
                _syncUserLiquidity(id, account);
                userLiquiditySynced[id][account] = true;
            }

            UserRewardData storage _userState = userRewardStates[id][account][token];
            _userState.accrued = _earned(id, token, account).toUint160();
            _userState.lastRewardPerToken = newRewardPerToken;
        }
    }

_state.lastUpdateTime = uint32(lastUpdateTimeApplicable);
MEDIUM severity: `TimeWeightedIncentiveLogic` distributions are not possible for tokens that revert on zero transfers. **Description:** When rewards in a given token are deposited to `TimeWeightedIncentiveLogic` for the first time, the cached end timestamp will be zero. Comparison with the current non-zero block timestamp causes execution to enter the conditional block that handles new distributions after the...

Vulnerable code:
function _depositRewards(
    IncentivizedPoolId id,
    address token,
    uint256 amount,
    uint256 duration,
    uint256 requiredDuration,
    RewardType rewardType
) internal {
    ...

    // Update the reward distribution parameters
    uint32 endTimestampCache = _state.endTimestamp;
    if (endTimestampCache < block.timestamp) {
        ...

        // Remove past rewards if the distribution is over
        _removePastRewards(id, token);

        ...
    } else {
        ...
    }

    ...
}

function _removePastRewards(IncentivizedPoolId id, address token) internal {
    address manager = distributionManagers[id][token];

    uint256 amount = withdrawableAmounts[id][token][manager];
    withdrawableAmounts[id][token][manager] = 0;

    IERC20(token).safeTransfer(manager, amount);

    emit RewardsWithdrawn(id, token, manager, amount);
}

function test_ZeroTransferDoS() public {
    ZeroTransferERC20 token = new ZeroTransferERC20("TKN", "TKN", 18);

    logic.updateDefaultFee(0);

    IncentivizedPoolId incentivizedId = IncentivizedPoolKey({ id: pool1, lpToken: lpToken1 }).toId();

    manager.setListedPool(incentivizedId, true);

    address[] memory systems = new address[](1);
    systems[0] = address(logic);

    uint256 amount = 1000 ether;
    uint256 duration = 5 weeks;
    uint256 requiredDuration = 1 weeks;

    manager.notifyAddLiquidty(systems, pool1, lpToken1, user1, int256(100 ether));

    vm.expectRevert();
    logic.depositRewards(
        incentivizedId,
        address(token),
        amount,
        duration,
        requiredDuration,
        TimeWeightedIncentiveLogic.RewardType.WITHDRAW
    );
}
LOW severity: Missing tick validation when creating a new range in `MultiRangeHook`. **Description:** When adding liquidity to a pool configured with the `MultiRangeHook`, it is necessary to first initialize the pool and create the desired range. Users are limited to adding liquidity to tick ranges that are multiples of the configured `tickSpacing` of the pool, such that an...

Vulnerable code:
function test_InvalidTickRangeCreation() public {
    int24 lowerTick = -90;
    int24 upperTick = 90;

    // Pool with 20 tick spacing
    initPool(key2.currency0, key2.currency1, IHooks(address(multiRange)), 1000, SQRT_PRICE_1_1);

    rangeKey = RangeKey(id2, lowerTick, upperTick);
    rangeId = rangeKey.toId();

    multiRange.createRange(key2, rangeKey);

    uint256 token0Amount = 100 ether;
    uint256 token1Amount = 100 ether;

    vm.expectRevert(abi.encodeWithSelector(TickBitmap.TickMisaligned.selector, lowerTick, int256(int24(20))));
    multiRange.addLiquidity(
        MultiRangeHook.AddLiquidityParams(
            key2, rangeKey, token0Amount, token1Amount, 99 ether, 99 ether, address(this), MAX_DEADLINE
        )
    );
}

function createRange(PoolKey calldata key, RangeKey calldata rangeKey) external {
++      if(
++          rangeKey.tickLower % key.tickSpacing != 0 ||
++          rangeKey.tickUpper % key.tickSpacing != 0
++      ) revert();


        PoolId poolId = key.toId();
        RangeId rangeId = rangeKey.toId();

        if (!initializedPools[poolId]) revert PoolNotInitialized();
        if (alreadyCreatedRanges[poolId][rangeId] || rangeLpToken[rangeId] != address(0)) revert RangeAlreadyCreated();
        alreadyCreatedRanges[poolId][rangeId] = true;

        ...
    }
LOW severity: Missing zero address validation in `BoostedIncentiveLogic`. **Description:** Unlike in the `BaseIncentiveLogic` constructor, `BoostedIncentiveLogic` fails to validate `_boostingPowerAdaptor` against the zero address which could result in Dos if erroneously configured in this way.

**Recommended Mitigation:**

**Paladin:** Fixed by commit...

Vulnerable code:
constructor(address _incentiveManager, uint256 _defaultFee, address _boostingPowerAdaptor)
        BaseIncentiveLogic(_incentiveManager, _defaultFee)
    {
++      if (_boostingPowerAdaptor == address(0)) revert();
        boostingPowerAdaptor = IBoostingPowerAdapter(_boostingPowerAdaptor);
    }
LOW severity: Value mistakenly sent to `PoolCreator` can be permanently locked. **Description:** The `PoolCreator` is a simple contract designed to create Uniswap v4 pools. The `createPool()` function is currently marked `payable`, however this is not necessary as no value is passed to `PoolManager`:

Due to the absence of any refunds or functions to sweep native tokens, any...

Vulnerable code:
function createPool(PoolKey calldata key, uint160 sqrtPriceX96) external payable returns (PoolId poolId, int24 tick) {
    poolId = key.toId();
    tick = poolManager.initialize(key, sqrtPriceX96);
}
LOW severity: Unsafe downcast in `ValkyrieSubscriber::toInt256` could silently overflow. **Description:** While it is highly unlikely that liquidity amounts will ever get close to overflowing `int256` for tokens with a reasonable number of decimals, there is an unsafe downcast in `ValkyrieSubscriber::toInt256` from `uint256` that could silently overflow:

This function is called with...

Vulnerable code:
function toInt256(uint256 y) internal pure returns (int256 z) {
    z = int256(y);
}

function test_notifyUnsubscribe_Overflow() public {
    IncentivizedPoolId expectedId = IncentivizedPoolKey({ id: id, lpToken: address(0) }).toId();

    positionManager.notifySubscribe(0, EMPTY_BYTES);
    positionManager.notifySubscribe(5, EMPTY_BYTES);

    uint256 amount = uint256(type(int256).max);
    uint256 halfAmount = amount / 2;
    // add half the amount twice to overflow int256
    positionManager.notifyModifyLiquidity(5, int256(halfAmount), feeDelta);
    positionManager.notifyModifyLiquidity(5, int256(halfAmount), feeDelta);

    (IncentivizedPoolId receivedId, address account, int256 liquidityDelta) =
    incentiveManager.lastNotifyAddData(address(subscriber));

    assertEq(IncentivizedPoolId.unwrap(receivedId), IncentivizedPoolId.unwrap(expectedId));
    assertEq(account, owner2);
    assertEq(liquidityDelta, (int256(halfAmount)));

    // now remove full amount
    console.log("unsubscribing: notifies removal of all liquidity");
    positionManager.notifyUnsubscribe(5);

    (receivedId, account, liquidityDelta) =
    incentiveManager.lastNotifyRemoveData(address(subscriber));

    assertEq(IncentivizedPoolId.unwrap(receivedId), IncentivizedPoolId.unwrap(expectedId));
    assertEq(account, owner2);
    uint256 deltaU256 = uint256(-liquidityDelta);
    console.log("actually notified removal of %s liquidity due to overflow", deltaU256 / 1e18);
    assertGt(amount, deltaU256);
    console.log("unsubscribe removed all %s liquidity but reported different delta", amount / 1e18);
}

[PASS] test_notifyUnsubscribe_Overflow() (gas: 256275)
Logs:
  unsubscribing: notifies removal of all liquidity
  actually notified removal of 220 liquidity due to overflow
  unsubscribe removed all 57896044618658097711785492504343953926634992332820282019728 liquidity but reported different delta
LOW severity: Positions can be locked by malicious re-initialization in the event of multiple `ValkyrieSubscriber` contracts being deployed. **Description:** `IncentiveManager::notifyInitialize` does not currently check whether there is already an assigned hook corresponding to a give pool identifier:

Therefore, if this function can be called again then the permissioned role can be taken, preventing the previous one from executing any.....

Vulnerable code:
function notifyInitialize(PoolId id, address lpToken) external onlyAllowedHooks {
    IncentivizedPoolId _id = _convertToIncentivizedPoolId(id, lpToken);
    listedPools[_id] = true;
    poolLinkedHook[_id] = msg.sender;
}

function notifySubscribe(uint256 tokenId, bytes memory /*data*/) external override onlyPositionManager {
    (PoolKey memory poolKey,) = positionManager.getPoolAndPositionInfo(tokenId);
    PoolId poolId = poolKey.toId();

    if(!_initializedPools[poolId]) {
        if (address(incentiveManager) != address(0)) {
            incentiveManager.notifyInitialize(poolId, address(0));
        }
        _initializedPools[poolId] = true;
    }
    ...
}

function notifyAddLiquidty(PoolId id, address lpToken, address account, int256 liquidityDelta)
    external
    onlyAllowedHooks
{
    ...
    if (poolLinkedHook[_id] != msg.sender) revert NotLinkedHook();
    ...
}

function notifyRemoveLiquidty(PoolId id, address lpToken, address account, int256 liquidityDelta)
    external
    onlyAllowedHooks
{
    ...
    if (poolLinkedHook[_id] != msg.sender) revert NotLinkedHook();
    ...
}

function _notifySwap(PoolId id, address lpToken) internal {
    ...
    if (poolLinkedHook[_id] != msg.sender) revert NotLinkedHook();
    ...
}
HIGH severity: Unused custom errors and constants can be removed. **Description:** * `MultiRangeHook` [declares](https://github.com/PaladinFinance/Valkyrie/blob/6b97685d127c97bc369c0613943f45a547d89b18/src/hooks/MultiRangeHook.sol#L68-L69) the `RangeNotCreated()` custom error but it is never used. It would make sense to perform an associated check within...

Vulnerable code:
address lpToken = rangeLpToken[params.range.toId()];
if (lpToken == address(0)) revert RangeNotInitialized();

function beforeInitialize(address, PoolKey calldata key, uint160)
    external
    override
    onlyPoolManager
    returns (bytes4)
{
    if (key.tickSpacing != 60) revert TickSpacingNotDefault();
    ...
}
HIGH severity: Insufficient validation on the pool key in liquidity-modifying functions. **Description:** `FullRangeHook::addLiquidity` and `FullRangeHook::removeLiquidity` should only be callable with pools created via the hook itself; however there is a lack of validation on the pool key that means this is not strictly enforced. Instead, the current logic relies on the revert...

Vulnerable code:
++ if (poolInfo[poolId].liquidityToken == address(0)) revert InvalidLiquidityToken();