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

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

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

HybridPool's flashSwap gives the total fee to barFeeTo

Handle 0xsanson Vulnerability details Impact In HybridPool's flashSwap function there's a transfer to barFeeTo transfertokenIn, fee, barFeeTo, false; Here fee = amountIn swapFee / MAXFEE is the total swap fee. However it should transfer out only a fraction of it barFee/MAXFEE otherwise liquidity...

UniswapV3Helper: getSqrtPriceX96() doesn't work for tokens with non-18 decimals

Handle hickuphh3 Vulnerability details Impact The getSqrtPriceX96 will return incorrect values for pairs comprising of non-18 decimals. This affects the amounts calculated for a position. Proof of Concept Let us take the ETH-WBTC pair as an example. Note that WBTC has 8 decimals, and is an active...

Missing timelock for critical contract setters of privileged roles

Handle 0xRajeev Vulnerability details Impact Setter functions for critical protocol parameters accessible only by privileged roles e.g. onlyOwner should consider adding timelocks so that users and other privileged roles in the case of a multiSig can detect upcoming changes and have the time to...

Auction.sol#settleAuction() Mishandling bounty state could potentially disrupt settleAuction()

Handle WatchPug Vulnerability details function withdrawBountyuint256 memory bountyIds internal // withdraw bounties for uint256 i = 0; i bountyIds.length; i++ Bounty memory bounty = bountiesbountyIdsi; requirebounty.active; IERC20bounty.token.transfermsg.sender, bounty.amount; bounty.active =...

Use mutex lock on VaultHelper.sol

Handle tensors Vulnerability details Impact I strongly recommend adding a nonreentrant modifier on the functions within VaultHelper.sol The contract makes a bunch of unsafe external calls to the user submitted addresses vault and gauge. Also, add some checks to make sure vault and gauge are...

PostAuctionLauncher can be manipulated by a caller other than the owner

Handle tensors Vulnerability details Impact A comment in the code asks whether it is safe to allow anyone to call PostAuctionLauncher, finalize. In the case that an attacker can get even a few wei of the auction token, it is not safe. Suppose an attacker somehow gets a small amount of the auction...

Controller.withdrawAll sets wrong vault balance

Handle cmichel Vulnerability details The Controller.withdrawAll decreases the vault balance by amount, the want token amount that has been withdrawn from the strategy and transferred to the vault. Note that amount gets overwritten in the convert != address0 branch and is a convert token value...

The function addToken does not check if the token was already added

Handle hrkrshnn Vulnerability details addToken does not check if the token was already added The function addToken does not check if the token was already present. function addToken address vault, address token external override notHalted onlyStrategist requireallowedTokenstoken, "!allowedTokens"...

VaultHelper deposits don't work with fee-on transfer tokens

Handle cmichel Vulnerability details There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer or transferFrom. Others are rebasing tokens that increase in value over time like...

Vault: Withdrawal amount isn't un-normalized

Handle hickuphh3 Vulnerability details Impact In withdraw, the withdrawal amount is the proportion of the normalized amounts of all the tokens in the vault and its strategies. However, this amount isn't un-normalized to the output token's decimals, thus leading to an erroneous token amount being...

Vault.balance() mixes normalized and standard amounts

Handle cmichel Vulnerability details The Vault.balance function uses the balanceOfThis function which scales "normalizes" all balances to 18 decimals. for uint8 i; i tokens.length; i++ address token = tokensi; // everything is padded to 18 decimals balance = balance.addnormalizeDecimalstoken,...

harvestNextStrategy never executes because lastCalled is 0 and stays 0

Handle gpersoon Vulnerability details Impact The function harvestNextStrategy of Harvester.sol checks canHarvest to make sure it can harvest. Initially strategy.lastCalled will be 0 so canHarvest will return false. Thus the require in harvestNextStrategy fails And it never reaches the point where...

Arithmetic Error - manualRebalance function has multiple arithmetic bugs

Handle tabish Vulnerability details In short there are 2 errors in manualRebalance function : a ratio currentLockRatio has been compared to balance newLockRatio and at another point in the same function subtracted . 1 Expanding on the first one - checking newLockRatio Solution: Instead the if...

Allowance checks not correctly implemented

Handle cmichel Vulnerability details Vulnerability Details The nTokenAction implements two token approvals, the nTokenWhitelist which is always used first, and the nTokenAllowance which is checked second. If the nTokenWhitelist does not have enough allowance for the transfer, the transaction fail...

Rewards squatting - setting rewards in different ERC20 tokens opens various economic attacks.

Handle moose-code Vulnerability details Impact Users have essentially have an option to either claim currently earned reward amounts on future rewards tokens, or the current rewards token. Although stated on line 84, it does not take into account the implications and lock in this contract will ha...

initializeMarket uses wrong market index for synthetic

Handle cmichel Vulnerability details The LongShort.initializeMarket function accepts a marketIndex parameter to identify which market should be initialized. However, this index is not used everywhere, when calling IStakerstaker.addNewStakingFund the latestMarket variable is used. In the...

Incorrect balance computed in getUsersConfirmedButNotSettledSynthBalance()

Handle hack3r-0m Vulnerability details Consider the following state: longsynthbalace = 300; shortsynthbalace = 200; marketUpdateIndex1 = x; userNextPricecurrentUpdateIndex = 0; userNextPricesyntheticTokentoShiftAwayFrommarketSide1true = 0; batchedamountSyntheticTokentoShiftAwayFrommarketSide1true...

Synths minted to the wrong market when initializing

Handle 0xImpostor Vulnerability details Impact Synthetix tokens are not minted to the correct market index since the creation of the synth market and the initialization are 2 separate steps. Proof of Concept 1. Create 2 synth market without initializing them 2. Call initializeMarket twice 3. Synt...

2 variables not indexed by marketIndex

Handle gpersoon Vulnerability details Impact In the token contract: batchedstakerNextTokenShiftIndex is indexed by marketIndex, so it can have separate or the same values for each different marketIndex. stakerTokenShiftIndextolongShortMarketPriceSnapshotIndexmapping and...

Staker.sol: Wrong values returned in edge cases of _calculateFloatPerSecond()

Handle hickuphh3 Vulnerability details Impact In calculateFloatPerSecond, the edge cases where full rewards go to either the long or short token returns return 1e18 k longPrice, 0; and return 0, 1e18 k shortPrice; respectively. This is however 1e18 times too large. We can verify this by checking...

transfer blocked by insufficient unallocatedSherX

Handle gpersoon Vulnerability details Impact When doYield is done, it subtracts the "withdrawableamount" from unallocatedSherX. However unallocatedSherX could be lowered by a large payout, possibly being insufficient to subtract "withdrawableamount". If this would be the case, then doYield would...

Possible DoS attack in creating new DAO proposals

Handle shw Vulnerability details Impact The functions of creating new DAO proposals e.g., newActionProposal are permissionless. Anyone can create a new proposal by paying some fees in SPARTA, as long as the previous proposal is closed. Thus, an attacker could then front-run proposals of benign...

Router.zapLiquidity(uint256,address,address) has unchecked transfers

Handle heiho1 Vulnerability details Impact Router.zapLiquidityuint256,address,address has unchecked transfers on lines 65, 67 and 69. Several tokens do not revert in case of failure and return false. If one of these tokens is used in Router, deposit will not revert if the transfer fails, and an...

Pool._addPoolMetrics(uint256) is subject to potential miner manipulation

Handle heiho1 Vulnerability details Impact Pool.addPoolMetricsuint256 on line 334 relies on block.timestamp and is potentially vulnerable to miner manipulation. This could lead to erroneous pool metrics. Proof of Concept Tools Used Slither Recommended Mitigation Steps An external time oracle like...

Synth: cannot change approval once set to max value

Handle cmichel Vulnerability details The Synth.approve function performs a no-op if the allowance is currently set to typeuint256.max. This leads to the issue that approvals cannot be changed anymore once they are set to the max value. Impact Imagine someone approving an operator with the max val...

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 is generally not an issue when the...

DaoVault.withdraw(address,address) potentially subject to timestamp manipulation

Handle heiho1 Vulnerability details Impact DaoVault.withdrawaddress,address uses block.timestamp based comparisons can be affected by miner behavior, leading to withdrawal impacts on the user. Proof of Concept Tools Used Slither Recommended Mitigation Steps An external time oracle like ChainLink...

Router.removeLiquiditySingle(uint256,bool,address) has unchecked transfers

Handle heiho1 Vulnerability details Impact Router.removeLiquiditySingleuint256,bool,address on lines 121, 126, 129 ignores the boolean return on transfers. This is a brittle implementation because it relies on the boolean return value being hard-coded to true. Tokens may return false instead of...

Broken access control leads to protocol functionality freeze

Handle 0xRajeev Vulnerability details Impact The contracts use an access control pattern where the contract deployer is included in the onlyDAO modifier which is used for authorized access to critical functions. Such contracts also include a purgeDeployer function which renounces sets to...

Missing slippage checks

Handle cmichel Vulnerability details The Router and Pool does not implement any slippage checks with comparing the swap / liquidity results with a minimum swap / liquidity value. Impact Users can be frontrun and receive a worse price than expected when they initially submitted the transaction...

TransferHelper does not work with all ERC20 tokens

Handle cmichel Vulnerability details The TransferHelper.safeTransferFrom function will revert if tokens do not return a boolean because the interface ERC20.transfer function it uses indicates that this function always returns a boolean. Some tokens like USDT don't correctly implement the EIP20...

Lending Pair initialize function can be front run.

Handle jonah1005 Vulnerability details Impact LendingPair does not initialize tokenMaster, controller, tokens. A hacker can listen the deployer address and front run the initialize transaction. The initialized contract would look almost exactly the same if the hacker only replace lpTokenMaster wi...

addFunds and execute may send tokens twice

Handle pauliax Vulnerability details Impact Both calls to IFulfillHelper addFunds and execute are wrapped in separate try/catch statements so basically if addFunds succeeds but execute fails or both of these functions fail, the catch will still send assets to the receivingAddress. I think these...

Approval is not reset if the call to IFulfillHelper fails

Handle pauliax Vulnerability details Impact Function fulfill first approves the callTo to transfer an amount of toSend tokens and tries to call IFulfillHelper but if the call fails it transfers these assets directly. However, in such case the approval is not reset so a malicous callTo can pull...

WETH not verified to be one of the pair tokens

Handle gpersoon Vulnerability details Impact The ETH functions of LendingPair don't check that WETH is one of the 2 pairs of the contract and introduces a third token. The third token ETH/WETH falls outside the other checks that the contract is doing. You could accidentally pay ETH/WETH to the...

User prepare can be denied

Handle cmichel Vulnerability details Vulnerability Details Upon observing a prepare transaction, an attacker can frontrun it with the same invariantData but an amount of a single wei. This inserts a value into variantTransactionDatadigest and the original transcation will fail because of the...

Exposure.sortVaultsByDelta can underflow

Handle cmichel Vulnerability details Vulnerability Details The sortVaultsByDelta function performs an unsafe subtraction on two uint256 before casting them to int256. The subtraction can underflow and the cast to int256 can either fail and revert the transaction if greater than typeint256.max, or...

Wrong funding index in settle when no base?

Handle cmichel Vulnerability details The TracerPerpetualSwaps.settle function updates the user's last index to currentGlobalFundingIndex, however a comment states: "// Note: global rates reference the last fully established rate hence the -1, and not the current global rate. User rates reference...

No support for token with decimals > 18

Handle s1m0 Vulnerability details Impact The smart contract doesn't behave correctly if deployed with token that have decimals 18. Proof of Concept The functions tokenToWad and wadToToken revert if the tokenDecimals is 18. These functions are called in critical places like deposit and withdraw...

Bots can frontrun market maker orders and cancel orders.

Handle tensors Vulnerability details Impact Market makers will not be able to have competitive spreads or cancel their orders to update their strategies. It will be difficult for them to provide liquidity on the platform, which will hurt the platform overall. Proof of Concept In traditional finan...

prb-math not audited

Handle gpersoon Vulnerability details Impact The library prb-math documents that it is not audited by a security researcher. This means its more risky to rely on this library. Proof of Concept // The contracts have not been audited by a security researcher. Tools Used Recommended Mitigation Steps...

IdleYieldSource doesn't use mantissa calculations

Handle tensors Vulnerability details Impact Because mantissa calculations are not used in this case to account for decimals, the arithmetic can zero out the number of shares or tokens that should be given. For example, say I deposit 1 token, expecting 1 share in return. On L95, if the...

Initialization functions can be front-run with malicious values

Handle 0xRajeev Vulnerability details Impact Most contracts have public visibility initialization functions that can be front-run, allowing an attacker to incorrectly initialize the contracts. Due to the use of the delegatecall proxy pattern, PrizePool/YieldSourcePrizePool/StakePrizePool,...

Missing call to removeOldBids may affect foreclosure

Handle 0xRajeev Vulnerability details Impact Orderbook.removeBids as commented “///remove bids in closed markets for a given user ///this can reduce the users bidRate and chance to foreclose” removeOldBids is performed currently in Market.newRental and Treasury.deposit to “do some cleaning up, it...

Market-specific pause is not checked for payout

Handle cmichel Vulnerability details Vulnerability Details The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.payout. A paused market contract can therefore still pay out using payArtist, payCardAffiliate, payMarketCreator,...

Market-specific pause is not checked for sponsor

Handle cmichel Vulnerability details Vulnerability Details The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.sponsor. A paused market contract can therefore still deposit as a sponsor using Market.sponsor Impact The market-specif...

Critical uberOwner address changes should be a two-step process

Handle 0xRajeev Vulnerability details Impact As specified, uberOwners of Factory, Orderbook and Treasury have the highest privileges in the system because they can upgrade contracts of market, Nfthub, order book, treasury, token and factory which form the critical components of the protocol. The...