Lack of segregation between users' assets and collected fees resulting in loss of funds for the users
The users' assets are wrongly sent to the owner due to a lack of segregation between users' assets and collected fees, which might result in an irreversible loss of assets for the victims.\\nGLX uses the Chainlink Automation to execute the `LimitOrderRegistry.performUpkeep` function when there are orders that need to be fulfilled. The `LimitOrderRegistry` contract must be funded with LINK tokens to keep the operation running.\\nTo ensure the LINK tokens are continuously replenished and funded, users must pay a fee denominated in Native ETH or ERC20 WETH tokens on orders claiming as shown below. The collected ETH fee will be stored within the `LimitOrderRegistry` contract.\\n```\\nFile: LimitOrderRegistry.sol\\n    function claimOrder(uint128 batchId, address user) external payable returns (ERC20, uint256) {\\n..SNIP..\\n        // Transfer tokens owed to user.\\n        tokenOut.safeTransfer(user, owed);\\n\\n        // Transfer fee in.\\n        address sender = _msgSender();\\n        if (msg.value >= userClaim.feePerUser) {\\n            // refund if necessary.\\n            uint256 refund = msg.value - userClaim.feePerUser;\\n            if (refund > 0) sender.safeTransferETH(refund);\\n        } else {\\n            WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);\\n            // If value is non zero send it back to caller.\\n            if (msg.value > 0) sender.safeTransferETH(msg.value);\\n        }\\n..SNIP..\\n```\\n\\nTo retrieve the ETH fee collected, the owner will call the `LimitOrderRegistry.withdrawNative` function that will send all the Native ETH and ERC20 WETH tokens within the `LimitOrderRegistry` contract to the owner's address. After executing this function, the Native ETH and ERC20 WETH tokens on this contract will be zero and wiped out.\\n```\\nFile: LimitOrderRegistry.sol\\n    function withdrawNative() external onlyOwner {\\n        uint256 wrappedNativeBalance = WRAPPED_NATIVE.balanceOf(address(this));\\n        uint256 nativeBalance = address(this).balance;\\n        // Make sure there is something to withdraw.\\n        if (wrappedNativeBalance == 0 && nativeBalance == 0) revert LimitOrderRegistry__ZeroNativeBalance();\\n\\n        // transfer wrappedNativeBalance if it exists\\n        if (wrappedNativeBalance > 0) WRAPPED_NATIVE.safeTransfer(owner, wrappedNativeBalance);\\n        // transfer nativeBalance if it exists\\n        if (nativeBalance > 0) owner.safeTransferETH(nativeBalance);\\n    }\\n```\\n\\nMost owners will automate replenishing the `LimitOrderRegistry` contract with LINK tokens to ensure its balance does not fall below zero and for ease of maintenance. For instance, a certain percentage of the collected ETH fee (e.g., 50%) will be swapped immediately to LINK tokens on a DEX upon collection and transferred the swapped LINK tokens back to the `LimitOrderRegistry` contract. The remaining will be spent to cover operation and maintenance costs.\\nHowever, the issue is that there are many Uniswap V3 pools where their token pair consists of ETH/WETH. In fact, most large pools in Uniswap V3 will consist of ETH/WETH. For instance, the following Uniswap pools consist of ETH/WETH as one of the pool tokens:\\nUSDC / ETH (0.05% Fee) (TLV: $284 million)\\nWBTC / ETH (0.3% Fee) (TLV: $227 million)\\nUSDC / ETH (0.3% Fee) (TLV: $88 million)\\nDAI / ETH (0.3% Fee) (TLV: $14 million)\\nAssume that the owner has configured and setup the `LimitOrderRegistry` contract to work with the Uniswap DAI/ETH pool, and the current price of the DAI/ETH pool is 1,500 DAI/ETH.\\nBob submit a new Buy Limit Order swapping DAI to ETH at the price of 1,000 DAI/ETH. Bob would deposit 1,000,000 DAI to the `LimitOrderRegistry` contract.\\nWhen Bob's Buy Limit Order is ITM and fulfilled, 1000 ETH/WETH will be sent to and stored within the `LimitOrderRegistry` contract.\\nThe next step that Bob must do to claim the swapped 1000 ETH/WETH is to call the `LimitOrderRegistry.claimOrder` function, which will collect the fee and transfer the swapped 1000 ETH/WETH to Bob.\\nUnfortunately, before Bob could claim his swapped ETH/WETH, the `LimitOrderRegistry.withdrawNative` function is triggered by the owner or the owner's bots. As noted earlier, when the `LimitOrderRegistry.withdrawNative` function is triggered, all the Native ETH and ERC20 WETH tokens on this contract will be transferred to the owner's address. As a result, Bob's 1000 swapped ETH/WETH stored within the `LimitOrderRegistry` contract are sent to the owner's address, and the balance of ETH/WETH in the `LimitOrderRegistry` contract is zero.\\nWhen Bob calls the `LimitOrderRegistry.claimOrder` function, the transaction will revert because insufficient ETH/WETH is left in the `LimitOrderRegistry` contract.\\nUnfortunately for Bob, there is no way to recover back his ETH/WETH that is sent to the owner's address. Following outline some of the possible scenarios where this could happen:\\nThe owners set up their infrastructure to automatically swap a portion or all the ETH/WETH received to LINK tokens and transfer them to the `LimitOrderRegistry` contract, and there is no way to retrieve the deposited LINK tokens from the `LimitOrderRegistry` contract even if the owner wishes to do so as there is no function within the contract to allow this action.\\nThe owners set up their infrastructure to automatically swap a small portion of ETH/WETH received to LINK tokens and send the rest of the ETH/WETH to 100 investors/DAO members' addresses. So, it is no guarantee that the investors/DAO members will return the ETH/WETH to Bob.
Consider implementing one of the following solutions to mitigate the issue:\\nSolution 1 - Only accept Native ETH as fee\\nUniswap V3 pool stored ETH as Wrapped ETH (WETH) ERC20 token internally. When the `collect` function is called against the pool, WETH ERC20 tokens are returned to the caller. Thus, the most straightforward way to mitigate this issue is to update the contract to `collect` the fee in Native ETH only.\\nIn this case, there will be a clear segregation between users' assets (WETH) and owner's fee (Native ETH)\\n```\\nfunction withdrawNative() external onlyOwner {\\n// Remove the line below\\n    uint256 wrappedNativeBalance = WRAPPED_NATIVE.balanceOf(address(this));\\n    uint256 nativeBalance = address(this).balance;\\n    // Make sure there is something to withdraw.\\n// Remove the line below\\n    if (wrappedNativeBalance == 0 && nativeBalance == 0) revert LimitOrderRegistry__ZeroNativeBalance();\\n// Add the line below\\n    if (nativeBalance == 0) revert LimitOrderRegistry__ZeroNativeBalance();\\n\\n// Remove the line below\\n    // transfer wrappedNativeBalance if it exists\\n// Remove the line below\\n    if (wrappedNativeBalance > 0) WRAPPED_NATIVE.safeTransfer(owner, wrappedNativeBalance);\\n    // transfer nativeBalance if it exists\\n    if (nativeBalance > 0) owner.safeTransferETH(nativeBalance);\\n}\\n```\\n\\n```\\nfunction claimOrder(uint128 batchId, address user) external payable returns (ERC20, uint256) {\\n..SNIP..\\n    // Transfer tokens owed to user.\\n    tokenOut.safeTransfer(user, owed);\\n\\n    // Transfer fee in.\\n    address sender = _msgSender();\\n    if (msg.value >= userClaim.feePerUser) {\\n        // refund if necessary.\\n        uint256 refund = msg.value // Remove the line below\\n userClaim.feePerUser;\\n        if (refund > 0) sender.safeTransferETH(refund);    \\n    } else {\\n// Remove the line below\\n       WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);\\n// Remove the line below\\n       // If value is non zero send it back to caller.\\n// Remove the line below\\n       if (msg.value > 0) sender.safeTransferETH(msg.value);\\n// Add the line below\\n  revert LimitOrderRegistry__InsufficientFee;\\n    }\\n..SNIP..\\n```\\n\\nSolution 2 - Define state variables to keep track of the collected fee\\nConsider defining state variables to keep track of the collected fee so that the fee will not mix up with users' assets.\\n```\\nfunction claimOrder(uint128 batchId, address user) external payable returns (ERC20, uint256) {\\n..SNIP..\\n    // Transfer fee in.\\n    address sender = _msgSender();\\n    if (msg.value >= userClaim.feePerUser) {\\n// Add the line below\\n     collectedNativeETHFee // Add the line below\\n= userClaim.feePerUser\\n        // refund if necessary.\\n        uint256 refund = msg.value - userClaim.feePerUser;\\n        if (refund > 0) sender.safeTransferETH(refund);\\n    } else {\\n// Add the line below\\n     collectedWETHFee // Add the line below\\n= userClaim.feePerUser\\n        WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);\\n        // If value is non zero send it back to caller.\\n        if (msg.value > 0) sender.safeTransferETH(msg.value);\\n    }\\n..SNIP..\\n```\\n\\n```\\nfunction withdrawNative() external onlyOwner {\\n// Remove the line below\\n   uint256 wrappedNativeBalance = WRAPPED_NATIVE.balanceOf(address(this));\\n// Remove the line below\\n   uint256 nativeBalance = address(this).balance;\\n// Add the line below\\n uint256 wrappedNativeBalance = collectedWETHFee;\\n// Add the line below\\n uint256 nativeBalance = collectedNativeETHFee;\\n// Add the line below\\n collectedWETHFee = 0; // clear the fee\\n// Add the line below\\n collectedNativeETHFee = 0; // clear the fee\\n    // Make sure there is something to withdraw.\\n    if (wrappedNativeBalance == 0 && nativeBalance == 0) revert LimitOrderRegistry__ZeroNativeBalance();\\n\\n    // transfer wrappedNativeBalance if it exists\\n    if (wrappedNativeBalance > 0) WRAPPED_NATIVE.safeTransfer(owner, wrappedNativeBalance);\\n    // transfer nativeBalance if it exists\\n    if (nativeBalance > 0) owner.safeTransferETH(nativeBalance);\\n}\\n```\\n
Loss of assets for the users
```\\nFile: LimitOrderRegistry.sol\\n    function claimOrder(uint128 batchId, address user) external payable returns (ERC20, uint256) {\\n..SNIP..\\n        // Transfer tokens owed to user.\\n        tokenOut.safeTransfer(user, owed);\\n\\n        // Transfer fee in.\\n        address sender = _msgSender();\\n        if (msg.value >= userClaim.feePerUser) {\\n            // refund if necessary.\\n            uint256 refund = msg.value - userClaim.feePerUser;\\n            if (refund > 0) sender.safeTransferETH(refund);\\n        } else {\\n            WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);\\n            // If value is non zero send it back to caller.\\n            if (msg.value > 0) sender.safeTransferETH(msg.value);\\n        }\\n..SNIP..\\n```\\n