Upgraded Q -> M from 36 [1655654413719]

Judge has assessed an item in Issue 36 as Medium risk. The relevant finding follows: Allows owner to mint more tokens than planned There are two ways for the owner of the contract to mint tokens for themselves unrelated to the public sale: through the teamSummon function or by setting the minter ...

_handleExecuteTransaction may not working correctly on fee-on-transfer tokens. Moreover, if it is failed, fund may be locked forever.

Lines of code Vulnerability details Impact handleExecuteTransaction may not working correctly on fee-on-transfer tokens. As duplicated fee is applied to fee on transfer token when executing a arbitrary call message passing request. Moreover, the Executor contract increase allowance on that token...

Relayer Will Not Receive Any Fee If execute Reverts

Lines of code Vulnerability details Proof-of-Concept Connext relies on the relayer to trigger the BridgeFacet.execute function on the destination domain to initiate the token transfer and calldata execution processes. Relayers pay for the gas cost to trigger the execute function, and in return fo...

Ether can get lock on functions takeMultipleOneOrders and takeOrders

Lines of code Vulnerability details Impact Ether send by the user cang gets locks when taking order/s Proof of Concept If Bob use function takeMultipleOneOrders or takeOrders to buy and sends more ETH that it supposes to remaing ETH will be lost, also if the seller is selling for other token and...

LibDiamond.diamondCut() should check diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] != 0

Lines of code Vulnerability details Impact Normally, diamondStorage.acceptanceTimeskeccak256abi.encodediamondCut will be set in LibDiamond.proposeDiamondCut. Then in LibDiamond.diamondCut, it checks that diamondStorage.acceptanceTimeskeccak256abi.encodediamondCut function proposeDiamondCut...

Protocol can steal WETH founds

Lines of code Vulnerability details Impact The protocol can steal WETH founds with the refunds gas cost mechanism in the functions matchOneToOneOrders, matchOneToManyOrders and matchOrders This functions can call only by the MATCHEXECUTOR but we don't know what is this contract/address according...

ETH rescue does not work

Lines of code Vulnerability details Impact Both contracts InfinityExchange and InfinityStaker have a function rescueETH to allow an admin to rescue any ETH accidentally sent to the contracts. However, this ETH rescue functionality does not work. The code expects ETH to be sent to this function an...

function removeRouter() in RouterFacet don't check that router has balance and don't transfer it, it just set router owner and recipient to 0x0 which can cause make router balance in danger or unavailable for router owner

Lines of code Vulnerability details Impact There are some security levels for router, like setting owner and recipient and when removeRouter is called this values set to 0x0 and router address become vulnerable. contract should transfer router balance to recipient before removing it. Proof of...

NO TIMELOCK ON setProtocolFee() CAN LEAD TO SELLERS LOSING THEIR NFTs

Lines of code Vulnerability details NO TIMELOCK ON setProtocolFee CAN LEAD TO SELLERS LOSING THEIR NFTs In InfinityExchange.sol, there is no timelock on setProtocolFee. This is the fee that is applied in orders, and determines how much the Exchange receives in fee VS how much the seller receives...

Wrong gasCost calculation per order

Lines of code Vulnerability details Impact During a match, there's some accounting on how much gas we're spending, so that the executor can be reimbursed. The gas cost is split between multiple orders, computing the difference between the gas at the start and at the end. The gas at the start is...

matchOneToManyOrders doesn't consider numItems of the orders

Lines of code Vulnerability details Impact An order can specify a numItems in MakerOrder.constraints0. This number is the min/max number of items the order wants to buy/sell. For example a buy order can provide a list of nfts and say that wants to buy only 3 of them from that list. The function...

Maker buy order with no specified NFT tokenIds may get fulfilled in matchOneToManyOrders without receiving any NFT

Lines of code Vulnerability details The call stack: matchOneToManyOrders - matchOneMakerSellToManyMakerBuys - execMatchOneMakerSellToManyMakerBuys - execMatchOneToManyOrders - transferMultipleNFTs Based on the context, a maker buy order can set OrderItem.tokens as an empty array to indicate that...

Did Not Approve To Zero First Causing Certain Token Transfer To Fail

Lines of code Vulnerability details Proof-of-Concept Some tokens like USDT do not work when changing the allowance from an existing non-zero allowance value. For example Tether USDT's approve function will revert if the current approval is not zero, to protect against front-running changes of...

Incorrect check in diamondCut allows update be executed without proposal

Lines of code Vulnerability details The current implementation of LibDiamond.diamondCut allows any diamondCut update to be executed instantly, defeating the purpose of the 7 day update delay mechanism. The issue is this check in LibDiamond.diamondCut: require...

Router Owner Could Be Rugged By Admin

Lines of code Vulnerability details Proof-of-Concept Assume that Alice's router has large amount of liquidity inside. Assume that the Connext Admin decided to remove a router owned by Alice. The Connext Admin will call the RoutersFacet.removeRouter function, and all information related to Alice's...

Wrong implementation of withdrawAdminFees() can cause the adminFees to be charged multiple times and therefore cause users' fund loss

Lines of code Vulnerability details function withdrawAdminFeesSwap storage self, address to internal IERC20 memory pooledTokens = self.pooledTokens; for uint256 i = 0; i pooledTokens.length; i++ IERC20 token = pooledTokensi; uint256 balance = self.adminFeesi; if balance != 0 token.safeTransferto,...

Accumulated ETH fees of InfinityExchange cannot be retrieved

Lines of code Vulnerability details ETH fees accumulated from takeOrders and takeMultipleOneOrders operations are permanently frozen within the contract as there is only one way designed to retrieve them, a rescueETH function, and it will work as intended, not being able to access ETH balance of...

The amount of gas calculation for refunding is incorrect

Lines of code Vulnerability details Impact While matching the orders the gas cost is calculated so that it can be refunded back to the contract. The calculation for this gas is incorrect. The function keeps track of the gasleft at the beginning of the loop and adds additional amount of gas for pr...

attacker can perform griefing for process() in PromiseRouter by reverting calls to callback() in callbackAddress

Lines of code Vulnerability details Impact process in PromiseRouter is used for process stored callback function and anyone calls it gets callbackFee and it calls callback function of callbackAddress. but attacker set a callbackAddress that reverts on callback and cause process caller griefing...

Router Owner Could Steal All The Funds Within SponsorVault

Lines of code Vulnerability details Proof-of-Concept Assume the following: For simplity sake, only two 2 routers exist within Connext. Gas, relayer, callback fees and slippage are ignored. An attacker owns Router A. Router A has 1,000,000 oUSDC on Optimism Domain/Chain Router B has only 100 oUSDC...

Centralization Risk with onlyOwner modifier

Lines of code Vulnerability details Impact During the code review, It has been observed the all currency tokens can be withdraw by owner without timelock. The currency token should not be withdrawn by owner. This poses centralization risk. Proof of Concept 1. Navigate to the following contract...

Multiple items order can be matched to many same items order

Lines of code Vulnerability details Impact Let's consider an example. Alice makes an order for an ERC1155, where she wants to buy 10 items with id=1 and 10 with id=2. This order can be matched using matchOneToManyOrders with two orders that sell both 10 items with id=1. Basically Alice gets 20 id...

Protocol fee rate can be arbitrarily modified by the owner and the new rate will apply to all existing orders

Lines of code Vulnerability details function matchOneToOneOrders OrderTypes.MakerOrder calldata makerOrders1, OrderTypes.MakerOrder calldata makerOrders2 external uint256 startGas = gasleft; uint256 numMakerOrders = makerOrders1.length; requiremsg.sender == MATCHEXECUTOR, 'OME';...

Malicious Relayer Can Replay Execute Calldata On Different Chains Causing Double-Spend Issue

Lines of code Vulnerability details Proof-of-Concept This issue is only applicable for fast-transfer. Slow transfer would not have this issue because of the built-in fraud-proof mechanism in Nomad. First, the attacker will attempt to use Connext to send 1000 USDC from Ethereum domain to Optimism...

Current implementation of arbitrary call execute failure handler may break some use case for example NFT bridge.

Lines of code Vulnerability details Impact Current implementation of arbitrary call execute failure handler may break some use case for example NFT Bridge. In the case of NFT Bridge, NFT may be lost forever. This is likely to be happened in the case of out of gas. Proof of Concept Relayer receive...

Incorrect accounting results in loss of user stake

Lines of code InfinityStaker.solL116-L131 InfinityStaker.solL290-L325 Vulnerability details Impact In InfinityStaker.sol, when calling the unstake function updateUserStakedAmounts is called to readjust the user staked amounts. If a user stakes an amount in a duration and has an already vested amo...

Extra msg.value in takeOrders

Lines of code Vulnerability details Impact In takeOrders, msg.sender can pay the items in ether or other native coins. // check to ensure that for ETH orders, enough ETH is sent // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent if isMakerSeller &&...

Owner cannot transfer ETH balance of the exchange

Lines of code Vulnerability details User called takeOrders and takeMultipleOneOrders functions accumulate native token fees over time. These fees end up being frozen on the contract balance. There is only one way for an owner to transfer them, a rescueETH function, that isn’t able to access ETH...

User may lose funds if msg.value > totalPrice

Lines of code InfinityExchange.solL300-L328 InfinityExchange.solL336-L364 Vulnerability details Impact When using InfinityExchange.sol:takeMultipleOneOrders or InfinityExchange.sol:takeOrders, if currency == address0 and a user has a msg.value totalPrice the overspent ETH is left in the contract...

Rescue ETH at InfinityExchange.sol is incorrectly implemented

Lines of code Vulnerability details Impact Admins are unable to recover any ETH locked in the contract PoC The function rescueETHInfinityExchange.sol is incorrectly implemented. It is using msg.value instead of the balance of the contract function rescueETHaddress destination external payable...

getCurrentPrice will be return a higher value than expected due to insufficient check in isTimeValid

Lines of code Vulnerability details The Boolean value will always return true if start price is zero. The condition will be sufficiently fulfilled since a time will be set in the future but no time is set in the past or at current time: sumCurrentPrices uses the returned value of getCurrentPrice...

_transferNFTs can end up transferring nothing

Lines of code Vulnerability details Malicious maker can list an NFT that conforms to ERC-165, but reports that it's neither ERC721, nor ERC1155, i.e. both supportsInterface0x80ac58cd and supportsInterface0xd9b67a26 are false. In all other regards it can be fully valid NFT, for example having...

Timelock able to be bypassed because of wrong check in LibDiamond

Lines of code Vulnerability details Impact In the walkthrough video, it said that the upgrades of Diamond must go through a proposal window with a delay of 7 days. Upgrade should be done by first call proposeDiamondCut and then wait 7 days and call diamondCut. But this timelock can be bypassed...

rescueETH function doesn't works as expected

Lines of code Vulnerability details Impact The rescueETH function is implemented to collect any unexpected ETH transferred to the infinityExchange.sol contract, But this function will not work as expected. The function is supposed to return the eth from the contract to the specified destination...

Maker order buyer is forced to reimburse the gas cost at any tx.gasprice

Lines of code Vulnerability details uint256 gasCost = startGasPerOrder - gasleft + wethTransferGasUnits tx.gasprice; // if the execution currency is weth, we can send the protocol fee and gas cost in one transfer to save gas // else we need to send the protocol fee separately in the execution...

Taker can provide several instances of the cheapest ERC-1155 item in a multi item bundle

Lines of code Vulnerability details Maker bid for a bundle of ERC-1155 items can be tricked into successful execution by providing several instances of the cheapest item instead of the required bundle. This way a malicious taker can receive full maker's price, providing several instances of the...

Accidental ETH transfers to InfinityStaker cannot be withdrawn

Lines of code Vulnerability details The rescueETH function is meant to allow the contract owner to withdraw ETH accidentally sent to the contract. However, it will forward the exact ETH amount sent by the caller to the given destination address, rather than withdraw ETH from the contract balance:...

InfinityExchange owner can steal user's tokens via front-running

Lines of code Vulnerability details Impact Contract InfinityExchange.sol charges protocol fee through PROTOCOLFEEBPS. The issue is that owner of the contract is able to change protocol fee at any time without any restriction which puts him in a very privileged position and allows him to steal...

Diamond upgrade proposition can be falsified

Lines of code Vulnerability details Impact Diamond is to be upgraded after a certain delay to give time to the community to verify changes made by the developers. If the proposition can be falsified, the contract admins can exploit the contract in any way of their choice. Proof of Concept To...

Tokens with decimals larger than 18 are not supported

Lines of code Vulnerability details For tokens with decimals larger than 18, many functions across the codebase will revert due to underflow. function getPriceFromDexaddress tokenAddress public view returns uint256 PriceInfo storage priceInfo = priceRecordstokenAddress; if priceInfo.active uint25...

routerBalances[msg.sender][_local] can be inflated in repayAavePortal due to underflow in unchecked math

Lines of code Vulnerability details function repayAavePortal address local, uint256 backingAmount, uint256 feeAmount, uint256 maxIn, bytes32 transferId external uint256 totalAmount = backingAmount + feeAmount; // in adopted uint256 routerBalance = s.routerBalancesmsg.senderlocal; // in local //...

Malicious Relayer Could Cause A Router To Provide More Liquidity Than It Should

Lines of code Vulnerability details Proof-of-Concept Assume this is a fast-transfer path and the sequencer has a good reason e.g. some sophisticated liquidity load balancing algorithm to assign 3 routers to provide liquidity for a transfer of 90 DAI Therefore, each of them will provide 30 DAI...

Diamond can be updated without proposing the change

Lines of code Vulnerability details Impact The diamond shall be monitored externally to remove the need of trust to developers. If a timelock can be bypassed, it poses a threat as people who weren't trusted can exploit the system. Additionally, the contract can immediately perform any delegatecal...

Upgraded Q -> M from 225 [1655654402923]

Judge has assessed an item in Issue 225 as Medium risk. The relevant finding follows: C4-011 : Centralization Risk On The teamSummon Function - LOW Impact - LOW With the teamSummon function, owner can mint unlimited warriors. This poses a security risk. The max/min limit should be implemented at...

Issues beyond expected behavior.

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. function unstakeuint256 amount external override nonReentrant whenNotPaused requireamount != 0, 'stake amount cant be 0'; uint256 noVesting = userstakedAmountsmsg.senderDuration.NONE.amount; uint256...

Precision Loss During Division

Lines of code Vulnerability details Proof-of-Concept Assume that toSwap = 10 DAI and pathLen = 3 in this example. Therefore, the bridge will pull 10 DAI from the RouterFacet contract. However, within the BridgeFacet.handleExecuteLiquidity function, due to precision loss when solidity handles...

Funds(ETH) permanent lock on InfinityStaker.sol

Lines of code Vulnerability details Impact The rescueETH function does not work as expected and if the contract receives ETH, it gets stuck in the contract Proof of Concept If an addresswallet or contract send ETH to the InfinityStaker.sol contract, the owneradmin can't rescue the ETH because the...

Malicious Relayers Could Favor Their Routers

Lines of code Vulnerability details Vulnerability Details Assume that a malicious relayer operates a router in Connext providing fast-liquidity service. A malicious relayer could always swap the routers within the execute calldata with the routers owned by malicious relayer, and submit it to the...

AssetLogic's _swapAssetOut can fail on the one step approval

Lines of code Vulnerability details swapAssetOut performs one step approval for an arbitrary assetIn before calling pool's swapExactOut. As ERC20 that do not allow approval race condition prohibit setting approval to a new positive value when allowance is positive already, this call will fail if...

nonReentrant modifier is not added to all functions that generate state changes, there is a possibility of re-entry.

Lines of code Vulnerability details Impact Detailed description of the impact of this finding. I noticed that the nonReentrant modifier only adds some of the functions that generate state changes to the InfinityExchange.sol contract, which provides the feasibility of reentrancy between multiple...