ConcentratedLiquidityPoolManager's incentives can be stolen

Handle cmichel Vulnerability details The ConcentratedLiquidityPoolManager keeps all tokens for all incentives in the same contract. The reclaimIncentive function does not reduce the incentive.rewardsUnclaimed field and thus one can reclaim tokens several times. This allows anyone to steal all...

Override existing market

Handle pauliax Vulnerability details Impact When calling function createMarket an admin can override an existing market by specifying the same underlying and maturity: marketsum = Marketc, zctAddr, vAddr; it does not check if the market for these parameters already exists, so technically it is...

ERC20 return values not checked

Handle cmichel Vulnerability details The ERC20.transfer and ERC20.transferFrom functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead. This value is not checked throughout the...

Previously created markets can be overwritten

Handle 0xRajeev Vulnerability details Impact The createMarket function allows accidental overwriting of previously created markets for the same combination of underlying and maturity timestamp u, m because there is no zero-address check to see if a previously created market exists for that...

range fee growth underflow

Handle broccoli Vulnerability details range fee growth underflow Impact The function RangeFeeGrowth ConcentratedLiquidityPool.solL601-L633 would revert the transaction in some cases. When a pool cross a tick, it only updates either feeGrowthOutside0 or feeGrowthOutside1. Ticks.solL23-L53...

Incentive should check that it hasn't started yet

Handle cmichel Vulnerability details The ConcentratedLiquidityPoolManager.addIncentive function can add an incentive that already has a non-zero incentive.secondsClaimed. Impact Rewards will be wrong. Recommended Mitigation Steps Add a check: requireincentive.secondsClaimed == 0, "!secondsClaimed...

Admin can abuse onlySwivel

Handle 0xsanson Vulnerability details Impact In MarketPlace.sol, admin can change the swivel address at any time. A malicious admin can abuse this by setting swivel to his personal address. Then they can call some functions like p2pZcTokenExchange and p2pVaultExchange to steal zc-tokens and...

Wrong reward calculation

Handle 0xsanson Vulnerability details Impact In ConcentratedLiquidityPoolManager, an user can claimReward of a subscribed position. In order to compute the correct amount, secondsUnclaimed needs to be calculated, but it's implemented incorrectly: uint256 secondsUnclaimed = maxTime -...

ConcentratedLiquidityPoolManager: incorrect calculation of secondsUnclaimed

Handle hickuphh3 Vulnerability details Impact The subtraction of secondsClaimed should be performed after the left shifting of bits in uint256 secondsUnclaimed = maxTime - incentive.startTime 128 - incentive.secondsClaimed; Recommended Mitigation Steps uint256 secondsUnclaimed = maxTime -...

Prevent creating the same market twice

Handle gpersoon Vulnerability details Impact The function createMarket of MarketPlace.sol doesn't check if the market already exists. So it could accidentally deploy a market with has the same maturity timestamp twice and overwrite the previous values of the market. The previously deployed market...

burn do not burn reserve

Handle broccoli Vulnerability details burn do not burn reserve Impact ConcentratedLiquidityPool.solL263-L266 The dev mistakenly write: reserve0 -= uint128amount0fees; reserve1 -= uint128amount1fees; It should be reserve0 -= uint128amount0fees; reserve1 -= uint128amount1fees; Other users can't min...

Users cannot receive rewards from ConcentratedLiquidityPoolManager if their liquidity is too large

Handle broccoli Vulnerability details Impact There could be an integer underflow error when the reward of an incentive is claimed, forcing users to wait for a sufficient period or reduce their liquidity to claim the rewards. Proof of Concept The unclaimed reward that a user could claim is...

Unclaimed rewards are not deducted when reclaiming an incentive

Handle broccoli Vulnerability details Impact The reclaimIncentive function of ConcentratedLiquidityPoolManager does not update the rewardsUnclaimed variable after some rewards are reclaimed. Thus, an attacker could add an incentive with a corresponding token, such as DAI, and reclaim the incentiv...

Wrong reserve decrease in burn

Handle 0xsanson Vulnerability details Impact When burning a liquidity position the reserves should be decreased by the tokens' amount that leaves the contract. However in ConcentratedLiquidityPool's burn they are decreased only by the fees. Proof of Concept Tools Used editor Recommended Mitigatio...

Admin is a single-point of failure without any mitigations

Handle 0xRajeev Vulnerability details Impact Admin role has absolute power across Swivel, Marketplace and VaultTracker contracts with several onlyOwner functions. There is no ability to change admin to a new address or renounce it which is helpful for lost/compromised admin keys or to delegate...

Missing events/timelocks for owner/admin only functions that change critical parameters

Handle defsec Vulnerability details Impact Owner/admin only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them...

ConcentratedLiquidityPoolManager.sol#reclaimIncentive() Unsafe implementation allows malicious users to steal yield

Handle WatchPug Vulnerability details The reclaimIncentive function allows users who added incentives before to withdraw unclaimed rewards. However, the current implementation did not manage the state correctly, incentive.rewardsUnclaimed is not updated after the token transfer, which allows the...

Cannot claim reward

Handle cmichel Vulnerability details The ConcentratedLiquidityPoolManager.claimReward requires stake.initialized but it is never set. It also performs a strange computation as 128 - incentive.secondsClaimed which will almost always underflow and revert the transaction. Impact One cannot claim...

Infinite mint by transferring nTokens to self

Handle cmichel Vulnerability details The VaultTracker.transferNotionalFrom function first stores the vaults data for f and t in a memory variable not using storage pointers. An attacker can transfer tokens to themself using f = t and mint free tokens for themself. The vaultsf storage is set first...

Market can be overwritten

Handle cmichel Vulnerability details The MarketPlace.createMarket function does not check if a market already exists for the underlying, maturity pair. Impact Existing markets can be overwritten and the old zt and n tokens will lose their association to the market. Recommended Mitigation Steps Ad...

Safe transfers of tokens

Handle pauliax Vulnerability details Impact Not every ERC20 token returns true on transfer success. To support different tokens, the current best practice is to use SafeERC20 safeTransfer, safeTransferFrom, etc: Similarly, some ERC20 tokens like USDT require resetting the approval to 0 first befo...

ConcentratedLiquidityPool.burn() Wrong implementation

Handle WatchPug Vulnerability details The reserves should be updated once LP tokens are burned to match the actual total bento shares hold by the pool. However, the current implementation only updated reserves with the fees subtracted. Makes the reserve0 and reserve1 smaller than the current...

ConcentratedLiquidityPoolHelper: getTickState() might run out of gas

Handle hickuphh3 Vulnerability details Impact getTickState attempts to fetch the state of all inserted ticks including MINTICK and MAXTICK of a pool. Depending on the tick spacing, this function may run out of gas. Recommended Mitigation Steps Have a starting index parameter to start the iteratio...

Swivel.sol transfer of tokens in initiateZcTokenFillingZcTokenExit and initiateVaultFillingVaultExit can fail without reverting causing account problems

Handle GalloDaSballo Vulnerability details Impact initiateZcTokenFillingZcTokenExit and initiateVaultFillingVaultExit in Swivel.sol are using transferFrom This function call can fail meaning the tokens are not transferred without causing a revert This would break the accounting in the protocol I...

Wrong inequality when adding/removing liquidity in current price range

Handle cmichel Vulnerability details The ConcentratedLiquidityPool.mint/burn functions add/remove liquidity when priceLower currentPrice && currentPrice priceUpper. Shouldn't it also be changed if priceLower == currentPrice? Impact Pools that mint/burn liquidity at a time where the currentPrice i...

ConcentratedLiquidityPosition.sol#burn() Wrong implementation allows attackers to steal yield

Handle WatchPug Vulnerability details When a user calls ConcentratedLiquidityPosition.solburn to burn their liquidity, it calls ConcentratedLiquidityPool.solburn - updatePosition: The updatePosition function will return amount0fees and amount1fees of the whole position with the lower and upper ti...

uToken's approve() Function is Susceptible to Frontrunning Attacks

Handle leastwood Vulnerability details Impact Order makers will call the approve function to facilitate transactions between order takers and makers. If a taker sees that a maker is seeking to reduce its order exposure by reducing Swivel.sol's allowance, a taker can frontrun this transaction to...

ConcentratedLiquidityPoolManager: incentive is incorrectly accessed

Handle hickuphh3 Vulnerability details Impact The positionId is used to retrieve the incentive info instead of incentiveId. Incentive storage / memory incentive = incentivesposition.poolpositionId; Recommended Mitigation Steps Incentive storage / memory incentive =...

Incorrect usage of typecasting in _getAmountsForLiquidity lets an attacker steal funds from the pool

Handle broccoli Vulnerability details Impact The getAmountsForLiquidity function of ConcentratedLiquidityPool explicitly converts the result of DyDxMath.getDy and DyDxMath.getDx from type uint256 to type uint128. The explicit casting without checking whether the integer exceeds the maximum number...

uToken ERC20 approve method missing return value check #L109

Handle defsec Vulnerability details Impact The initiateVaultFillingZcTokenInitiate function performs an ERC20.approve call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead. Proof of Concept 1. Navigate to "" 2...

transferNotionalFrom doesn't check from != to

Handle gpersoon Vulnerability details Impact The function transferNotionalFrom of VaultTracker.sol uses temporary variables to store the balances. If the "from" and "to" address are the same then the balance of "from" is overwritten by the balance of "to". This means the balance of "from" and "to...

ConcentratedLiquidityPoolHelper.getTickState reverts

Handle cmichel Vulnerability details ConcentratedLiquidityPoolHelper.getTickState allocates only tickCount elements for the ticks array. But the while loop iterates over all ticks of the pool which can potentially become very large, much larger than any tickCount could iterate in a reasonable tim...

Unsafe cast in ConcentratedLiquidityPool burn leads to attack

Handle cmichel Vulnerability details The ConcentratedLiquidityPool.burn function performs an unsafe cast of a uint128 type to a signed integer. uint256 amount0fees, uint256 amount1fees = updatePositionmsg.sender, lower, upper, -int128amount; Note that amount is chosen by the caller and when...

Unsafe handling of underlying tokens

Handle 0xsanson Vulnerability details Impact Not every ERC20 token follows OpenZeppelin's recommendation. It's possible inside ERC20 standard that a transferFrom doesn't revert upon failure but returns false. The code doesn't check these return values. For example uToken.transferFrommsg.sender,...

Missing event & timelock for critical onlyAdmin functions

Handle 0xRajeev Vulnerability details Impact onlyAdmin functions that change critical contract parameters/addresses/state should emit events and consider adding timelocks so that users and other privileged roles can detect upcoming changes by offchain monitoring of events and have the time to rea...

Burning does not update reserves

Handle cmichel Vulnerability details The ConcentratedLiquidityPool.burn function sends out amount0/amount1 tokens but only updates the reserves by decreasing it by the fees of these amounts. unchecked // @audit decreases by fees only, not by amount0/amount1 reserve0 -= uint128amount0fees; reserve...

ConcentratedLiquidityPoolManager uses wrong index for incentive

Handle cmichel Vulnerability details The ConcentratedLiquidityPoolManager uses the positionId as an index for incentivespoolpositionId when it should be incentiveId instead: // @audit should be Incentive memory incentive = incentivespoolincentiveId; Incentive memory incentive =...

Integer underflow error in claimReward of ConcentratedLiquidityPoolManager

Handle broccoli Vulnerability details Impact The claimReward function of ConcentratedLiquidityPoolManager calculates the secondsUnclaimed variable using a formula with an unclear intention: uint256 secondsUnclaimed = maxTime - incentive.startTime 128 - incentive.secondsClaimed; This formula cause...

Wrong subtraction from reserve in burn

Handle pauliax Vulnerability details Impact function burn should subtract amount0 and amount1, not only fees from reserve0 and reserve1 here as whole amounts are withdrawn: reserve0 -= uint128amount0fees; reserve1 -= uint128amount1fees; Recommended Mitigation Steps reserve0 -= uint128amount0;...

Wrong indexes: positionId vs incentiveId

Handle 0xsanson Vulnerability details Impact In multiple functions in ConcentratedLiquidityPoolManager, the index positionId is used instead of the correct incentiveId when dealing with the incentives mapping. Of course the issue is that incentives cannot be used, or in some cases only by lucky...

Markets Do Not Operate Correctly on Non-Standard Tokens

Handle leastwood Vulnerability details Impact There are a number of functions which interact with uToken in Swivel.sol but do not check the return value of the associated transfer, approve or transferFrom call. As a result, if a market is created using a non-standard token, a failed call will be...

ConcentratedLiquidityPoolManager: reclaimIncentive() does not decrement rewardsUnclaimed

Handle hickuphh3 Vulnerability details Impact reclaimIncentive withdraws any unclaimed rewards to the incentive owner. While there is a check to prevent re-claiming of rewards requireincentive.rewardsUnclaimed = amount, "ALREADYCLAIMED"; it is ineffective because incentive.rewardsUnclaimed is not...

ConcentratedLiquidityPool: incorrect feeGrowthGlobal accounting when crossing ticks

Handle hickuphh3 Vulnerability details Impact Swap fees are taken from the output. Hence, if swapping token0 for token1 zeroForOne is true, then fees are taken in token1. We see this to be the case in the initialization of feeGrowthGlobal in the swap cache feeGrowthGlobal = zeroForOne ?...

Swivel: Taker is charged fees twice in exitVaultFillingVaultInitiate

Handle itsmeSTYJ Vulnerability details Impact Taker is charged fees twice in exitVaultFillingVaultInitiate . Maker is transferring less than premiumFilled to taker and then taker is expected to pay fees i.e. taker's net balance is premiumFilled - 2fee Recommended Mitigation Steps function...

Erc20.transferFrom() return value is ignored

Handle pants Vulnerability details According to the ERC-20 Token Standard, the function transferFrom returns false on failure. However, the return value of Erc20.transferFrom is ignored 11 times: 1. In Swivel.initiateVaultFillingZcTokenInitiate, line 103. 2. In...

Erc20.approve() return value is ignored

Handle pants Vulnerability details According to the ERC-20 Token Standard, the function approve returns false on failure. However, the return value of Erc20.approve is ignored 3 times: 1. In Swivel.initiateVaultFillingZcTokenInitiate, line 109. 2. In Swivel.initiateZcTokenFillingVaultInitiate, li...

MarketPlace.sol: createMarket should check if market already exists before creating

Handle itsmeSTYJ Vulnerability details Impact createMarket is a privileged function that can only be called by an admin but that doesn't necessarily mean that it is not susceptible to mistakes. Furthermore, it is a function that is called somewhat often so following murphy's law - anything can go...

Erc20.transfer() return value is ignored

Handle pants Vulnerability details According to the ERC-20 Token Standard, the function transfer returns false on failure. However, the return value of Erc20.transfer is ignored 8 times: 1. In Swivel.exitVaultFillingZcTokenExit, line 313. 2. In Swivel.exitVaultFillingZcTokenExit, line 315. 3. In...

Erc20.transferFrom() doesn't work on non-standard compliant tokens like USDT

Handle pants Vulnerability details Non-standard compliant tokens like USDT don't return a return value on transferFrom. The function Erc20.transferFrom reverts if the token doesn't return a return value, as it is defined to always return a boolean. Impact When using any non-standard compliant tok...

Erc20.transfer() doesn't work on non-standard compliant tokens like USDT

Handle pants Vulnerability details Non-standard compliant tokens like USDT don't return a return value on transfer. The function Erc20.transfer reverts if the token doesn't return a return value, as it is defined to always return a boolean. Impact When using any non-standard compliant token like...