trachev - High - Owner can accidentally withdraw users' funds

[sherlock-admin]

trachev

high

High - Owner can accidentally withdraw users' funds

Summary

The contract owner can accidentally withdraw user funds, due to the withdrawNative function.

Vulnerability Detail

A key functionality of the contract is that the owner can withdraw gas fees derived from users, who have called the claimOrder function. These fees can often be provided as WETH. The problem occurs due to the fact that in Uniswap V3 it is allowed to place limit orders on pools, where funds can be exchanged for WETH. Therefore after a fulfilled order, which has exchanged funds for WETH, the user's WETH will first be transfered to the contract, until the users individually calls claimOrder to acquire their exchanged funds. Before the claimOrder function has been called, the owner can accidentally evoke the withdrawNative function, which will withdraw gas per user fees, INCLUDING the WETH that has just been transfered to the contract, which belongs to users.
Here is an example:

1. Alice adds a new order which will eventually swap some token for WETH after the order is ITM (in the money).
2. The current exhange price moves and Alice's order is now ITM and can be fulfilled. So the performUpkeep function gets called, which consequently calls _fulfillOrder, which transfers the amountOut, in this case WETH, to the contract.
3. Now in order for Alice to claim the funds, she just swapped, claimOrder needs to be called. But before she is able to do that, the owner decides to collect gas per user fees (this are different fees from the swap fees) and calls withdrawNative.
4. Now Alice's funds are accidentally taken by the owner, and whenever she decides to call claimOrder the call would fail, due to lack of funds, or in some cases take funds that belong to other users.

Impact

Due to the fact that a substantial amount of funds can easily be taken by the owner, even though he may not be malicious, this vulnerability can have detrimental effects. A claimOrder transaction does not need to be frontran, nor does withdrawNative need to be called immediately after an order has been fulfilled, in order for the accident to occur. Furthermore, a lack of funds will occur and other users will also not be able to claim their liquidity.

Code Snippet

Below we can see that it is expected for the WRAPPED_NATIVE ERC20 address to be WETH's address.

 // @notice the token addres of the wrapped native token
    ERC20 public immutable WRAPPED_NATIVE; // Mainnet 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2

The function that the owner can use to withdraw the WETH:

function withdrawNative() external onlyOwner {
        uint256 wrappedNativeBalance = WRAPPED_NATIVE.balanceOf(address(this));
        uint256 nativeBalance = address(this).balance;
        // Make sure there is something to withdraw.
        if (wrappedNativeBalance == 0 && nativeBalance == 0) revert LimitOrderRegistry__ZeroNativeBalance();

        // transfer wrappedNativeBalance if it exists
        if (wrappedNativeBalance > 0) WRAPPED_NATIVE.safeTransfer(owner, wrappedNativeBalance);
        // transfer nativeBalance if it exists
        if (nativeBalance > 0) owner.safeTransferETH(nativeBalance);
    }

This is where fulfilled orders are transfered to the contract, where the params include recipient, pointing to address(this):
https://github.com/sherlock-audit/2023-06-gfx/blob/main/uniswap-v3-limit-orders/src/LimitOrderRegistry.sol#L1400

collectParams = NonFungiblePositionManager.CollectParams({
      tokenId: target,
      recipient: address(this),
      amount0Max: amount0Max,
      amount1Max: amount1Max
});
POSITION_MANAGER.collect(collectParams);

Tool used

Manual Review

Recommendation

Keep track of how much WETH the contract owes to users and subtract it from the entire WETH balance of the contract when calling withdrawNative.

Duplicate of #48