Farseen0/scar-pairs huggingface dataset:
LOW severity: Dynamic LP fees will remain zero by default unless explicitly updated. **Description:** As given by the logic in `LPFeeLibrary::getInitialLPFee`, dynamic fee pools initialize with an LP fee of 0%:

While `AngstromL2::setPoolLPFee` allows the contract owner to update the dynamic LP fee, there will be a period following initialization when it is not set. As such, it...

Vulnerable code:
function getInitialLPFee(uint24 self) internal pure returns (uint24) {
    // the initial fee for a dynamic fee pool is 0
    if (self.isDynamicFee()) return 0;
    self.validate();
    return self;
}

function test_zeroInitialLPFee() public {
    PoolKey memory key = initializePool(address(token), 10, 3);
    assertEq(manager.getSlot0(key.toId()).lpFee(), 0);
}
LOW severity: `TickIterator::_advanceToNextUp` sets uninitialized end tick as the current tick which causes `TickIterator::hasNext` to return true when this is not actually the case. **Description:** `TickIterator::hasNext` returns true if there are more ticks to iterate, inclusive if the end tick, while `TickIterator::getNext` returns the next tick and advances the iterator:

`TickIterator::_advanceToNextUp` intends to advance the upward tick iterator to the next initialized...

Vulnerable code:
function hasNext(TickIteratorUp memory self) internal pure returns (bool) {
@>   return self.currentTick <= self.endTick;
}

function getNext(TickIteratorUp memory self) internal view returns (int24 tick) {
    if (!hasNext(self)) revert NoNext();
    tick = self.currentTick;
    _advanceToNextUp(self);
}

function _advanceToNextUp(TickIteratorUp memory self) private view {
    do {
        (int16 wordPos, uint8 bitPos) =
            TickLib.position(TickLib.compress(self.currentTick, self.tickSpacing) + 1);

        if (bitPos == 0) {
            self.currentWord = self.manager.getPoolBitmapInfo(self.poolId, wordPos);
        }

        bool initialized;
        (initialized, bitPos) = self.currentWord.nextBitPosGte(bitPos);
@>      self.currentTick = TickLib.toTick(wordPos, bitPos, self.tickSpacing);
@>      if (initialized) break;
@>  } while (self.currentTick < self.endTick);

function test_iterateUp_phantomAtTopOfWordBoundary() public view {
    // No liquidity anywhere.

    int24 startTick = 2500;
    int24 endTick = 2550;
    TickIteratorUp memory iter =
        TickIteratorLib.initUp(manager, pid, TICK_SPACING, startTick, endTick);

    // Erroneously returns true - should be false with no initialized ticks up to boundary.
    bool hasNext = iter.hasNext();
    console2.log("iter.hasNext(): %s", hasNext);

    // Returns endTick even though it isn't initialized - should revert with NoNext().
    int24 tick = iter.getNext();
    console2.log("iter.getNext(): %s", tick);
    console2.log("endTick: %s", endTick);

    // Prove the returned tick is not initialized.
    (int16 wordPos, uint8 bitPos) = TickLib.position(TickLib.compress(tick, TICK_SPACING));
    uint256 word = IPoolManager(address(manager)).getPoolBitmapInfo(pid, wordPos);
    console2.log("isInitialized: %s", TickLib.isInitialized(word, bitPos));
}

function test_iterateDown_noPhantomAtBottomOfWordBoundary() public view {
    // No liquidity anywhere.

    int24 startTick = 50;
    int24 endTick = 0;

    TickIteratorDown memory iter =
        TickIteratorLib.initDown(manager, pid, TICK_SPACING, startTick, endTick);

    // Should be exhausted - no initialized ticks in (0, 50].
    assertFalse(iter.hasNext(), "Down iterator has a phantom next tick");
}
LOW severity: `CompensationPriceFinder` amount delta rounding directions are not consistent with Uniswap. **Description:** `CompensationPriceFinder::getZeroForOne` and `CompensationPriceFinder::getOneForZero` both round down in every instance when invoking `SqrtPriceMath::getAmount0Delta` and `SqrtPriceMath::getAmount1Delta`:

To be consistent with how Uniswap charges swaps in...

Vulnerable code:
uint256 delta0 =
    SqrtPriceMath.getAmount0Delta(priceLowerSqrtX96, priceUpperSqrtX96, liquidity, false);
uint256 delta1 =
    SqrtPriceMath.getAmount1Delta(priceLowerSqrtX96, priceUpperSqrtX96, liquidity, false);

if (exactIn) {
    uint256 amountRemainingLessFee =
        FullMath.mulDiv(uint256(-amountRemaining), MAX_SWAP_FEE - _feePips, MAX_SWAP_FEE);
    amountIn = zeroForOne
@>      ? SqrtPriceMath.getAmount0Delta(sqrtPriceTargetX96, sqrtPriceCurrentX96, liquidity, true)
@>      : SqrtPriceMath.getAmount1Delta(sqrtPriceCurrentX96, sqrtPriceTargetX96, liquidity, true);
    if (amountRemainingLessFee >= amountIn) {
        // `amountIn` is capped by the target price
        ...
    } else {
        // exhaust the remaining amount
        ...
    }
    amountOut = zeroForOne
@>      ? SqrtPriceMath.getAmount1Delta(sqrtPriceNextX96, sqrtPriceCurrentX96, liquidity, false)
@>      : SqrtPriceMath.getAmount0Delta(sqrtPriceCurrentX96, sqrtPriceNextX96, liquidity, false);
} else {
LOW severity: `CompensationPriceFinder::getZeroForOne` may compute smaller effective prices than expected. **Description:** `CompensationPriceFinder::getOneForZero` contains a conditional branch that exists to skip execution that would result in reverts either due to underflow or division by zero:

This logic is also present in `CompensationPriceFinder::getZeroForOne`; however, in this case, neither...

Vulnerable code:
@>  if (sumAmount0Deltas > taxInEther) {
@>      uint256 simplePstarX96 = sumAmount1Deltas.divX96(sumAmount0Deltas - taxInEther);
        if (simplePstarX96 <= uint256(priceUpperSqrtX96).mulX96(priceUpperSqrtX96)) {
            pstarSqrtX96 = _oneForZeroGetFinalCompensationPrice(...);

            return (lastTick, pstarSqrtX96);
        }
    }

@>  if (sumAmount0Deltas > taxInEther) {
        if (
@>          sumAmount1Deltas.divX96(sumAmount0Deltas + taxInEther)
                >= uint256(priceLowerSqrtX96).mulX96(priceLowerSqrtX96)
        ) {
            pstarSqrtX96 = _zeroForOneGetFinalCompensationPrice(...);

            return (lastTick, pstarSqrtX96);
        }
    }

if (simplePstarX96 > uint256(priceLowerSqrtX96).mulX96(priceLowerSqrtX96)) {
LOW severity: Dust due to rounding tax calculations will accumulate in `Angstrom` and cannot be recovered. **Description:** The sum of rewards owed to positions can be less than the total tax amount due to rounding. While it is correct to round down these proportional share calculations, any dust that is not accounted to `unclaimedProtocolRevenueInEther` will remain locked in the `AngstromL2`...

Vulnerable code:
// Distribute remainder to last range and update global accumulator.
unchecked {
    cumulativeGrowthX128 += PoolRewardsLib.getGrowthDelta(taxInEther, liquidity);
    rewards[ticks.poolId].globalGrowthX128 += cumulativeGrowthX128;
}

function test_rewardDust() public {
    PoolKey memory key = initializePool(address(token), 10, 3);

    uint256 PRIORITY_FEE = 10 gwei;
    setPriorityFee(PRIORITY_FEE);
    uint256 tax = angstrom.getSwapTaxAmount(PRIORITY_FEE);

    int24[2][] memory positions = new int24[2][](3);
    positions[0] = [int24(0), int24(10)];
    positions[1] = [int24(10), int24(20)];
    positions[2] = [int24(20), int24(30)];

    for (uint256 i = 0; i < positions.length; i++) {
        addLiquidity(key, positions[i][0], positions[i][1], 1e22);
    }

    router.swap(key, false, 1000e18, int24(25).getSqrtPriceAtTick());

    uint256 cumulativeRewards;

    for (uint256 i = 0; i < positions.length; i++) {
        cumulativeRewards += angstrom.getPendingPositionRewards(
            key, address(router), positions[i][0], positions[i][1], bytes32(0)
        );
    }

    assertEq(cumulativeRewards, tax, "All tax should be rewarded/accounted");
}
LOW severity: `IBeforeInitializeHook` should be added to the `Angstrom` inheritance chain. **Description:** `AngstromL2.sol` imports the `IBeforeInitializeHook` interface; however, it is not currently used. Given that `AngstromL2` is expected to implement this hook, it should be added to the inheritance chain:

**Sorella Labs:** Fixed in commit...

Vulnerable code:
contract AngstromL2 is
    UniConsumer,
    Ownable,
+   IBeforeInitializeHook
    IBeforeSwapHook,
    IAfterSwapHook,
    IAfterAddLiquidityHook,
    IAfterRemoveLiquidityHook
{
    ...
}
LOW severity: Modifier-style base `Ownable()` constructor call can be removed from `Angstrom`. **Description:** `AngstromL2` inherits the `Ownable` contract which has no constructor. Instead, it is expected that the `_initializeOwner()` function is called, as is currently the case:

Therefore, the modifier-style base constructor call without arguments can be removed to silent the `forge...

Vulnerable code:
constructor(IPoolManager uniV4, address owner, IFlashBlockNumber flashBlockNumberProvider)
    UniConsumer(uniV4)
@>  Ownable()
{
@>  _initializeOwner(owner);
    ...
}

constructor(IPoolManager uniV4, address owner, IFlashBlockNumber flashBlockNumberProvider)
    UniConsumer(uniV4)
-   Ownable()
{
    _initializeOwner(owner);
    ...
}
LOW severity: `rewardGrowthOutsideX128` is not correctly initialized in `PoolRewards::updateAfterLiquidityAdd`. **Description:** The Uniswap V3/V4 convention is that if a tick has just been initialized, and the current tick is to the right of that tick, then its `feeGrowthOutside[0/1]X1278` values must be initialized with `feeGrowthGlobal[0/1]X128`.

`PoolRewards::updateAfterLiquidityAdd` intends to...

Vulnerable code:
gi - lgi
= rgo[10] - rgo[0] - lgi
= 0 - 3 - 997 = 997 - 997 = 0

gi - lgi
== G - rgo[0] - rgo[10] - lgi
== 10 - 3 - 10 - 997
== 997 - 997
== 0

gi - lgi
== rgo[0] - rgo[10] - lgi
== 7 - 10 - 997
== 997 - 997
== 0
LOW severity: Remove unnecessary local variables. **Description:** The following functions in `AngstromL2` assign an unnecessary local variable `priorityFee` that can be removed:

The `simplePstarX96` variable within `CompensationPriceFinder::getOneForZero` can also be inlined and removed:

**Sorella Labs:** Acknowledged. Will leave the variables.....

Vulnerable code:
function _getSwapTaxAmount() internal view returns (uint256) {
-   uint256 priorityFee = tx.gasprice - block.basefee;
-   return getSwapTaxAmount(priorityFee);
+   return getSwapTaxAmount(tx.gasprice - block.basefee);
}

function _getJitTaxAmount() internal view returns (uint256) {
    if (_getBlock() == _blockOfLastTopOfBlock) {
        return 0;
    }
-   uint256 priorityFee = tx.gasprice - block.basefee;
-   return getJitTaxAmount(priorityFee);
+   return getJitTaxAmount(tx.gasprice - block.basefee);
}

if (sumAmount0Deltas > taxInEther) {
-   uint256 simplePstarX96 = sumAmount1Deltas.divX96(sumAmount0Deltas - taxInEther);
-   if (simplePstarX96 <= uint256(priceUpperSqrtX96).mulX96(priceUpperSqrtX96)) {
+   if (
+       sumAmount1Deltas.divX96(sumAmount0Deltas - taxInEther)
+           <= uint256(priceUpperSqrtX96).mulX96(priceUpperSqrtX96)
+   ) {
        pstarSqrtX96 = _oneForZeroGetFinalCompensationPrice(
            liquidity,
            priceLowerSqrtX96,
            taxInEther,
            sumAmount0Deltas - delta0,
            sumAmount1Deltas - delta1
        );

        return (lastTick, pstarSqrtX96);
    }
}
LOW severity: `Angstrom:beforeInitialize` conditionals can be combined. **Description:** `AngstromL2::beforeInitialize` validates support for both dynamic fees and native currency within the pool key:

These conditionals both revert with the same `IncompatiblePoolConfiguration()` custom error and so can be combined to save gas.

**Recommended Mitigation:**

**Sorella...

Vulnerable code:
function beforeInitialize(address, PoolKey calldata key, uint160)
    external
    view
    returns (bytes4)
{
    _onlyUniV4();
    if (key.currency0.toId() != NATIVE_CURRENCY_ID) revert IncompatiblePoolConfiguration();
    if (!LPFeeLibrary.isDynamicFee(key.fee)) revert IncompatiblePoolConfiguration();
    return this.beforeInitialize.selector;
}

function beforeInitialize(address, PoolKey calldata key, uint160)
    external
    view
    returns (bytes4)
{
    _onlyUniV4();
-   if (key.currency0.toId() != NATIVE_CURRENCY_ID) revert IncompatiblePoolConfiguration();
-   if (!LPFeeLibrary.isDynamicFee(key.fee)) revert IncompatiblePoolConfiguration();
+   if (key.currency0.toId() != NATIVE_CURRENCY_ID || !LPFeeLibrary.isDynamicFee(key.fee)) {
+       revert IncompatiblePoolConfiguration();
+   }
    return this.beforeInitialize.selector;
}
LOW severity: Unnecessary arithmetic validation within `Angstrom:withdrawProtocolRevenue` can be removed. **Description:** `AngstromL2::withdrawProtocolRevenue` first validates whether `unclaimedProtocolRevenueInEther` is sufficient to cover a withdrawal of `amount` by the owner; however, this is not necessary and can be removed as the subsequent decrement would panic revert due to...

Vulnerable code:
function withdrawProtocolRevenue(uint160 assetId, address to, uint256 amount) public {
    _checkOwner();

    if (assetId == NATIVE_CURRENCY_ID) {
@>      if (!(amount <= unclaimedProtocolRevenueInEther)) {
            revert AttemptingToWithdrawLPRewards();
        }
@>      unclaimedProtocolRevenueInEther -= amount.toUint128();
    }

    UNI_V4.transfer(to, assetId, amount);
}

function withdrawProtocolRevenue(uint160 assetId, address to, uint256 amount) public {
    _checkOwner();

    if (assetId == NATIVE_CURRENCY_ID) {
-       if (!(amount <= unclaimedProtocolRevenueInEther)) {
-           revert AttemptingToWithdrawLPRewards();
-       }
        unclaimedProtocolRevenueInEther -= amount.toUint128();
    }

    UNI_V4.transfer(to, assetId, amount);
}
LOW severity: `Angstrom:_computeAndCollectProtocolSwapFee` computation can be simplified. **Description:** `AngstromL2::_computeAndCollectProtocolSwapFee` currently performs the following computation:

However, the highlighted line can be simplified to:

**Sorella Labs:** Fixed in commit [aa90806](https://github.com/SorellaLabs/l2-angstrom/commit/aa9080697d683aae327de2c64f638f2730c193dd)...

Vulnerable code:
uint256 fee = exactIn
        ? absTargetAmount * protocolFeeE6 / FACTOR_E6
@>      : absTargetAmount * FACTOR_E6 / (FACTOR_E6 - protocolFeeE6) - absTargetAmount;
    fee128 = fee.toInt128();

absTargetAmount * protocolFeeE6 / (FACTOR_E6 - protocolFeeE6)
MEDIUM severity: Uninitialized CCTP domain mapping can send USDC to the incorrect blockchain. **Description:** `USDCBridgeV2::chainIdToCCTPDomain` maps Wormhole chain IDs to Circle's CCTP domain IDs:

**Impact:** When this mapping isn't initialized for a wormhole chain id, it returns 0 by default (Solidity's default value for `uint32`). However [Circle's CCTP domain 0 is Ethereum...

Vulnerable code:
mapping(uint16 => uint32) public chainIdToCCTPDomain;

function getCCTPDomain(uint16 _chain) internal view returns (uint32) {
    return chainIdToCCTPDomain[_chain];  // @audit returns 0 if not set!
}

circleTokenMessenger.depositForBurn(
            _amount,
            getCCTPDomain(_targetChain), // @audit 0 by default = Ethereum mainnet
            targetAddressBytes32,        // mintRecipient on destination
            USDC,          // burnToken
            destinationCallerBytes32,        // destinationCaller (restrict who can mint)
            0,
            1000
        );

function getCCTPDomain(uint16 _chain) internal view returns (uint32 domain) {
    domain = chainIdToCCTPDomain[_chain];
    // Wormhole ChainID 2 = Ethereum https://wormhole.com/docs/products/reference/chain-ids/
    // Only allow CCTP Domain 0 for Ethereum https://developers.circle.com/cctp/cctp-supported-blockchains#cctp-v2-supported-domains
    require(domain != 0 || _chain == 2, "CCTP domain not configured");
}
MEDIUM severity: Hard-coding 0 max fee with fast finality is incompatible as this combination commonly has minimum fees of 1. **Description:** Circle's [CCTPv2 Technical Guide](https://developers.circle.com/cctp/technical-guide) provides the following relevant information:
* Messages with `minFinalityThreshold` of 1000 or lower are considered Fast messages
* Messages with `minFinalityThreshold` of 2000 are considered...

Vulnerable code:
$ wget --quiet \
  --method GET \
  --header 'Content-Type: application/json' \
  --output-document \
  - https://iris-api-sandbox.circle.com/v2/burn/USDC/fees/0/1
[{"finalityThreshold":1000,"minimumFee":1},{"finalityThreshold":2000,"minimumFee":0}]%

$ wget --quiet \
  --method GET \
  --header 'Content-Type: application/json' \
  --output-document \
  - https://iris-api-sandbox.circle.com/v2/burn/USDC/fees/0/5
[{"finalityThreshold":1000,"minimumFee":1},{"finalityThreshold":2000,"minimumFee":0}]%

circleTokenMessenger.depositForBurn(
    _amount,
    getCCTPDomain(_targetChain),
    targetAddressBytes32,        // mintRecipient on destination
    USDC,          // burnToken
    destinationCallerBytes32,        // destinationCaller (restrict who can mint)
    0,   // @audit maximum fee
    1000 // @audit fast finality
);
MEDIUM severity: Investors using smart contract wallets may have their destination chain tokens issued to an address they don't control. **Description:** `SecuritizeBridge::bridgeDSTokens` encodes `_msgSender()` in the payload message to be destination address for bridged tokens delivered on the destination chain; this is expected to represent the investor's wallet address:

`SecuritizeBridge::receiveWormholeMessages` executed on...

Vulnerable code:
// Send Relayer message
        wormholeRelayer.sendPayloadToEvm{value: msg.value} (
            targetChain,
            targetAddress,
            abi.encode(
                investorDetail.investorId,
                value,
                _msgSender(), // @audit destination address of bridged tokens
                investorDetail.country,
                investorDetail.attributeValues,
                investorDetail.attributeExpirations
            ), // payload
            0, // no receiver value needed since we"re just passing a message
            gasLimit,
            whChainId,
            _msgSender()
        );

address[] memory investorWallets = new address[](1);
        investorWallets[0] = investorWallet;

        // @audit assumes investor controls same wallet address
        // on destination chain - not necessarily true for smart contract wallets
        registryService.updateInvestor(investorId, investorId, country, investorWallets, attributeIds, attributeValues, attributeExpirations);
        dsToken.issueTokens(investorWallet, value);
LOW severity: Don't use `transfer` to send ETH. **Description:** Using `transfer` to send ETH hasn't been recommended since the Istanbul hard fork in December 2019 which increased the gas cost of some operations; `transfer` hard-codes gas to 2300 which can cause receiving functions to revert hence is not future-proof.

The [recommended way to...

Vulnerable code:
bridge/USDCBridgeV2.sol
202:        _to.transfer(amount);
HIGH severity: Use `SafeER` approval and transfer functions instead of standard IERfunctions. **Description:** Use [SafeERC20::forceApprove](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L105-L110) and `safeTransfer` functions instead of standard IERC20 functions:

**Securitize:** Fixed in commit...

Vulnerable code:
wormhole/WormholeCCTPUpgradeable.sol
121:        IERC20(USDC).approve(address(circleTokenMessenger), amount);

bridge/USDCBridgeV2.sol
150:        IERC20(USDC).transferFrom(_msgSender(), address(this), _amount);
264:        IERC20(USDC).approve(address(circleTokenMessenger), _amount);
LOW severity: Misleading event emission in `USDCBridgeV2::addBridgeCaller, removeBridgeCaller` when role was not granted or revoked. **Description:** `AccessControlUpgradeable::_grantRole,_revokeRole` [return](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/AccessControlUpgradeable.sol#L204-L230) `bool` to indicate whether the role has been granted or revoked.

But...

Vulnerable code:
function addBridgeCaller(address _account) external override addressNotZero(_account) onlyRole(DEFAULT_ADMIN_ROLE) {
-       grantRole(BRIDGE_CALLER, _account);
-       emit BridgeCallerAdded(_account);
+       if(_grantRole(BRIDGE_CALLER, _account)) emit BridgeCallerAdded(_account);
    }

    function removeBridgeCaller(address _account) external override addressNotZero(_account) onlyRole(DEFAULT_ADMIN_ROLE) {
-       revokeRole(BRIDGE_CALLER, _account);
-       emit BridgeCallerRemoved(_account);
+       if(_revokeRole(BRIDGE_CALLER, _account)) emit BridgeCallerRemoved(_account);
    }
HIGH severity: Cache storage to prevent identical storage reads. **Description:** Reading from storage is expensive; cache storage to prevent identical storage reads:
* `contracts/wormhole/WormholeCCTPUpgradeable.sol`

* `contracts/bridge/SecuritizeBridge.sol`

* `contracts/bridge/USDCBridgeV2.sol`

**Securitize:** Acknowledged.

Vulnerable code:
// cache `USDC` in `redeemUSDC`
65:        uint256 beforeBalance = IERC20(USDC).balanceOf(address(this));
67:        return IERC20(USDC).balanceOf(address(this)) - beforeBalance;

// cache `dsToken` in `bridgeDSTokens`
73:        require(dsToken.balanceOf(_msgSender()) >= value, "Not enough balance in source chain to bridge");
88:        dsToken.burn(_msgSender(), value, BRIDGE_REASON);
108:        emit DSTokenBridgeSend(targetChain, address(dsToken), _msgSender(), value);

// cache `dsToken` in `receiveWormholeMessages`
144:        dsToken.issueTokens(investorWallet, value);
146:        emit DSTokenBridgeReceive(sourceChain, address(dsToken), investorWallet, value);

// cache `USDC` in `sendUSDCCrossChainDeposit`
143:        if (IERC20(USDC).balanceOf(_msgSender()) < _amount) {
150:        IERC20(USDC).transferFrom(_msgSender(), address(this), _amount);
// also change `_transferUSDC` to take cached `USDC` as parameter to save
// 2 storage reads inside `_transferUSDC`;
// cache `USDC, `circleTokenMessenger` in `_transferUSDC`
264:        IERC20(USDC).approve(address(circleTokenMessenger), _amount);
268:        circleTokenMessenger.depositForBurn(
272:            USDC,          // burnToken
LOW severity: Refactor `SecuritizeBridge::bridgeDSTokens` and `quoteBridge` to use `internal` function saves 2 storage reads per bridging transaction. **Description:** `SecuritizeBridge::bridgeDSTokens`:
* L71 calls `quoteBridge` which reads `wormholeRelayer` and `gasLimit` from storage
* L91 calls `wormholeRelayer.sendPayloadToEvm` which reads `wormholeRelayer` from storage again
* L108 reads `gasLimit` from storage again

Storage reads are...

Vulnerable code:
// new internal function
    function _quoteBridge(IWormholeRelayer relayer, uint256 _gasLimit, uint16 targetChain) internal view returns (uint256 cost) {
        (cost, ) = relayer.quoteEVMDeliveryPrice(targetChain, 0, _gasLimit);
    }

// modify `quoteBridge` to use new internal function
    function quoteBridge(uint16 targetChain) public override view returns (uint256 cost) {
        (cost, ) = _quoteBridge(wormholeRelayer, gasLimit, targetChain);
    }

// in `bridgeDSTokens` to cache `wormholeRelayer` and `gasLimit`
// then pass them to `_quoteBridge` and use them at L91 & L108