Dug - Protocol is unable to support any pools that include wrapped native tokens

[sherlock-admin]

Dug

medium

Protocol is unable to support any pools that include wrapped native tokens

Summary

The audit details indicate that the protocol should be able to support any ERC20 tokens which trade on Uniswap v3.

However, the protocol, as currently implemented, is unable to support any pools that user the wrapped native token as part of the trading pair as it will result in the owner being unable to withdraw fees without draining all pending claims in that token.

Vulnerability Detail

The main reason the protocol is unable to support trading pairs that include the wrapped native token is in how fee accounting in performed in the contract.

When a claim is made, the fee is taken in the native token or the wrapped, ERC20 version of it. This is done in the claimOrder function.

        if (msg.value >= userClaim.feePerUser) {
            // refund if necessary.
            uint256 refund = msg.value - userClaim.feePerUser;
            if (refund > 0) sender.safeTransferETH(refund);
        } else {
            WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);
            // If value is non zero send it back to caller.
            if (msg.value > 0) sender.safeTransferETH(msg.value);
        }

The fee is held in the contract until withdrawn from the owner via the withdrawNative function.

    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 function withdraws the entire balance of native tokens from the contract.

This conflicts with how token balances are held in the contract as orders are fulfilled. When an order if fulfilled the token is held in the contract until claimed by users.

This means that fulfilling any order that uses the wrapped native token will result in the contract holding a balance of the wrapped native token. The owner is then unable to withdraw their fees from the contract without also withdrawing all pending claims in that wrapped native token.

Impact

To support trading pairs that use the wrapped native token, the owner will be unable to withdraw fees, representing a loss of revenue for the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-06-gfx/blob/main/uniswap-v3-limit-orders/src/LimitOrderRegistry.sol#L505-L515

Tool used

Manual Review

Recommendation

Use the tokenSwapFees mapping to account for the fees in the native token. This will allow the owner to withdraw their fees without withdrawing the pending claims.

        if (msg.value >= userClaim.feePerUser) {
            // refund if necessary.
            uint256 refund = msg.value - userClaim.feePerUser;
            if (refund > 0) sender.safeTransferETH(refund);
        } else {
            WRAPPED_NATIVE.safeTransferFrom(sender, address(this), userClaim.feePerUser);
+           tokenToSwapFees[WRAPPED_NATIVE] += userClaim.feePerUser;
            // If value is non zero send it back to caller.
            if (msg.value > 0) sender.safeTransferETH(msg.value);
        }

    function withdrawNative() external onlyOwner {
        uint256 wrappedNativeBalance = tokenToSwapFees[WRAPPED_NATIVE];
        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);
+       if (wrappedNativeBalance > 0) {
+           tokenToSwapFees[WRAPPED_NATIVE] = 0;
+           WRAPPED_NATIVE.safeTransfer(owner, wrappedNativeBalance);
+       }
        // transfer nativeBalance if it exists
        if (nativeBalance > 0) owner.safeTransferETH(nativeBalance);
    }

Duplicate of #48

[dugdaniels]

Escalate for 10 USDC

It was noted in the judging report that this was invalidated for the following reason:

Known issue. Check Cyfrin 6.2.2

However, that Cyfrin issue describes an entirely different scenario.

This issue illustrates how if the protocol supports a pool that includes wrapped native tokens, when the owner withdraws their fees from the contract, all pending claims in that wrapped native token will be withdrawn with it.

The protocol is intended to be able to support "any ERC20 tokens which trade on univ3". However, that cannot be done without encountering situations where users lose funds or the protocol loses revenue.

[sherlock-admin2]

Escalate for 10 USDC

It was noted in the judging report that this was invalidated for the following reason:

Known issue. Check Cyfrin 6.2.2

However, that Cyfrin issue describes an entirely different scenario.

This issue illustrates how if the protocol supports a pool that includes wrapped native tokens, when the owner withdraws their fees from the contract, all pending claims in that wrapped native token will be withdrawn with it.

The protocol is intended to be able to support "any ERC20 tokens which trade on univ3". However, that cannot be done without encountering situations where users lose funds or the protocol loses revenue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[xiaoming9090]

This should be a duplicated of #48

[hrishibhat]

Result:
High
Duplicate of #48

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• dugdaniels: accepted