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

Two-step change of a swivel address

Handle pauliax Vulnerability details Impact MarketPlace allows an admin to change swivel to a different address. This function has no validations, even a simple check for zero-address is missing, and there is no validation of the new address being correct. If the admin accidentally uses an invali...

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

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

Manipulation of secondsClaimed

Handle pauliax Vulnerability details Impact function addIncentive does not verify secondsClaimed so any arbitrary value can be set initially by the creator and it may break calculations. Recommended Mitigation Steps Consider either requiring that incentive.secondsClaimed is 0 or manually resettin...

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

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

return value of 0 from ecrecover not checked

Handle gpersoon Vulnerability details Impact The solidity function ecrecover is used, however the error result of 0 is not checked for. See documentation: "recover the address associated with the public key from elliptic curve signature or return zero on error. " Now you can supply invalid input...

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

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

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

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

Incorrect usage of typecasting in burn lets an attacker corrupt the pool state

Handle broccoli Vulnerability details Impact In the burn function of ConcentratedLiquidityPool, when calling updatePosition, the amount of liquidity to burn is explicitly converted from uint128 to int128, which could result in a positive integer if amount is larger than 1 127 and less than 1 128...

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

ConcentratedLiquidityPoolManager.sol#claimReward() and reclaimIncentive() will fail when incentive.token is token0 or token1

Handle WatchPug Vulnerability details In ConcentratedLiquidityPosition.collect, balances of token0 and token1 in bento will be used to pay the fees. uint256 balance0 = bento.balanceOftoken0, addressthis; uint256 balance1 = bento.balanceOftoken1, addressthis; if balance0 newBalance0 token0amount =...

TridentNFT.permit should always check recoveredAddress != 0

Handle cmichel Vulnerability details The TridentNFT.permit function ignores the recoveredAddress != 0 check if isApprovedForAllownerrecoveredAddress is true. Impact If a user accidentally set the zero address as the operator, tokens can be stolen by anyone as a wrong signature yield...

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

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

Missing zero address check for ecrecover will validate invalid signatures

Handle 0xRajeev Vulnerability details Impact Solidity’s ecrecover returns 0 if signature is invalid. The Sig.sol library does not perform zero address check on ecrecover’s return value and returns it as-is. The validOrderHash function in Swivel which uses Sig.recover compares its return value...

fee possibly send in the wrong way in exitVaultFillingVaultInitiate

Handle gpersoon Vulnerability details Impact The functions exitZcTokenFillingZcTokenInitiate and exitVaultFillingVaultInitiate are kind of similar, however the processing of the fee is different. exitZcTokenFillingZcTokenInitiate send the fee from o.maker exitVaultFillingVaultInitiate sends the f...

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

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

ConcentratedLiquidityPool: secondsPerLiquidity should be modified whenever pool liquidity changes

Handle hickuphh3 Vulnerability details Impact secondsPerLiquidity is updated as such: secondsPerLiquidity += uint160diff 128 / liquidity; where diff = timestamp - uint256lastObservation. Hence, whenever liquidity changes, secondsPerLiquidity should be updated prior to the change. In particular,...

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

functions permit and permitAll can be tricked by an invalid signature

Handle pauliax Vulnerability details Impact When the signature is not valid, ecrecover returns empty 0x0 address. There is a potential check against that: require recoveredAddress != address0 && recoveredAddress == owner || isApprovedForAllownerrecoveredAddress, "INVALIDPERMITSIGNATURE" ; However...

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

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

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

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

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

Overflow in the mint function of ConcentratedLiquidityPool causes LPs' funds to be stolen

Handle broccoli Vulnerability details Impact Similar to a previous finding in the IndexPool contract, the mint function of ConcentratedLiquidityPool allows integer overflows when checking the balance is larger or equal to the received amount of token plus the reserve. As a result, the attacker...

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

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

exitZcTokenFillingZcTokenInitiate in Swivel.sol, token transfer may fail without function reverting

Handle GalloDaSballo Vulnerability details Impact exitZcTokenFillingZcTokenInitiate in Swivel.sol uses transferFrom this function can failmeaning tokens are not transferred,without causing a revert. This can break the accounting of the protocol The reason why this can happen is that certain ERC20...

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 usage of positionId in ConcentratedLiquidityPoolManager

Handle broccoli Vulnerability details Impact In the subscribe function of ConcentratedLiquidityPoolManager, the incentive to subscribed is determined as follows: Incentive memory incentive = incentivespoolpositionId; However, positionId should be incentiveId, a counter that increases by one...

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

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

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

onlyAdmin Role May Unintentionally Cause Issues in the Underlying Protocol

Handle leastwood Vulnerability details Impact The onlyAdmin role points to an EOA account managed by the Swivel team. Withdrawals from the Swivel protocol have to be scheduled by the onlyAdmin role, ensuring that users have sufficient time to withdraw, if required. The setFee function in Swivel.s...

Incentives for different pools should differ by a large factor

Handle tensors Vulnerability details I'm adding this as an issue because I didn't see it mentioned anywhere in the codebase, and I think its a fair point that relates to how the protocol gives out rewards to users. As I understand , the point of staking is to provide users with additional...

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

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

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