Function `exactInputStableSwap` gives the possibility to send value when exchanging between tokens ERC20

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x95114a0fdaca1035f50fa2d91637300c886cee100bb49498183aadf3723ba7f7 Severity: medium

Description: Description:

Note: The native token is ROSE, like ETH in ethereum

The exactInputStableSwap function of the StableSwapRouter contract has the modifier payable

This enables the user to send ROSE as a value to the contract when exchanging token A for token B (without using ROSE on the way), it would be locked in the contract and could be stolen by anyone

Attack Scenario/Proof of Concept (PoC):

For example if Alice call the function exactInputStableSwap with:

router.exactInputStableSwap
    {value: 1 ether}(   // msg.value
    [WROSE, TokenA],    // path
    [2, 2],             // flag
    1 ether             // amountIn
    0                   // amountOutMin
    address(Alice)      // to
);

As a result of the confusion between WROSE and ROSE, Alice sends 1 ether as value, the exchange takes place and Alice gets her A token amount and the 1 ROSE that Alice sent remains within the StableSwapRouter contract

Another user, Kane sends:

router.exactInputStableSwap(
    [ROSE, TokenA],            // path
    [2, 2],                    // flag
    Constants.CONTRACT_BALANCE // amountIn
    0                          // amountOutMin
    address(Kane)              // to
);

With this Kane uses the balance of the contract (where the 1 ROSE belongs to Alice) to exchange for the A token and finally Kane gets the 1 ROSE on the A token

Recommendation:

One way to patch this would be to check if path[0] is different from ROSE the msg.value should be 0:

@@ -135,6 +135,7 @@ contract StableSwapRouter is
             if(srcToken==ROSE){
                 require(msg.value>=amountIn, "Invalid msg.value");
             }else{
+                require(msg.value == 0, "msg.value should be 0");
                 pay(srcToken, msg.sender, address(this), amountIn);
             }
         }

[omega-audits]

This is a user error - the user can send tokens by mistake to the contract in many different ways, an dit is not the responsibility of the contract to protect the user against themselves

[rotcivegaf]

The example of "can send tokens by mistake to the contract in many different ways" is not applicable. I am talking about the native ROSE, which is not a token.

The router is the gateway component to all users as well as to contracts or protocols that want to integrate Thorn, so it must be the safest, simplest, most secure component, the bug is not about sending ROSE to the contract, the bug is in the exactInputStableSwap function.

[omega-audits]

Your fix is for the scenario where someone wants to swap an WROSE token, and sends both WROSE and ROSE with the transaction. How is that not a user error?