charles__cheerful - Medium6-UserReceivesLessThanMintToLimit

[sherlock-admin2]

charles__cheerful

medium

Medium6-UserReceivesLessThanMintToLimit

by CarlosAlegreUr

Summary

In 2 cross-chain swap scenarios users using the protocol in an expected and valid way can receive less tokens than the desired marked minimum limit thus unwillingly lose funds. Even if the users lose funds, I rate this a Medium and not a High because the protocol has some capability (though not capable in all-scenarios) of fighting back this problem in case third-party services decide to increase their fees.

Vulnerability Detail

When executing a cross-swap through the crossSwap() function at the WooCrossChainRouterV4 with sgETH as bridgeToken and the native coin like ETH as the toToken (in the code this translates to toToken == ETH_PLACEHOLDER_ADDR) the user can receive less than the specified minToAmount. It can also happen with any fromToken and a bridgeToken == toToken but bridgeToken != sgETH.

Impact

Users using the protocol in an expected and valid way will receive less tokens on cross-chain swaps than the desired marked minimum limit thus unwillingly lose funds.

Code Snippet

🚧 Note ⚠️: I didn't provide any executable code snippet as I couldnt find in the codebase a quickly reusable code to use the cross-chain router locally or on a testnet and I didn't have time to create one on my own. Instead I provide this clear and very detaild execution code as Proof Of Concept.

🔔 Notice ℹ️: The only requirement for this to happen is that (bridgeToken == sgEth && toToken == ETH_PLACEHOLDER_ADDR) || (fromToken == ANY_TOKEN && bridgeToken == toToken). Other simulated inputs in the explanation are specified to have a simpler execution flow to explain.

User calls crossSwap() at the WooCrossChainRouterV4 contract:

• Inputs:

uint256 refId = validRefId, // Any (only used off-chain so it doesn't interfere in on-chain logic flow)
address payable to, // Destination address in destination chain owned by User
SrcInfos memory srcInfos, // fromToken == bridgeAsset && bridgeToken == sgInfo.sgETHs(sgInfo.sgChainIdLocal())
DstInfos calldata dstInfos, // Same bridgeToken as in srcInfos and toToken == ETH_PLACEHOLDER_ADDR
Src1inch calldata src1inch, // Any valid pool in source chain of fromToken to bridgeToken
Dst1inch calldata dst1inch // Doesn't matter won't execute

All the checks on the source chain will pass and the stargate protocol will initiate the cross-chain bridging of the bridge asset.
To see this is true follow the (🟢1️⃣) numbers that explain the execution in the crossSwap() function below:

See detailed execution flow explanation 👁️

   function crossSwap(
        uint256 refId,
        address payable to,
        SrcInfos memory srcInfos,
        DstInfos calldata dstInfos,
        Src1inch calldata src1inch,
        Dst1inch calldata dst1inch
    ) external payable whenNotPaused nonReentrant {
        require(srcInfos.fromToken != address(0), "WooCrossChainRouterV3: !srcInfos.fromToken");
        require(
            dstInfos.toToken != address(0) && dstInfos.toToken != sgInfo.sgETHs(dstInfos.chainId),
            "WooCrossChainRouterV3: !dstInfos.toToken"
        );
        require(to != address(0), "WooCrossChainRouterV3: !to");

        uint256 msgValue = msg.value;
        uint256 bridgeAmount;
        uint256 fee = 0;

        {
            // Step 1: transfer
            // 🟢1️⃣ In our example fromToken == sgETH and not ETH_PLACEHOLDER_ADDR so we execute the else block
            if (srcInfos.fromToken == ETH_PLACEHOLDER_ADDR) {
                // code for when is native coin...
            } else {
                TransferHelper.safeTransferFrom(srcInfos.fromToken, msg.sender, address(this), srcInfos.fromAmount);
            }

            // 🟢2️⃣ In our example fromToken == srcInfos.bridgeToken so we execute the else block
            // Step 2: local swap by 1inch router
            if (srcInfos.fromToken != srcInfos.bridgeToken) {
                // 🟢3️⃣ Here goes the code for when srcInfos.fromToken != srcInfos.bridgeToken
                // this means a swap will be made whether through 1inch swap or WooRouterV2.
                // In the 1inch case the `bridgeAmount` will be returned with an already deducted fee
                // from 1inch, which we can't control.
                  if (src1inch.swapRouter != address(0)) {
                    TransferHelper.safeApprove(srcInfos.fromToken, address(wooRouter), srcInfos.fromAmount);
                    bridgeAmount = wooRouter.externalSwap(/*func args swaping fromAmount*/);
                    // 🟢4️⃣ Notice here is another fee, but this one is set by the protocol team so in case
                    // of needing to be lower so users are not damaged they could set it lower.
                    // Thus if taken this way `bridgeAmount` would be the amount received by the sawp from
                    // the fromAmount - 1inchFees - slippageOfTheSwap. Two factors the protocol cant control.
                    fee = (bridgeAmount * srcExternalFeeRate) / FEE_BASE;
                } else{
                    // WooRouterV2 swap code would go here...
                }
            } else {
                // 🟢5️⃣ Coming back to the execution flow of the example. This check passes as inputs set corretly 
                // but notice the `minToAmount` is not checked anywhere against `bridgeAmount`
                require(
                    srcInfos.fromAmount == srcInfos.minBridgeAmount, "WooCrossChainRouterV3: !srcInfos.minBridgeAmount"
                );
                bridgeAmount = srcInfos.fromAmount;
            }

            // 🟢6️⃣ Still `minToAmount` not checked
            require(
                bridgeAmount <= IERC20(srcInfos.bridgeToken).balanceOf(address(this)),
                "WooCrossChainRouterV3: !bridgeAmount"
            );
        }

        // Step 3: deduct the swap fee
        bridgeAmount -= fee;

        // Step 4: cross chain swap by StargateRouter
        // 🟢7️⃣ Notice!`bridgeAmount` is what we eventually bridge and there is no guarantees
        // after fees deduction that bridgeAmount >= minToAmount set by user. Sometimes exeution
        // flows, as seen, carry more fees than other but none makes sure bridgeAmount >= minToAmount.
        // So the user now would be sending a `bridgeAmount` > `minToAmount` expecting that, if
        // this would be the case something would stop and return him his funds. But it won't
        // happen as we will see now.
        // In _bridgeByStargate() there are also no checks of this kind:
        // you can see the code of this func here: 
        // https://github.com/sherlock-audit/2024-03-woofi-swap/blob/main/WooPoolV2/contracts/CrossChain/WooCrossChainRouterV4.sol#L219
        _bridgeByStargate(refId, to, msgValue, bridgeAmount, srcInfos, dstInfos, dst1inch);

        emit WooCrossSwapOnSrcChain(/*event params*/);
    }

    // 🟢8️⃣ When in destination chain sgReceive() will be activated and the Stargate router will
    // send the sgETH with the call to this function
    function sgReceive(
        uint16, 
        bytes memory, 
        uint256, 
        address bridgedToken,
        uint256 amountLD,
        bytes memory payload
    ) external {
        require(msg.sender == sgInfo.sgRouter(), "WooCrossChainRouterV3: INVALID_CALLER");

        (uint256 refId, address to, address toToken, uint256 minToAmount, Dst1inch memory dst1inch) =
            abi.decode(payload, (uint256, address, address, uint256, Dst1inch));

        // toToken won't be SGETH, and bridgedToken won't be ETH_PLACEHOLDER_ADDR
        // 🟢9️⃣ See we used sgEths so _handleNativeReceived() will be executed
        if (bridgedToken == sgInfo.sgETHs(sgInfo.sgChainIdLocal())) {
            // bridgedToken is SGETH, received native token
            _handleNativeReceived(refId, to, toToken, amountLD, minToAmount, dst1inch);
        } else {
            // 🟢1️⃣0️⃣ This won't be executed, once _handleNativeReceived() returns all is over
        }
    }

    // 🟢1️⃣1️⃣ And finally we are in _handleNativeReceived()
    function _handleNativeReceived(
        uint256 refId,
        address to,
        address toToken,
        uint256 bridgedAmount,
        uint256 minToAmount,
        Dst1inch memory dst1inch
    ) internal {
        address msgSender = _msgSender();
        if (toToken == ETH_PLACEHOLDER_ADDR) {
            // Directly transfer ETH
            // 🟢1️⃣2️⃣ bridgedAmount which was > minToAmount is transfered and the return will execute and all
            // will finish. The user will receive less toToken than he was willing to pay for.
            TransferHelper.safeTransferETH(to, bridgedAmount);
            emit WooCrossSwapOnDstChain(/*event params*/);
            return;
        }
        // 🟢1️⃣3️⃣ Rest of code wont be exeuted as toToken == ETH_PLACEHOLDER_ADDR.
        // more code...
    }

    // 🟢1️⃣4️⃣ And finally notice that if we would have made bridgeToken == toToken and taken the
    // _handleERC20Received() path, the same would have happened. No checks and directly transfered
    // the bridgedAmount to the user.
        function _handleERC20Received(/*func args*/) internal {
        address msgSender = _msgSender();
        if (toToken == bridgedToken) {
            TransferHelper.safeTransfer(bridgedToken, to, bridgedAmount);
            emit WooCrossSwapOnDstChain(/*event args*/);
        } 

See all code analyzed on the WooCrossChainRouterV4.sol.

Tool used

Manual Review

Recommendation

At crossSwap() before sending the bridged token to the user. If the bridgedToken can be easily compared with the toToken as with the sgETH example exposed, then compare minToAmount >= bridgedAmount

If not calculate the conversion of bridgeToken and toToken to the same accounting unit (USD for example) and compare them.

Dont allow cases were fees add up and minToAmount < bridgedAmount to keep executing as once cross-chain swap is sent users have no capacity to cancel it.

Duplicate of #85

[fb-alexcq]

Good, thanks for pointing it out : the source chain's bridge amount needs to be checked for minBridgeAmount after deducting the swap fee.

But on destination chain, we already check against minToAmount in WooRouter's swap or externalSwap.

[0xmer1in]

In 🟢5️⃣ and 🟢6️⃣, we wont check minToAmount because it belongs to dstInfos which will be used on the destination chain.

In 🟢7️⃣, you might misunderstanding the minToAmount and minBridgeAmount, but we got your point and will fix as:

        bridgeAmount -= fee;
+       require(bridgeAmount >= srcInfos.minBridgeAmount, "WooCrossChainRouterV4: !srcInfos.minBridgeAmount");

In 🟢1️⃣2️⃣, what you said bridgedAmount which was > minToAmount, might have a typo for >, should be <.

For 🟢1️⃣2️⃣ and 🟢1️⃣4️⃣, minToAmount is using in swap or externalSwap, the bridgedAmount is sent by sgInfo.sgRouter() and when sgReceive being triggered, that means the bridge action is done.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits:
woonetwork/WooPoolV2@151443b

[WangSecurity]

Inteded behaviour -> invalid

[CarlosAlegreUr]

Escalate

I don't know why the issue is invalidated, even though the issue has been acknowledged and addressed by the team, as we can see in the comments discussion.

Let's address the comment from 0xmeri1n point by point:

1. And 2:

In 🟢5️⃣ and 🟢6️⃣, we won't check minToAmount because it belongs to dstInfos which will be used on the destination chain.

In 🟢7️⃣, you might misunderstand the minToAmount and minBridgeAmount, but we got your point and will fix as:
        bridgeAmount -= fee;
+       require(bridgeAmount >= srcInfos.minBridgeAmount, "WooCrossChainRouterV4: !srcInfos.minBridgeAmount");

This points out that my recommendation to fix the issue is actually incomplete, not invalid though. This is because my recommendation misses a little detail which is, maybe the bridgeToken has different decimals on both chains thus comparing data from the srcInfos and dstInfos can be dangerous. My recommendation wouldn't work if the token had different decimals in the 2 chains being bridged, but this can easily be fixed by adding an extra parameter to the struct passed to the destination chain specifying the decimals of the token in the origin chain for example. Anyway, the fix the team made is simpler than that and valid.

My point here is that my recommendation was good but incomplete, is this why the finding is invalidated even though it's describing a real problem addressed by the team?

3. It is true, I made a typo at 🟢1️⃣2️⃣ and 0xmeri1n is right, I meant to write <. Actually, I made 2 typos:

In 🟢7️⃣ I did the same typo too: So the user now would be sending a bridgeAmount > minToAmount it should actually be a < sign, it's just a typo. But are typos reason enough to invalidate the finding? Which even with these 2 typos the dev team understood and fixed.

4. The dev is almost right when saying in his comment:

For 🟢1️⃣2️⃣ and 🟢1️⃣4️⃣, minToAmount is being used in swap or externalSwap, the bridgedAmount is sent by sgInfo.sgRouter() and when sgReceive being triggered, that means the bridge action is done.

But my issue doesn't mean that, the valid execution flow I provide is the one who directly transfers funds with no swap in the destination chain either because toToken == bridgedToken or toToken == ETH_PLACEHOLDER_ADDR. So then the swap code that actually detects and uses minToAmount won't execute. Instead, tokens will be directly sent using bridgeAmount as seen in 🟢1️⃣2️⃣ and 🟢1️⃣4️⃣ in my issue. Here are the exact links to the archive repo so you can check it is actually like that:

• When toToken == ETH_PLACEHOLDER_ADDR, see here Its clear that after the transfer nothing else will execute as there is a return; statement.

• When toToken == bridgedToken, see here. There is no return statement here but you can see that there is no code after the if statement.

• And here is the TransferHelper used for transfering so you can see the only code executed is a .call with some msg.value or a safeTransfer: see here

Thus bridgeAmount would be transferred without certainty that it is >= minToAmount. (Not anymore as the team fixed the issue in the commit but this is thanks to this issue being submitted).

If this last 4th point was the reason to invalidate the finding I think is wrong because the dev team missed or forgot by point 🟢1️⃣2️⃣ that in my issues' PoC inputs we are taking the code path of toToken == ETH_PLACEHOLDER_ADDR which wont exeute the destination swaps.

The judge says: Intended behaviour -> invalid. I don't see any intended behaviour being described by the devs comment, instead the devs did something and changed the code to fix it so it looks it was not intentional and actually an issue. I don't understand why this is invalidated and I would appreciate some clarifications, probably I'm misunderstanding something? are typos or the recommendation thing the reason?

[sherlock-admin2]

Escalate

I don't know why the issue is invalidated, even though the issue has been acknowledged and addressed by the team, as we can see in the comments discussion.

Let's address the comment from 0xmeri1n point by point:

1. And 2:

In 🟢5️⃣ and 🟢6️⃣, we won't check minToAmount because it belongs to dstInfos which will be used on the destination chain.

In 🟢7️⃣, you might misunderstand the minToAmount and minBridgeAmount, but we got your point and will fix as:
        bridgeAmount -= fee;
+       require(bridgeAmount >= srcInfos.minBridgeAmount, "WooCrossChainRouterV4: !srcInfos.minBridgeAmount");

This points out that my recommendation to fix the issue is actually incomplete, not invalid though. This is because my recommendation misses a little detail which is, maybe the bridgeToken has different decimals on both chains thus comparing data from the srcInfos and dstInfos can be dangerous. My recommendation wouldn't work if the token had different decimals in the 2 chains being bridged, but this can easily be fixed by adding an extra parameter to the struct passed to the destination chain specifying the decimals of the token in the origin chain for example. Anyway, the fix the team made is simpler than that and valid.

My point here is that my recommendation was good but incomplete, is this why the finding is invalidated even though it's describing a real problem addressed by the team?

3. It is true, I made a typo at 🟢1️⃣2️⃣ and 0xmeri1n is right, I meant to write <. Actually, I made 2 typos:

In 🟢7️⃣ I did the same typo too: So the user now would be sending a bridgeAmount > minToAmount it should actually be a < sign, it's just a typo. But are typos reason enough to invalidate the finding? Which even with these 2 typos the dev team understood and fixed.

4. The dev is right when saying in his comment:

For 🟢1️⃣2️⃣ and 🟢1️⃣4️⃣, minToAmount is being used in swap or externalSwap, the bridgedAmount is sent by sgInfo.sgRouter() and when sgReceive being triggered, that means the bridge action is done.

But my issue doesn't mean that, the valid execution flow I provide is the one who directly transfers funds with no swap in the destination chain either because toToken == bridgedToken or toToken == ETH_PLACEHOLDER_ADDR. So then the swap code that actually detects and uses minToAmount won't execute. Instead, tokens will be directly sent using bridgeAmount as seen in 🟢1️⃣2️⃣ and 🟢1️⃣4️⃣ in my issue. Here are the exact links to the archive repo so you can check it is actually like that:

• When toToken == ETH_PLACEHOLDER_ADDR, see here

• When toToken == bridgedToken, see here. There is no return statement here but you can see that there is no code after the if statement.

• And here is the TransferHelper used for transfering so you can see the only code executed is a .call with some msg.value or a safeTransfer: see here

Thus bridgeAmount would be transferred without certainty that it is >= minToAmount. (Not anymore as the team fixed the issue in the commit but this is thanks to this issue being submitted).

If this last 4th point was the reason to invalidate the finding I think is wrong because the dev team missed or forgot by point 🟢1️⃣2️⃣ that in my issues' PoC inputs we are taking the code path of toToken == ETH_PLACEHOLDER_ADDR which wont exeute the destination swaps.

The judge says: Intended behaviour -> invalid. I don't see any intended behaviour being described by the devs comment, instead the devs did something and changed the code to fix it so it looks it was not intentional and actually an issue. I don't understand why this is invalidated and I would appreciate some clarifications, probably I'm misunderstanding something? are typos or the recommendation thing the reason?

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]

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.

Escalate

[sherlock-admin2]

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.

Escalate

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]

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.

Escalate

#97 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 2 main reasons why this issue has been considered invalid:

1. Some say it is a UX problem and not a bug.
2. The amount lost is small and finite.

I totally disagree with the 1st one and I could agree on the second one but it depends on how Sherlok defines small and finite. The exacts reasons why I disagree are written after the summary of the issue written in the next section.

---

First lets summ up the issue (links to the code provided along the explanation): The issue is that in a valid cross-chain swap a user could end up receiving less token amount than the minToAmount specified.

The type of cross-chain swaps would be the following:

fromToken => Any token != bridgeToken

bridgeToken => any valid bridge token

toToken => The same as bridgeToken, native coin in case bridgeToken == sgEth

In this case it can happen than a user specifies a minBridgeAmount on srcChain of lets say 1000$, but as fromToken != bridgeToken a swap is carried out and it can happen that the 1inch swap returns the minBridgeAmount as it is the minimum amount parameter set.

Now after that, due to using an external swap the bridgeAmount, which is now equal to minBridgeAmount, will be deducted a fee proportional (a small percentage, like 1%) to the value of bridgeAmount. And after this deduction there is no check in code to see if the new bridgeAmount with fee deducted is still >= minBridgeAmount. So now we would have a 990$ bridgeAmount and a 1000$ minBridgeAmount. This check is precisely what the team added due to this issue being presented, you can see it here.

Now the cross-chain swap will be carried out sending a bridgeAmount < minBridgeAmount. And now, because the bridgeToken == toToken, in dstChain there won't be any swap and a transfer will be carried out with bridgeAmount as a paramter which is less than minBridgeAmount. Notice that the minToAmount parameter is never checked against so it won't stop the transfer either. Same happens in the transfer through the native coin in case bridgeToken == sgEth.

---

Now let me address the 2 reasons why this issue has been considered invalid and why I disagree with 1, potentially both:

1.-

The argument of UX is that if the user reads the code and sees that the fee check is never done they can always send more money so after the fee is discounted the desired amount of value arrives to the other chain. Lets say that you want 1000$ at least to be sent, but you know the minBridgeAmount won't protect you so you send 1020$ instead and eventually around 1009.8$ are sent so you actually receive at least the 1000$ desired.

My critique to this is was the following (copied from discord):

But is a user expected to read all the code and workflow of a contract before calling it? Then all bugs are invalid as users can prevent them reading the code.

A user is expected to see the interface and docs of the contract, or maybe not even that if using an UI and then fill up the args.

In the most informed case on this codebase the interface a user would see is a crossSwap() function with the follwoing args and no docs attached to it:

    uint256 refId,
    address payable to,
    SrcInfos memory srcInfos,
    DstInfos calldata dstInfos,
    Src1inch calldata src1inch,
    Dst1inch calldata dst1inch

Then it would see the SrcInfos, DstInfos, Src1inch and Dst1inch structs which are: linkt to codebase .

There are no comments on these args so the user would infer their functionality by their names, here I add in comments a logical conclusion from a users perspective: (only added in the args that apply to this issue)

    struct SrcInfos {
        address fromToken;
        address bridgeToken;
        uint256 fromAmount;
        uint256 minBridgeAmount; // 🟢 Min amount I expect to receive in dstChain (e.g. 4000 USDC)
    }

    // 🟢 Doesnt involve the minAmount received by 1 inch swap
    // as that is: srcInfos.minBridgeAmount 
    struct Src1inch { 
        address swapRouter;
        bytes data;
    }

    struct DstInfos {
        uint16 chainId;
        address toToken;
        address bridgeToken;
        uint256 minToAmount; // 🟢 Min amount I want to receive of the `to` token
        // Notice as a user I know toToken == brigeToken
        // in this crossSwap call so I set it to the same amount
        // as the minBridgeAmount (e.g. 4000 USDC)
        uint256 airdropNativeAmount;
        uint256 dstGasForCall;
    }

    // 🟢 Same doesnt involve the minBridgeAmount
    struct Dst1inch {
        address swapRouter;
        bytes data;
    }

You can see that a user would expect that the minToAmount is the same as the minBridgeAmount as the bridgeToken is the same as the toToken in this case. So the user would expect to receive the same amount in the destination chain as the amount sent in the source chain. But as I explained in the previous points this is not guaranteed and the user can receive less than the minToAmount specified.

The interface of the contract is misleading and has no warnings that minToAmount shouldnt be treated equally as minBridgeAmount when bridgeToken == toToken, which is a logical conclusion to make. But this warning is no where and the a valid user logical conclusion is that when bridgeToken == toToken the minToAmount should be the same as the minBridgeAmount as the user expects to receive the same amount in the destination chain as the amount sent in the source chain as bridgeToken becacuse the user wants to carry out a crossSwap where bridge token == to token.

Also, regarding to minBridgeAmount, from the users perspective this parameter should stop the tx the execution flow if less value is gonna be sent. And it wont as we saw. So as a user Im not obligated to read the whole code and then chose my values as inputs, then auditing makes no sense as users are expected to read code and always chose the perfect inputs. And thats not real, a realistic approach is users use an UI or the docs that define the contract interface and reach the conclusion that minBridgeAmount or minToAmount will protect them ina cross swap with toToken == bridgeToken, and this doesnt happen. So the minBridgeAmount input's function is to assure/protect against receiving less on the destination chain with a revert, and this is not assured

On top of all these like the team said: but we got your point and will fix as... : #97 (comment)

To conclude first point user shouldn't need to worry about the nitty-gritty details of the implementation, just read the interface and docs and expect things to work. And all the interfaces and docs present in the code so far tells the user that the minBridge amount and minToAmount will protect them in all cases when they actually don't. An extra line of code is required so the interface actually accomplishes what it suggests, and I think that is why the dev team fixed it, because it differed from the intended functionality of the feature as the min amounts are really meant to be understood the way I explained and thus protect you, but as they dont, the devs fixed it so the code doesn't have the issue. I really only see it as not a user issue, but an implementation issue.

2.-

My agreement with the argument of small and finite 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 using external swaps, which is a small % (lets say 1% or even 0.05%).

Now despite of that a 1% loss on let's imagine a bridged 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 terms then okay its 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 bridged 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 can accumulate loses due to the protocol and that number 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.

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

[WangSecurity]

I see the points from the watson and I think this issue can valid as broken core functionality.
The problem is that the user can indeed receive less than minTo limit and it's similar to issue #85 .

On the other hand, I believe the loss doesn't exceed small and finite amounts, therefore, should remain low. I'm unsure what is the main argument for judging such issues, therefore, escalated myself and leaving the decision to the head of judging.

[Czar102]

@CarlosAlegreUr @WangSecurity pardon if I'm misunderstanding, but doesn't the comment

But on destination chain, we already check against minToAmount in WooRouter's swap or externalSwap.

invalidate this?

[WangSecurity]

The problem is that in edge cases, when we get into _handleNativeReceived or _handleERC20Received we don't trigger swap or externalSwap (for rxample, when toToken == ETH_PLACEHOLDER_ADDR for native or toToken == bridgedToken for ERC20).

The problem here is that fromToken != bridgeToken and == toToken. Therefore, we first swap the token here. Or a couple of lines below in a regular swap. minToAmount is checked there. After that, we deduct the fees from that amount and till the moment when the user receives the funds we don't check if it's >= minToAmount after deducting the fees (no swap on destination chain, cause bridgeToken == toToken).

Thus, the only loss here is that fee, therefore, I think it doesn't exceed the rule of smal and finite amounts and there is no guarantee that this amount will indeed be lower after deducting the fees.

[CarlosAlegreUr]

@CarlosAlegreUr @WangSecurity pardon if I'm misunderstanding, but doesn't the comment

But on destination chain, we already check against minToAmount in WooRouter's swap or externalSwap.

invalidate this?

First, Thanks for your time and patience and letting me explain myself.

@Czar102 , I didn't read Issue #85 before, but now that @WangSecurity pointed it out in this reply, I've read it and it is what I've been trying to explain all this time.

#85 has been rewarded, I think my issue should be considered a duplicate of it.

I admit that #85 explains better than me the issue, hope that is not enough reason to not to consider mine a duplicate.

We both address the consequences of bridgedAmount not being checked after external fee is deducted in srcChain. The only "difference" is that in my issue I focused on the consequences that that has in dstChain when bridgeToken == toToken. Which are that eventually this code is executed and transfers bridgeAmount to the user, which can be lower than the user specified minBridgeAmount as #85 explains perfectly specially on its Impact section:

---

(copied from issue #85)
Impact
But as can be noticed, an external swap fee is deducted from the bridgeAmount after the swap is done via an external aggregator (1inch aggregator) and after checking that the bridgeAmount is sufficient as per detrmined by the user ( > srcInfos.minBridgeAmount), and this might result in the bridgeAmount being less than required by the user srcInfos.minBridgeAmount.

---

The thing is that because I was thinking on a clear case where the very same bridgeAmount was transfered to explain the problem, I focused to describe on my PoC the execution flow for when bridgeToken == toToken that clearly ends up transfering the lesser amount of tokens to the user in dstChain.

But the core issue I was explaining is the same, the lack of checks for bridgedAmout in srChain that leads to that amout being bridged or even transfered to the final user in dstChain. Only thing is that in my issue writing I was comparing it with the end minToAmount due to in my head already thinking on the dstChain effects as then minBridgeAmount == minToAmount. But the core issue of bridgedAmount being substracted fees in srcChain thus could lead to < than min amounts desired is what I was explaining, just like #85.

Even though my writing was not the most clear one, hope this explanation helps to understand why I think my issue is a duplicate of #85.

[Czar102]

I agree, this seems to indeed be a duplicate of #85. @WangSecurity do you agree with duplication?

[WangSecurity]

Yes, I agree, sorry to both of you for wasting your time, haven't noticed it indeed should be a duplicate.

[CarlosAlegreUr]

Yes, I agree, sorry to both of you for wasting your time, haven't noticed it indeed should be a duplicate.

Don't worry you have been very hard-working answering lots of questions during the judging, you did your job well and it is obvious you tried your best! Good job @WangSecurity :D

[WangSecurity]

thank you very much 👉 👈

[Czar102]

Result:
Medium
Duplicate of #85

I agree with #97 (comment) ;)

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• WangSecurity: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.