sherlock-admin2 commented 7 months ago

GiuseppeDeLaZara

medium

`sgReceive` can be DoSed for token that don't allow setting non-zero to non-zero allowances

Summary

sgReceive can be DoSed for receiving tokens that don't allow setting non-zero to non-zero allowances. The most prominent example is USDT token. This results in the user's tokens being stuck in the WooCrossChainRouterV4 contract.

Vulnerability Detail

USDT token doesn't allow setting non-zero to non-zero allowances. This means if the allowance is non-zero after stargateRouter transfer, next transfers will revert.

function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

    // To change the approve amount you first have to reduce the addresses`
    //  allowance to zero by calling `approve(_spender, 0)` if it is not
    //  already 0 to mitigate the race condition described here:
    //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
    require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

If we observe the sgReceive function logic with bridgedToken being ERC20:

    function sgReceive(
        uint16, // srcChainId
        bytes memory, // srcAddress
        uint256, // nonce
        address bridgedToken,
        uint256 amountLD,
        bytes memory payload
    ) external {
        require(msg.sender == sgInfo.sgRouter(), "WooCrossChainRouterV3: INVALID_CALLER");

        // make sure the same order to abi.encode when decode payload
        (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
        if (bridgedToken == sgInfo.sgETHs(sgInfo.sgChainIdLocal())) {
            // bridgedToken is SGETH, received native token
            _handleNativeReceived(refId, to, toToken, amountLD, minToAmount, dst1inch);
        } else {
            // bridgedToken is not SGETH, received ERC20 token
            _handleERC20Received(refId, to, toToken, bridgedToken, amountLD, minToAmount, dst1inch);
        }
    }

    function _handleERC20Received(
        uint256 refId,
        address to,
        address toToken,
        address bridgedToken,
        uint256 bridgedAmount,
        uint256 minToAmount,
        Dst1inch memory dst1inch
    ) internal {
        address msgSender = _msgSender();

        if (toToken == bridgedToken) {
            TransferHelper.safeTransfer(bridgedToken, to, bridgedAmount);
        } else {
            // Deduct the external swap fee
            uint256 fee = (bridgedAmount * dstExternalFeeRate) / FEE_BASE;
            bridgedAmount -= fee;

>>>>            TransferHelper.safeApprove(bridgedToken, address(wooRouter), bridgedAmount);
            if (dst1inch.swapRouter != address(0)) {
                try
                wooRouter.externalSwap(
                    dst1inch.swapRouter,
                    dst1inch.swapRouter,
                    bridgedToken,
                    toToken,
                    bridgedAmount,
                    minToAmount,
                    payable(to),
                    dst1inch.data
                )
                returns (uint256 realToAmount) {
                } catch {
                    bridgedAmount += fee;
                    TransferHelper.safeTransfer(bridgedToken, to, bridgedAmount);
                }
            } else {
                try wooRouter.swap(bridgedToken, toToken, bridgedAmount, minToAmount, payable(to), to) returns (
                    uint256 realToAmount
                ) {

                } catch {
                    TransferHelper.safeTransfer(bridgedToken, to, bridgedAmount);
                }
            }
        }
    }

In case toToken is not equal to bridgedToken, the externalSwap function is called.
Before the externalSwap, approval is granted to the wooRouter contract for the bridgedAmount.
If the externalSwap fails, the bridgedAmount is transferred back to the to address, but there is a dangling allowance left for the wooRouter contract.

DoS scenario is the following:

Alice initiates a cross-chain swap from Ethereum to BSC for 1 USDT.
When she receives the USDT on BSC, she wants to swap it for USDC.
A griever noticed this transaction, initiates a cross-chain swap from Ethereum to BSC for 2 USDT, but specifies the dstGasForCall as 0.

His intention is to have his transaction fail inside the sgReceive and to get saved in the cachedSwapLookup mapping, so he can front-run Alice's transaction.

## Router.sol

function clearCachedSwap(
    uint16 _srcChainId,
    bytes calldata _srcAddress,
    uint256 _nonce
) external {
    CachedSwap memory cs = cachedSwapLookup[_srcChainId][_srcAddress][_nonce];
    require(cs.to != address(0x0), "Stargate: cache already cleared");
    // clear the data
    cachedSwapLookup[_srcChainId][_srcAddress][_nonce] = CachedSwap(address(0x0), 0, address(0x0), "");
    IStargateReceiver(cs.to).sgReceive(_srcChainId, _srcAddress, _nonce, cs.token, cs.amountLD, cs.payload);
}

He front-runs Alice's transaction and initiates the clearCachedSwap function to execute his payload.
His call reverts in the externalSwap function, but there is approval left for the wooRouter contract for the bridgedAmount of 2 USDT.
Now Alice's transaction will revert as the allowance is non-zero.
Alice's tokens are stuck in the WooCrossChainRouterV4 contract.

Impact

When a user is transferring USDT or any other token that doesn't allow setting non-zero to non-zero allowances, there is a DoS attack vector, resulting in user's tokens being stuck in the WooCrossChainRouterV4 contract. The likelihood of this happening is high as illustrated in the DoS scenario above. The impact is medium. I'm marking this as a medium-severity issue.

Code Snippet

https://github.com/sherlock-audit/2024-03-woofi-swap/blob/6518569/WooPoolV2/contracts/CrossChain/WooCrossChainRouterV4.sol#L416

Tool used

Manual Review

Recommendation

You can utilize the low-level call to imitate the same behavior as with try/catch but if externalSwap fails the approval is reverted inside the external call.

## WooCrossChainRouterV4.sol

+    function callExternalSwap(
+        address approveTarget,
+        address swapTarget,
+        address fromToken,
+        address toToken,
+        uint256 fromAmount,
+        uint256 minToAmount,
+        address payable to,
+        bytes calldata data
+    ) public {
+        require(msg.sender == address(this));
+        TransferHelper.safeApprove(fromToken, address(wooRouter), fromAmount);
+        wooRouter.externalSwap(approveTarget, swapTarget, fromToken, toToken, fromAmount, minToAmount, to, data);
+    }
+
     function _handleERC20Received(
         uint256 refId,
         address to,
@@ -413,10 +428,10 @@ contract WooCrossChainRouterV4 is IWooCrossChainRouterV3, Ownable, Pausable, Ree
             uint256 fee = (bridgedAmount * dstExternalFeeRate) / FEE_BASE;
             bridgedAmount -= fee;

-            TransferHelper.safeApprove(bridgedToken, address(wooRouter), bridgedAmount);
             if (dst1inch.swapRouter != address(0)) {
-                try
-                    wooRouter.externalSwap(
+                (bool success,) = address(this).call(
+                    abi.encodeWithSelector(
+                        this.callExternalSwap.selector,
                         dst1inch.swapRouter,
                         dst1inch.swapRouter,
                         bridgedToken,
@@ -426,37 +441,24 @@ contract WooCrossChainRouterV4 is IWooCrossChainRouterV3, Ownable, Pausable, Ree
                         payable(to),
                         dst1inch.data
                     )
-                returns (uint256 realToAmount) {
-                    emit WooCrossSwapOnDstChain(
-                        refId,
-                        msgSender,
-                        to,
-                        bridgedToken,
-                        bridgedAmount,
-                        toToken,
-                        toToken,
-                        minToAmount,
-                        realToAmount,
-                        dst1inch.swapRouter == address(0) ? 0 : 1,
-                        fee
-                    );
-                } catch {
+                );
+                if (!success) {
                     bridgedAmount += fee;
                     TransferHelper.safeTransfer(bridgedToken, to, bridgedAmount);
-                    emit WooCrossSwapOnDstChain(
-                        refId,
-                        msgSender,
-                        to,
-                        bridgedToken,
-                        bridgedAmount,
-                        toToken,
-                        bridgedToken,
-                        minToAmount,
-                        bridgedAmount,
-                        dst1inch.swapRouter == address(0) ? 0 : 1,
-                        0
-                    );
                 }
+                emit WooCrossSwapOnDstChain(
+                    refId,
+                    msgSender,
+                    to,
+                    bridgedToken,
+                    bridgedAmount,
+                    toToken,
+                    bridgedToken,
+                    minToAmount,
+                    bridgedAmount,
+                    dst1inch.swapRouter == address(0) ? 0 : 1,
+                    0
+                );

A similar pattern can be applied for the wooRouter.swap function call below.

sherlock-admin4 commented 7 months ago

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

WangAudit commented:

I know contest's README says any erc20 token will be used; but it doesn't specify about USDT and weird approvals haven't been explicitly mentioned; therefore; has to mark it as low; but a good catch tbh;; for reference you can also look at issue 29 in the recent telcoin contest (basically the same issue but with BNB)

windhustler commented 6 months ago

In the context of Stargate transfers USDT is not some obscure token but rather one of three tokens that can be transferred on Mantle for instance. See this resource: https://stargateprotocol.gitbook.io/stargate/developers/pool-ids

On other networks that the protocol supports, there is also only a handful of tokens that can be transferred via Stargate. So it's safe to assume there will be users that will want to transfer USDT. My issue explains how can this be exploited to lock those tokens.

Haxatron commented 6 months ago

Escalate

See above. Also the README suggests that any ERC20 token will be used and USDT is not an obscure token.

sherlock-admin2 commented 6 months ago

Escalate

See above. Also the README suggests that any ERC20 token will be used and USDT is not an obscure token.

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.

InfectedIsm commented 6 months ago

Please also consider my issue (I've escalated mine), which bring another PoV why it probably was expected to be implemented: https://github.com/sherlock-audit/2024-03-woofi-swap-judging/issues/130

WangSecurity commented 6 months ago

I see and understand points by the watsons above and thank you very much for raising this issue. But I'm still of the opinion, that according to rules this report should remain invalid: contest's README say "any" ERC20 token will be used, which means these don't have any weird traits (words of the Head of Judging), therefore, I believe this report should remain low, even tho I agree and understand that in fact it's dangerous for the protocol, under the rules it should remain low.

windhustler commented 6 months ago

Sherlock's rules were always judged complimentary with the context of the protocol. There are countless examples in other contests where weird-erc20s are excluded, but due to the specific nature of the issue, it gets accepted as a valid issue.

I'm just going to re-iterate the points from above. USDT is 1 out of 3 tokens that you can bridge with Stargate on a supported chain. This fact should be enough to consider this issue valid.

@WangSecurity I mean no disrespect, but can someone with higher seniority take a look at this one? @Czar102?

WangSecurity commented 6 months ago

The head of judging will decide should it remain invalid or be valid, it's not me who decides here at this point.

And as I've said I don't say your issue is invalid, I don't say it doesn't deserve a reward. I just expressed my reasoning why I judged it as invalid.

Thank you for pointing it out, again I have nothing against your issue or anything personal, just judging according to rules.

DevHals commented 6 months ago

Hi, issue #84 is a duplicate of this one in case it got validated,

Czar102 commented 6 months ago

The rules are clear that any weird traits need to be explicitly mentioned in the readme, hence I believe this issue should stay as is. Planning to reject the escalation.

windhustler commented 6 months ago

An identical issue (Stargate and USDT) was ruled valid in the Tapioca contest: https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/94. Rules around not accepting all weird-erc20s were even more explicitly stated there: https://audits.sherlock.xyz/contests/170

Haxatron commented 6 months ago

If the escalation is rejected it will be a very disappointing decision...

Given that this is a serious bug concerning a non-obscure token which should definitely be fixed.

Watson should not become a lawyer and read every single fine print in the rulebook.

It is very annoying to have a valid bug get rejected because of XYZ reason due to a technicality in your rules and it just drives away talented Watsons from your platform.

Consider updating your rulebook to allow for discretion.

Haxatron commented 6 months ago

Ok I would like to retract partially my comments above, after further research on README it says:

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts? NO.

However, I still feel Watsons can get confused, because it also says

Which ERC20 tokens do you expect will interact with the smart contracts? any

I think the README can do a better job at communicating this. And especially having a comment saying non-standard token issues will not be accepted.

WangSecurity commented 6 months ago

No one argues here that the severity of this bug is high or not, the problem is that we have to judge it as invalid according to rules as Haxatron mentioned above. I also agree that README can be better regarding erc20 tokens and as we see in xKeeper contest, the README is indeed better.

Regarding the comment from Windhustler about Tapioca, Tapioca also didn't mention tokens with non-standard approvals. But, on the other hand, it says that "1 of 1 tokens with special traits" shouldn't be valid mediums, when non-standard approvals is quite common.

windhustler commented 6 months ago

Tapioca explicitly stated what weird traits will be considered (balance changing, rebasing (WSTETH), pauseable (RWA), upgradable, flash mintable, low decimal, and high decimal) and there was no mention of non-standard approvals. I would disagree that non-standard approvals are common. Only a few popular tokens have this behavior.

After all, if it is a common pattern why this isn't an accepted issue then?

I understand your position, but you also have to give some credit to the wardens here. We can't navigate like lawyers around your rules and not report issues just because they might be invalidated. This is a security review not a debate over validity.

Czar102 commented 6 months ago

If the escalation is rejected it will be a very disappointing decision...

Given that this is a serious bug concerning a non-obscure token which should definitely be fixed.

Watson should not become a lawyer and read every single fine print in the rulebook.

@Haxatron The fact that some token gained some adoption doesn't mean that it's weirdness must bloat everyone's token-agnostic code. The rules are exactly for this kind of things, for low-value low-hanging fruits, of which the team is usually aware, not to be reported, so that we can focus on finding and rewarding serious bugs.

I hid comments that are irrelevant to this judgment as they discuss a judgment in a different contest. Past judgments are not a source of truth, and in this case there seems to be a misjudgment on that issue. Unfortunately, it hasn't been escalated.

(hidden comment) I understand your position, but you also have to give some credit to the wardens here. We can't navigate like lawyers around your rules and not report issues just because they might be invalidated. This is a security review not a debate over validity.

@windhustler Watsons don't have to be laywers, this is why you can submit 4 invalid issue per every accepted issue – you have a large room for error. But you can also get to know the rules and save yourself some work writing the reports and arguing.

I stand by my initial decision to reject the escalation and leave the issue as is

Haxatron commented 6 months ago

@Haxatron The fact that some token gained some adoption doesn't mean that it's weirdness must bloat everyone's token-agnostic code. The rules are exactly for this kind of things, for low-value low-hanging fruits, of which the team is usually aware, not to be reported, so that we can focus on finding and rewarding serious bugs.

Agreed. And thats why my 2nd comment suggested that the README should have been clearer, which seems to be the case for the new contests like xKeeper.

Thanks for your work in resolving this escalation. Won't comment any further.

Czar102 commented 6 months ago

Result: Low Unique

sherlock-admin3 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

haxatron: rejected

sherlock-audit / 2024-03-woofi-swap-judging

GiuseppeDeLaZara - `sgReceive` can be DoSed for token that don't allow setting non-zero to non-zero allowances #127

`sgReceive` can be DoSed for token that don't allow setting non-zero to non-zero allowances

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-03-woofi-swap-judging

GiuseppeDeLaZara - `sgReceive` can be DoSed for token that don't allow setting non-zero to non-zero allowances #127

sgReceive can be DoSed for token that don't allow setting non-zero to non-zero allowances

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

`sgReceive` can be DoSed for token that don't allow setting non-zero to non-zero allowances