ERC20 can be mistakenly used instead of ERC721

Lines of code Vulnerability details Impact // Attempt to transfer token into this address try IERC721EnumerableUpgradeablesettings.token.transferFrom // @audit could use ERC20 here msg.sender, addressthis, settings.tokenId catch revert TOKENNEEDSTOBEAPPROVEDTOCONTRACT; Both ERC20 and ERC721 has t...

Raffle is fair only if tokenRange is a power of 2.

Lines of code Vulnerability details Impact Based on the contract logic, the raffle works by sampling a value $ x \in \mathbbZ | 0 \leq x \lt 2^256 $ aka uint256 from a uniform distribution provided by Chainlink VRF, then applying modulo tokenRange, generating a new value $ y \in \mathbbZ | 0 \leq...

The owner can swap the proxy implementation with a malicious one

Lines of code Vulnerability details The owner of VRFNFTRandomDrawFactory.sol could swap the current implementation with a malicious one at any moment, without a waiting period. Impact The worse case scenario is one in which the private key of the contract owner gets stolen. In this case the owner...

winnerClaimNFT() does not check if recipient can receive ERC721

Lines of code Vulnerability details Impact In src\VRFNFTRandomDraw.sol winnerClaimNFT, you should use safeTransferFrom to protect against the case that the recipient cannot receive the ERC721 tokens, and thus will lose the tokens. This is more-so the case when we expect that normal users will be...

Use safeTransferFrom instead of transferFrom for ERC721 transfers

Lines of code Vulnerability details Impact In the contract VRFNFTRandomDraw.sol every transfer of ERC721 are done with the transferFrom instead of the recommended safeTransferFrom. This transferFrom does not check whether the receiver is capable of proper handling of NFTs. Proof of Concept If the...

The transferFrom() method is used instead of safeTransferFrom(), which I assume is a gas-saving measure. I however argue that this isn’t recommended because:

Lines of code Vulnerability details Vulnerability Detail OpenZeppelin’s documentation discourages the use of transferFrom; use safeTransferFrom whenever possible The recipient could have logic in the startDraw, fwinnerClaimNFT, lastResortTimelockOwnerClaimNFT, function src/VRFNFTRandomDraw.sol-18...

recoverTimelock can have a value shorter than the winner's deadline to claim

Lines of code Vulnerability details Impact The value of recoverTimelock is checked to be greater than a week and less than a year, but it should never be allowed to be shorter than block.timestamp + settings.drawBufferTime, which is the time given to the winner to claim the NFT. Otherwise it coul...

In consistent parameters settings can break the business logic

Lines of code Vulnerability details Impact The usual business logic of the raffle should be that: If a user wins a raffle, he can always claim the NFT before a redraw can be initialized. However, the settings parameters can be set to inconsistent so that a winner may not be able to claim the NFT...

Wrong update of stoploss in TradingExtension._limitClose()

Lines of code Vulnerability details Impact TradingExtension.limitClose returns a wrong stoploss which is favorable for users and it would be a significant loss for the protocol. Proof of Concept TradingExtension.limitClose is used to set takeprofit/stoploss prices for the pending order and execut...

Pool cannot recover from the emergency mode

Lines of code Vulnerability details Impact Pool cannot recover from the emergency mode. If the admin turn on the emergency mode, the user cannot swap or addLiqudity in the pool any more. Proof of Concept In the Pool Contract, the admin factory owner can set the status of the pool to ermergency:...

Pool creator can manipulate the price whatever they want

Lines of code Vulnerability details Impact Pool creator can manipulate the price whatever they want, user that not aware of this may swap in suboptimal price. Proof of Concept When a pool is created using the factory, the creator needs to supply a few parameters: function createuint256 fee, uint2...

Owner can rug PrePOMarket using re-initialized finalLongPayout.

Lines of code Vulnerability details Description In PrePOMarket.sol, the finalLongPayout represents the finalized value of a single long token in the market. It is settled post ICO / IPO according to predetermined rules. The issue is that this value may be re-initialized as many times as owner...

position's wrong profit and loss or incorrect distribution of funds in contract PrePoMarket because it doesn't consider expiryTime for setting the value of finalLongPayout and calculating redeem amounts based on positions and Markets never expire

Lines of code Vulnerability details Impact according to the docs: "The expiry date of the market. If a market has not settled by its expiry date, it will automatically settle at the lower bound of its Valuation Range." but Contract PrePOMarket doesn't consider expiryTime in its logic and...

Unlimited Global & User Withdrawal right after previous period ends and new period begins

Lines of code Vulnerability details Impact Checks for Global and User Withdraw Limit Per Period are missing for the first withdrawal request right AFTER period length expires and a new period begins. First withdrawal request amount after period length expires can be way higher than...

depositAndTrade::exactInputSingleParams the possible return values not checked

Lines of code Vulnerability details Impact The return values ​​from collateral.deposit and collateral.transferFrom are not used. This means that the values ​​returned by those functions will not be stored or used in the code. The swapRouter.exactInputSingle function also likely returns no value, ...

Wrong variable is used for the first parameter of depositHook.hook()

Lines of code Vulnerability details Impact Recipient is treated wrongly as sender. Proof of Concept As written here See DepositHook.solL43-L52, the first accepted parameter should be the sender. However, recipient See Collateral.solL53 is passed in here. Tools Used Manual Recommended Mitigation...

Irrelevant error message from PrePOMarket.redeem() worries users

Lines of code Vulnerability details Impact Users may receive error messages fee = 0 whenever making a redemption from PrePOMarket contract if the redemption amout is not enough. The error message is irrelevant to users. It's related to the protocol processing the redemption. If a user receives...

userToDeposits is always increasing, can eventually reach cap

Lines of code Vulnerability details Impact In contract DepositRecord when a deposit is recorded through function recordDeposit: function recordDepositaddress sender, uint256 amount external override onlyAllowedHooks requireamount + globalNetDepositAmount amount globalNetDepositAmount -= amount;...

Use wrong reserve values in Pool.addLiquidity()

Lines of code Vulnerability details Impact When doing Pool.addLiquidity, it would call bin.addLiquidity to add liquidity to the bins. And the calculation in bin.addLiquidity should be based on the amount of new tokens and the bin’s existing reserves. However, Pool.addLiquidity uses temp.deltaA an...

Merged status is not updated and bins are not reset.

Lines of code Vulnerability details Impact Bins are not removed while removing liquidity and this can lead to serious problems. Proof of Concept While removing liquidity, if the target bin is a merged one mergeId!=0, the protocol does further calculations regarding mergeBinBalance. // Bin.sol 126...

Overflow in BinMap can break pool

Lines of code Vulnerability details Impact The BinMap library performs multiplication on int32 values that can potentially overflow and cause the corresponding function calls to revert. The functions in question are used by essential Pool methods such as Pool.addLiquidity or Pool.swap and an...

Aggregated reserve amounts should be used instead of the first valid tick liquidity

Lines of code Vulnerability details Impact Liquidity can be biased on a specific side quote vs base and it is even possible a liquidity provider gets more LP tokens. Proof of Concept According to the PDF document provided, the number of LP tokens newSupply is calculated using the Table 1 as below...

Pool prices can be greatly skewed and exploited

Lines of code Vulnerability details Impact Token prices in the pool can be manipulated and set to values with great deviation from its originally intended ratio right after the pool has been created. With all key parameters already initialized at the constructor, liquidity in the pool is deemed...

user can loose ETH when using Router::multicall

Lines of code Vulnerability details Impact When using Router multicall function user must not forgot to append to the multicall data array calls for unwrapWETH9 or refundETH. If the user forgets to do this a MEV bot can see the missed tokens and take them out because anyone can call the mentioned...

There is no option to cancel an EMERGENCY mode.

Lines of code Vulnerability details Impact There is no option to cancel an EMERGENCY mode in the pool. Once the pool is in an EMERGENCY mode, the pool can't work as normal forever. Proof of Concept The pool can be changed to an EMERGENCY mode by adminAction. function adminActionuint256 action,...

Liquidity cannot be removed by an approved address via Router

Lines of code Vulnerability details Impact Using the Router, liquidity can only be removed by the owner of an NFT, which significantly limits liquidity management. The Pool contract, however, does allow approved addresses to remove liquidity. Proof of Concept The Router contract is a higher level...

A finding that cannot be disclosed at the moment

Lines of code Vulnerability details This finding couldn't be disclosed at the time when the contest was running. After consulting with CloudEllie, it was decided to create a stub report so the finding could be submitted after the contest is over. A checksum of the report:...

exactInput allows stealing of funds via a malicious pool contract

Lines of code Vulnerability details Impact Users can lose funds during swapping. Proof of Concept The Router contract is a higher level contract that will be used by the majority of the users. The contract implements the exactInput functions that users call to perform multiple swaps in a single...

Admin can call claim fee multiple times with no restriction

Lines of code Vulnerability details Impact Admin can call claim fee multiple times to drain the fund in the pool Proof of Concept Admin can perform certain actions in the Pool.sol function adminActionuint256 action, uint16 val, address recipient external checkReentrancytrue, true; // reentrancy...

Pool with any fee tier can be created

Lines of code Vulnerability details Impact Pool with any fee tier can be created Proof of Concept There is a section in this medium: In Fee Structure: Maverick AMM supports initializing pools with arbitrary fee rates, but it is expected that LPs will choose from one of the following “standard” fe...

Lack of access control for sweepToken, refundETH, unwrapWETH9 in Router.sol

Lines of code Vulnerability details Impact Lack of access control for sweepToken, refundETH, unwrapWETH9 in Router.sol, any WETH token, ETH and ERC20 sent to Router.sol is lost and claimable to anyone. Proof of Concept the function unwrapWETH9 and sweepToken and refundETH has no access control an...

Users can get free collateral when using non-reverting on failure baseTokens

Lines of code Vulnerability details Impact A user calling Collateral's deposit function when baseToken is a non-reverting on failure ERC20 token, can get an arbitrary amount of collateral without actually depositing a single base token in the contract. function depositaddress recipient, uint256...

Storage collision in Collateral.sol

Lines of code Vulnerability details Vulnerability details collateral.sol is an upgradeable contract. Upgradeable contracts should not use the constructor to initialize variables, as these will be set in the contract storage of the implementation contract, instead of the intended contract storage ...

function mint() in PrePOMarket shouldn't accept deposits after expiryTime of Market

Lines of code Vulnerability details Impact Function mint Mints Long and Short tokens in exchange for amount collateral and according to the comment in the IPrePOMarket: "Minting is not allowed after the market has ended." but there is no check or restriction in the code that to make sure minting ...

Protocol is not able to account for baseTokens generating yield

Lines of code Vulnerability details Impact The protocol's logic is based on the assumption that, while deposited, the underlying baseTokens will generate yield, which accrues to the Traders holding Collateral Tokens. However, there is no mechanism in Collateral.sol to allow it to account for this...

Attacker can make any function that relies on _satisfiesScoreRequirement reverts

Lines of code Vulnerability details Impact The NFTScoreRequirement contract have a function that checks the user score which diff it with the requiredScore variable, if it's higher then the function will continues, however the function that sets the requiredScore variable is made public with no...

Attacker can make anyone as allowedMsgSender

Lines of code Vulnerability details Impact The contract AllowedMsgSenders have a role allowedMsgSender, and the function to set that role is made public with no access control allowing the attacker to set anyone and escalate his privileges to that role Proof of Concept truffle console --networkId...

In WithdrawHook.hook(), withdraw limits can be bypassed.

Lines of code Vulnerability details Impact In WithdrawHook.hook, withdraw limits can be bypassed. As a result, users might withdraw more amount of the base token at a time than they should. Proof of Concept WithdrawHook.hook checks the withdraw limits like below. if lastGlobalPeriodReset +...

User may be blocked from market withdrawal for extended period of time

Lines of code Vulnerability details Impact WithdrawalHook::lastUserPeriodReset is global for all users, which means that each time that lastUserPeriodReset + userPeriodLength it'is able to block user from withdrawal', async = let previousResetTimestamp = await getLastTimestampethers.provider //...

Missing modifiers in the functions of several parent contracts

Lines of code Vulnerability details Impact The value can be set by everyone Proof of Concept All these functions can be overridden by the child contracts. If all these functions are called from the child contracts, there are modifiers restricting everyone to call the functions. However, all these...

An operator can bypass the withdrawal limits

Lines of code Vulnerability details Impact An operator can bypass the withdrawal limits if he withdraws when the lastUserPeriodReset + userPeriodLength or lastGlobalPeriodReset + globalPeriodLength is less than the block.timestamp. This causes a DOS as well since globalAmountWithdrawnThisPeriod...

WithdrawHook doesn't check that userWithdrawLimitPerPeriod is less than globalWithdrawLimitPerPeriod

Lines of code Vulnerability details Impact WithdrawHook doesn't check that userWithdrawLimitPerPeriod is less than globalWithdrawLimitPerPeriod. It allows to user withdraw more then globalWithdrawLimitPerPeriod per period. Proof of Concept Function WithdrawHook.hook should not allow to withdraw...

Unsafe usage of ERC20 methods

Lines of code Vulnerability details There are many weird ERC20 tokens that don't follow the standard ERC20 interface. Depending on the ERC20 token, some transfer errors may result in passing unnoticed, or some successful transfers may be treated as failed. The current implementation assumes that...

Insufficient support for fee-on-transfer type of ERC20

Lines of code Vulnerability details Some ERC20 token implementations have a fee that is charged on each token transfer. This means that the transferred amount isn't exactly what the receiver will get. A call to IERC20token.transferrecipient, 100 with a fee-on-transfer FOT of 5% will entitle the...

Mint might result in too few tokens minted

Lines of code Vulnerability details mint of PrePOMarket.sol allows the team to mint new short and long tokens in exchange for collateral tokens. Long and short tokens are default ERC20 tokens with 18 decimals, while the collateral ERC20 token uses the decimals of the underlying base token. Howeve...

unsafe transfer/TransferFrom breaks functionality of Collateral.sol

Lines of code Vulnerability details Description The ERC20 specification does not demand implementations to revert when the transfer and transferFrom functions fail. They may use the return value to signal the success code. Some tokens, like ZRX, indeed don't revert. In Collateral deposit and...

POSSIBLE FRONTRUNNING ATTACK ON MINTING LONG/SHORT POSITIONS

Lines of code Vulnerability details Impact createMarket function in PrePOMarketFactory.sol contract creates a new PrePOMarket contract. Salt is used for creating the contract which is computed from createPairTokens function. Variables passed to this function are visible from anyone they are input...

Permanent freeze of yield when TokenSender rewards bank is depleted and deposit or withdraw is called.

Lines of code Vulnerability details Description In collateral deposit and withdraw flow, a fee is calculated as a percentage of user's requested amount. It is passed to the DepositHook and WithdrawHook, for example in deposit: uint256 amountAfterFee = amount - fee; if addressdepositHook != addres...

Infinite approval given to DepositTradeHelper contract can be misused by malicious actors

Lines of code Vulnerability details Impact depositAndTrade function in DepositTradeHelper account has infinite approval to spend baseTokens & collateralTokens of user using offChain signatures. However, once swapRouter executes UniV3 swap transaction on line 33, the contract does not give up its...

Long and short tokens can continue to be minted even after expiry

Lines of code Vulnerability details Impact Users can mint after expiry but before the final price has been set Proof of Concept function mintuint256 amount external override nonReentrant returns uint256 requirefinalLongPayout MAXPAYOUT, "Market ended"; requirecollateral.balanceOfmsg.sender =...