charles__cheerful - Medium5-CrossChainWETHSwapFeesChargedUnnecesarily

[sherlock-admin4]

charles__cheerful

medium

Medium5-CrossChainWETHSwapFeesChargedUnnecesarily

by CarlosAlegreUr

Summary

When doing a cross-chain transfer with any valid fromToken, using sgETH as bridgeToken and WETH as toToken via the WooRouterV2 swap on destination chain. The user is charged an unnecessary fee.

Vulnerability Detail

When receiving a cross-chain swap trhough sgReceive() at WooCrossChainRouterV4, if the bridgeToken is sgETH then the _handleNativeReceived() will be called. This function if toToken != ETH_PLACEHOLDER_ADDR will perform a swap to change the eth used as bridgeToken for the toToken using, for example, the very same WooRouterV2. And for exchanging ETH it needs to be wrapped up as WETH which it does by calling IWETH(weth).deposit{value: bridgedAmount}();.

The problem comes when the toToken desired is WETH, then a WETH to WETH swap will be carried out by the WooRouterV2 which will result in a fee being charged to the user due to a swap which makes no sense but would execute. So the user is losing unnecessary unexpected money.

You can see that WooRouterV2 allows for swaps where from and to tokens are the same token exeuting the following code:

See swap the same `from` and `to` tokens via WooRouterV2 👁️

To run the code copy paste it inside the `./test/typesript/WooRouterV2.test.sol` file, then inside the `describe("Swap Functions", () => {})`, and then after the `beforeEach("Deploy WooRouterV2", async () => {})`, and then run: ```bash npx hardhat test test/typescript/WooRouterV2.test.ts ``` ```typescript it.only("swap btc -> btc", async () => { await btcToken.mint(user.address, ONE.mul(5)); console.log("POOL BTC BALANCE", await utils.formatEther(await btcToken.balanceOf(wooPP.address))); console.log("Swap: btc -> btc"); const fromAmount = ONE.mul(2); const minToAmount = ONE.mul(1); await btcToken.connect(user).approve(wooRouter.address, fromAmount); await wooRouter .connect(user) .swap(btcToken.address, btcToken.address, fromAmount, minToAmount, user.address, ZERO_ADDR); console.log("POOL BTC BALANCE", await utils.formatEther(await btcToken.balanceOf(wooPP.address))); console.log("That means from the 2 BTC user sent only 0.002 were left as fee."); console.log("What matters for our issue is that the tx succeeded and a fee was taken."); }); ```

📘 Note ℹ️: The cross-chain tx described is feasible as there is no kind of require(toToken != WETH && brdigeToken != sgETH) anywhere.

🚧 Note ⚠️: I'm not sure what would happen if choosing 1inch option. If the swaps go through this problem would apply. But if the tx reverts this problem wouldn't apply as the swapping fee of 1inch wouldn't be applied and the transfer of bridgeAmount would take place as expected. Due to personal lack of time I let this question open. Anyway the recommendation proposed would fix the problem too in case 1inch also allows execution of the unnecessary swap.

Impact

Users lose unnecessary money when doing a cross-chain transfer with sgETH as bridgeToken and WETH as toToken via the WooRouterV2 swap on detination chain.

Code Snippet

• _handleNativeReceived() deposit WETH to later perform swap

• _handleNativeReceived() can execute swap through router

Tool used

Manual Review

Recommendation

At _handleNativeReceived(). In the case of bridging with sgETH, after the if(toToken == ETH_PLACEHOLDER_ADDR){}, add an extra if that checks if toToken != WETH, and if they are indeed different proceed with the swap.

       if (toToken == ETH_PLACEHOLDER_ADDR) {
         // code for when no swap required...
        }

        IWETH(weth).deposit{value: bridgedAmount}();

+       if (toToken != WETH) {
            // Swap required!
            // Swap logic...
+        }else{
+           // send the WETH
+       }

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

technically yes; it's WETH to WETH; but the user want to exchange another token (e.g.sgETH) to WETH; therefore; the fees are taken cause the user initially swaps not-WETH to WETH

[CarlosAlegreUr]

Escalate

I think I undertand where your point comes from, but I think it's wrong for the following reasons.

The way your comment is wrong is that actually if using sgEth as bridgeToken, even though its a token and thus a swap should be made to WETH and thus charge the fee and thus be valid as you say, even though that, the thing is that bridged sgEth doesnt arrive to the CrossChainRouterV4 contract as a token but already as a native coin in msg.value. Thus when using sgETH as bridgeToken you are, a bit confusingly, not actually receiving a ERC20 token but native coin. You can see that this is true carefully looking at the code:

When sgReceive() is called and bridgedToken=sgETH we can see that the value being sent is not actualy sgETH token but pure ETH as msg.value.

That is why the code does the following, first in sgReceive(): See code in repo click here Notice the dev team added a comment pointing out what I'm trying to explain here. Click to see.

if (bridgedToken == sgInfo.sgETHs(sgInfo.sgChainIdLocal())) {
            // 🟢 The comment below this one was added by the dev team and also informs that when sgETH, native token (coin) is received
            // bridgedToken is SGETH, received native token
            _handleNativeReceived(refId, to, toToken, amountLD, minToAmount, dst1inch);
}

If sgEth is used, _handleNativeReceived() is called, and then inside _handleNativeReceived():

See code in repo click here

   ) internal {
        address msgSender = _msgSender();

        if (toToken == ETH_PLACEHOLDER_ADDR) {
            // Directly transfer ETH
            TransferHelper.safeTransferETH(to, bridgedAmount);
            emit WooCrossSwapOnDstChain(/*event args*/);
            return;
        }
    // (rest of code...)

You can see that the very first action taken is to check if you wanted native coin on destination chain, and if so, transfer it to you and then return;. This is because the sgETH is sent to the router as already native coin in msg.value and not as a token itself.

That is why there is no sgETH -> WETH swap and thus the unnecesarry WETH -> WETH swap in WooFi will execute as explained in the issue thus charging valid users fees that shouldnt be charged.

[sherlock-admin2]

Escalate

I think I undertand where your point comes from, but I think it's wrong for the following reasons.

The way your comment is wrong is that actually if using sgEth as bridgeToken, even though its a token and thus a swap should be made to WETH and thus charge the fee and thus be valid as you say, even though that, the thing is that bridged sgEth doesnt arrive to the CrossChainRouterV4 contract as a token but already as a native coin in msg.value. Thus when using sgETH as bridgeToken you are, a bit confusingly, not actually receiving a ERC20 token but native coin. You can see that this is true carefully looking at the code:

When sgReceive() is called and bridgedToken=sgETH we can see that the value being sent is not actualy sgETH token but pure ETH as msg.value.

That is why the code does the following, first in sgReceive(): See code in repo click here Notice the dev team added a comment pointing out what I'm trying to explain here. Click to see.

if (bridgedToken == sgInfo.sgETHs(sgInfo.sgChainIdLocal())) {
            // 🟢 The comment below this one was added by the dev team and also informs that when sgETH, native token (coin) is received
            // bridgedToken is SGETH, received native token
            _handleNativeReceived(refId, to, toToken, amountLD, minToAmount, dst1inch);
}

If sgEth is used, _handleNativeReceived() is called, and then inside _handleNativeReceived():

See code in repo click here

   ) internal {
        address msgSender = _msgSender();

        if (toToken == ETH_PLACEHOLDER_ADDR) {
            // Directly transfer ETH
            TransferHelper.safeTransferETH(to, bridgedAmount);
            emit WooCrossSwapOnDstChain(/*event args*/);
            return;
        }
    // (rest of code...)

You can see that the very first action taken is to check if you wanted native coin on destination chain, and if so, transfer it to you and then return;. This is because the sgETH is sent to the router as already native coin in msg.value and not as a token itself.

That is why there is no sgETH -> WETH swap and thus the unnecesarry WETH -> WETH swap in WooFi will execute as explained in the issue thus charging valid users fees that shouldnt be charged.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[WangSecurity]

Escalate

After additional discussions in discord, I admit that there is something I miss about this one. Therefore, escalating on behalf of @CarlosAlegreUr , after he provides additional comments from discord, I will give my reasons why it should remain invalid and leave the decision to the head of judging.

[sherlock-admin2]

Escalate

After additional discussions in discord, I admit that there is something I miss about this one. Therefore, escalating on behalf of @CarlosAlegreUr , after he provides additional comments from discord, I will give my reasons why it should remain invalid and leave the decision to the head of judging.

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.

[CarlosAlegreUr]

Escalate

After additional discussions in discord, I admit that there is something I miss about this one. Therefore, escalating on behalf of @CarlosAlegreUr , after he provides additional comments from discord, I will give my reasons why it should remain invalid and leave the decision to the head of judging.

95 Final escalation and thanks WangSecurity for your time, effort and kindness.

Summing up discord discussion with WangSecurity (lead judge) and Tapir (another contestant).

There are 4 main reasons why this issue has been considered invalid (from discord chat):

1. To me it looks like design advice honestly. It's a good find honestly, but it's an issue about improving user experience.
2. The loss is small and finite.
3. It's just complicating the code.
4. Using WETH as toToken swap might not be a "core" functonality.

I disagree reagarding 1, I potentially agree with number 2 but it depends on how Serlok defines small and finite, I disagree with 3 && 4. The counterarguments and reasons why I disagree are written after the summary of the issue.

---

First lets summarize the issue: The issue is that the user will be charged a fee he shouldn't when chosing to cross-swap WETH as toToken using WooPP as the exchange for the final swap in dstChain and sgEth as bridgeToken.

(from discord chat) Mmmm I will try to explain it again.

First when receiving sgETH the contract doesn't receive the ERC20 but diretly receives, from the bridge, native coin through msg.value.

Now lets remember the inputs:

fromToken => any birdgeToken => sgEth toToken => WETH dst1inch.swapRouter = WooPP pool

Because bridgeToken is sgETH the _handleNativeReceived() function will be executed as you can see in this if statement here.

Now inside _handleNativeReceived() there are two possible main paths, whether you want native coin and this block of code gets executed and transfers msg.value to you and returns, OR, the msg.value is wrapped up latter to procced with the swap bridgeToken => toToken.

Wrap happens after the if, here.

Here is the KEY PART, we are not using 1inch for the swap and we specified WooPP pool as the exchange to use. So this block of code will be executed.

The swap will be WETH => toToken as you can see in the next line. But toToken can also be WETH.

And as the in the code I added to my original issue, inside the: See swap the same from and to tokens via WooRouterV2 👁️ section. The WooPP pool allows for execution of "same-token" swaps even if they dont make sense. Like our now WETH => WETH swap.

In those executions you can see in the code I provided that a fee is also charged in the WooPP pool. This fee is the one that makes no sense to charge to the user as they already have the toTokend desired which is WETH.

Finally add that this makes sense if you are bridging for example to Arbitrum, a chain supported by the protocol.

The key points you might be missing are:

1. The swap is not done via 1inch but via WooPP pool. And this allows for the "same-token" WETH => WETH unnecessary swap that involves the fee any swap on WooPP is charged.

2. sgEth is given by the bridge to the corss-chain-router, but not as an ERC20 but as native coin through msg.value. That is why is later wrapped up to WETH to proceed with the swap in case you specified an ERC20 as toToken, but this toToken can be WETH and then the extra fee is charged.

---

1.

(from discrod chat) I don't agree on it is a UX problem. I couldn't find anywhere where it says you can't bridge WETH uing sgETH as bridgeToken using WooPP for the destination swap. The only limits on tokens you can use according to the protocol are the ones on their IntegrationHelper contract and the ones supported by the external brige they use (Stargate).

And WETH is supported in WooPP pool and can be bridged to Arbitrum (supported by the protocol) via stargate. As you can see in their contract deployed on getSupportedTokens(), the WETH Arbitrum address is among them. The only restriction I could find in bridging is that you can't use the native coin as bridgeToken, you gotta use sgETH or sgVersion. So, under all restrictions the protocol and team set, this is a valid user interaction which results in loss of funds due to unnecessary fee.

I will make an analogy with a car. If you buy a car and the seller tells you that the car can drive in uneven pavements but then you drive it in uneven pavement made of sand and the car gets some damange then it's not your UX fault because as far as you were told, the car works on uneven pavements. It said nothing about the sand, in fact is a family car and it is expected to probably go on beach holidays some times.

So to sum up I don't agree with the argument that it is a UX problem becaues the developers never said they forbid swapping WETH in their pool or bridging it as toToken. In fact all indicates this is expected because WETH is approved as a token to be used in the WooPP and there are no filters for forbiding it in the bridge function either.

2.

(from discrod chat) About the it's a small and finite amount. It depends what you consider small and finite. The WooPP pool charges a % fee, and depending on the size of the swap the % might be small but the absolute amount can be considered big.

This is very similar to what I wrote before in issue 97 comment section 2, adapted for this issue would be: My agreement with the argument of small defined loss depends on how sherlok defines a small finite loss. This could be considered small in terms of percentage as the loss will be as big as the protocols' fee charged for swapping on WooPP pool, which is a small % (lets say 1% or even 0.05%).

Now despite of that a 1% loss on let's imagine a traded amount of 1 million dollars would be 10K of loss which is quite an amount of money to lose (or 5K in the 0.05% case). So idk how Sherlok defines finite small loss, in absolute terms or proportional terms.

If it is in proportional termns then okay it's a small loss, you lost 10K while managing 1 million due to code issues. But if it is absolute terms I do not think 10K or 5K is a small loss.

As it is a percentage of the swapped amount, it can be considered proportionally low. But in absolute terms the amount lost can be seen as big, like the 10K loss explained. Also if we add the time factor, over time, all people using this option of bridging where this swap is unnecesarily done the amount of money lost will be accumulating and can get big. Anyway to sum up, depending on the nuance of how you define small and finite loss I would agree or disagree with the argument.

3.

(from discrod chat) I don't think fixing the issue adds a lot of complexity. As it can seen in the Recommendation section a simple if statement would fix the issue. As much, the if with a comment saying, if the toToken desired was WETH there is no need to swap and just send the token.

4.

(from discrod chat) I understand as "core functionalities" of this audit: their WooPP pool usage with the sPMM algorithm and cross-chain swaps. This is a valid cross-chain swap. Otherwise why would they bother to add the cross-chain contracts to the audit.

These are all the arguments and counterarguments given on discord for this issue.

[WangSecurity]

Thank you for such an insightful comment!

The reasons why I still think it should remain low since it's essentially works as it should be.

The inputs that the Watson uses in his examples are:

fromToken => any birdgeToken => sgEth toToken => WETH dst1inch.swapRouter = WooPP pool

Therefore, I assume that we charge the fee here as expected since sgETH and WETH are technically different tokens even tho we can say they're quite the same. Therefore, I believe it would be nice to not charge the fee in that case, but I cannot say anything it broken here.

Moreover, I think the rule of financial loss to exceed small and finite amounts can be applied here since it's a small percentage of the swap (but I may be applying it here incorrectly). Therefore, I think it in facts work as expected, but the report is improving the protocol a bit. Don't get me wrong, it's a nice finding, but I don't see it as medium, unfortunately.

I admit that I may be wrong in my assumptions and will take any decision from the head of Judging, but I believe it's not sufficient to be medium (thank you for such and insightful explanation G, it looks very good).

[Czar102]

I believe the current system may work suboptimally with sgETH and WETH, and the recommendation would remove that suboptimal behavior. But design improvements aren't security issues, and I consider this report to be informational.

As @WangSecurity mentioned:

Therefore, I assume that we charge the fee here as expected since sgETH and WETH are technically different tokens even tho we can say they're quite the same. Therefore, I believe it would be nice to not charge the fee in that case, but I cannot say anything it broken here.

Planning to reject the escalation and leave the issue as is.

[CarlosAlegreUr]

I believe the current system may work suboptimally with sgETH and WETH, and the recommendation would remove that suboptimal behavior. But design improvements aren't security issues, and I consider this report to be informational.

As @WangSecurity mentioned:

Therefore, I assume that we charge the fee here as expected since sgETH and WETH are technically different tokens even tho we can say they're quite the same. Therefore, I believe it would be nice to not charge the fee in that case, but I cannot say anything it broken here.

Planning to reject the escalation and leave the issue as is.

@Czar102 and @WangSecurity , thanks for your point of view, but I still don't agree, these are my reasons:

1.

When msg.value is used, the native coin, lets say ether in Arbitrum example. In that case there is no fee charged, even if the native coin ether. We can also say about ether to be: "technically different tokens even tho we can say they're quite the same".

Thus we can see that when receiving the same or similar a asset to sgEth in dstChain no swap is expected to be performed by the code. Thus same would apply to WETH.

2.

Lets say that, doesnt matter if they are similar, they are in fact different tokens and a swap fee should be taken. In that case the swap fee taken should be because of a swap sgETH => WETH, and not WETH => WETH. This is problematic as differnet swaps gather differents amount of fees thus even in this path, the user would be charged something he didn't pay for.

WooPP pool does not allow for sgETH to be swapped. As we cann see sgEth on Arbitrum is not expected in the supported tokens function mentioned during the dicussions. But even if they indeed supported it, in WooPP different tokens have different feeRates so, for example swaping ValidToken1 => ValidToken2 is no the same as swapping ValidToken2 => ValidToken3. So it would actually matter if the swap is sgETH => WETH or WETH => WETH, thus the code would still be charging incorrect amounts to clients.

In the other hand, in case of using 1inch same applies, nothing guarantees that the fee charged for a swap sgETH => WETH is the same as the fee charged for a swap WETH => WETH (if the latter is posible in 1inch) so users would be charged incorrect amount of fees in this case. Although this case sgETH => WETH can't be executed because the WETH address is hardoced in both swaps, whether trhough 1inch or WooPP swap.

Sum-up

No matter what, the conclusion I take is that fee should not be charged. First because I think the code clearly treats "quite the same" tokens as no need to swap. And second, even if lets say we must swap, the fees would be incorrect.

I don't see it as suboptimal behaviour but as an issue in the code charging fees that it should not charge and that didn't expect.

[Czar102]

@CarlosAlegreUr I don't quite understand your second point. Could you explain it in a different way? Preferably as a short summary?

[CarlosAlegreUr]

@CarlosAlegreUr I don't quite understand your second point. Could you explain it in a different way? Preferably as a short summary?

@Czar102

So, I don't think a fee should be charged if from bridged sgEth we eventually want WETH because I think the code is not meant to do that for the reasons mentioned in point 1 in the comment above.

But, assusming your position of a fee should be charged, there would still be a problem. As the swap made in the code would be WETH => WETH and not sgEth => WETH. The protocol's pool WooPP charges different fees for different assets swaped, so the fee for WETH => WETH would be different than the fee for sgEth => WETH. So, even if we assume a fee should be charged, the code would still be charging incorrect fees. (a similar thing appliess to 1inch swap)

Thus, sgEth as bridgeToken and WETH as toToken would still have incorrect fee behaviour.

[WangSecurity]

Do I understand correctly that when it comes to handleNativeReceive, we skip the first check if (toToken == ETH_PLACEHOLDER_ADDR) at L279, cause our toToken is not ETH.

After it at line 299 we wrap msg.value into WETH and then at lines 306 or 349 we swap WETH to WETH, even tho the bridge token was sgETH, the function still wraps it into WETH before swapping into WETH. Correct?

And another question, can you forward to lines of code where it handles swap from sgETH to WETH and WETH to WETH to see how the fees differ (sorry if you already sent it, cannot find).

[CarlosAlegreUr]

Do I understand correctly that when it comes to handleNativeReceive, we skip the first check if (toToken == ETH_PLACEHOLDER_ADDR) at L279, cause our toToken is not ETH.

After it at line 299 we wrap msg.value into WETH and then at lines 306 or 349 we swap WETH to WETH, even tho the bridge token was sgETH, the function still wraps it into WETH before swapping into WETH. Correct?

And another question, can you forward to lines of code where it handles swap from sgETH to WETH and WETH to WETH to see how the fees differ (sorry if you already sent it, cannot find).

Your understanding and description are right indeed.:

After it at line 299 we wrap msg.value into WETH and then at lines 306 or 349 we swap WETH to WETH, even tho the bridge token > was sgETH, the function still wraps it into WETH before swapping into WETH. Correct?

Second, the code does not do that. I was describing a hypothetical scenario trying to show that even if actually a fee had to be charged, the fee would be incorrect as WooPP charges different fees according to the token type. I added a link to the storage variables that handle this: tokenInfos storage, which points to a TokenInfo struct with a feeRate. By the way in feeRate don't get confused with the comment nex to it: // 1 in 100000; 10 = 1bp = 0.01%; max = 65535, this doesnt mean that the feeRate value is 1 in 100000, it is just how devs marked the decimals of precision.

So even in the hypothetical scenario that the code actually expects a swap (which I dont think it does by the resons provided earlier), the fee charged would be incorrect as it would be a WETH => WETH swap fee and not a sgETH => WETH swap fee.

[WangSecurity]

Then, as I've said earlier the only problem for me here is I don't really see it exceed small and finite amounts. It's not clear how to interpret this rule, cause hypothetically the fee will be around 0.05 - 0.1% as the Watson said earlier. Therefore, it seems to be small amout. But if it's 1M swap then the fee will be quite high, even tho it's only 0.05-0.1%.

Thus, if the head of judging decides that it should be valid, I will agree and accept the decision. I see where incorrect fees are taken the only problem is that I'm unsure we can say it exceeds small and finite amounts as the rules for medium say. Also, I guess it may be considered core functionality break, since we account not the fees we have to account. And for that, I also rely on the head of judging, cause I'm unsure how we should interpret the rules in that specific case.

And for the Watson, thank you for being so polite and calm, giving such thorough responses! It's a pleasure.

[fb-alexcq]

Okay. toToken could not be WETH in our server logic, and it won't happen. Also it's pretty hard to manually construct the param and interact with smart contract directly.

However, in technically aspect, when toToken is WETH, our current contract will fail the TX, and we need manually refund the user. So the issue posted here makes sense, but I'll let judges decide the priority level.

[WangSecurity]

Based on the comment by the sponsor above, as I understand the issue should indeed remain low/info since it'll be user mistake. Looping in the watson @CarlosAlegreUr if they can provide their opinion. But based on above, I believe it should remain low.

[CarlosAlegreUr]

Based on the comment by the sponsor above, as I understand the issue should indeed remain low/info since it'll be user mistake. Looping in the watson @CarlosAlegreUr if they can provide their opinion. But based on above, I believe it should remain low.

Based on @fb-alexcq comments. I understand that their server and/or UI won't have by default the option of WETH as toToken. So it won't be an worry if the user uses the official site I guess.

So only apps building on top of the protocol would be affected by this as there are no warnings or restrictions in the system's interface, docs or code to the WETH case. And, in my opinion, protocols that build on top of yours are also valid users doing valid actions and in this time, with invalid results.

I don't see it as a user mistake, I see it as a failed promise from the protocol's side that can cost, specially to apps building on top of the protocol, some money. So if user mistake is the reason for it to be a Low, as I don't think it is a user mistake, I don't think it is a Low.

[Evert0x]

Result: Medium Unique

---

Medium as it's clear that ANY token (including WETH) is supported, the unnecessary swap fee (0.05 - 0.1%) is pretty significant loss for users.

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• WangSecurity: accepted

[WangSecurity]

@CarlosAlegreUr want to again thank you for being very responsive and allocating so much time to correctly resolve this escalation. Great finding honestly, just wasn't sure how to correctly interpret it. Thank you very much again!

[sherlock-admin3]

The protocol team fixed this issue in the following PRs/commits: https://github.com/woonetwork/WooPoolV2/pull/124

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.