Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

users ROSE will be trapped in StableSwapRouter if the pool contract stops accepting ROSE #101

Open
hats-bug-reporter bot opened this issue Oct 13, 2024 · 1 comment
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

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

Description:
Description
When a user attempts to swap ROSE tokens using the exactInputStableSwap function in the StableSwapRouter contract, there is a issue i.e where ROSE tokens will be trapped in the contract(and may be swapped by another user). This occurs if the pool contract does not accept ROSE or stops accepting ROSE

Attack Scenario
lets understand with example

Attachments

  1. Proof of Concept (PoC) File

Here, let's say a user calls exactInputStableSwap with srcToken set to ROSE.

function exactInputStableSwap(
        address[] calldata path,
        uint256[] calldata flag,
        uint256 amountIn,
        uint256 amountOutMin,
        address to
    ) external payable nonReentrant returns (uint256 amountOut) {
        require(!isKill, "Contract is killed");
        address srcToken=  path[0];
        address dstToken = path[path.length - 1];
        
        // use amountIn == Constants.CONTRACT_BALANCE as a flag to swap the entire balance of the contract
        
        bool hasAlreadyPaid;
        if (amountIn == Constants.CONTRACT_BALANCE) {
            hasAlreadyPaid = true;
            if(srcToken==ROSE){
                amountIn = address(this).balance;
            }else{
                amountIn = IERC20(srcToken).balanceOf(address(this));
            }
        }

        if (!hasAlreadyPaid) {
            if(srcToken==ROSE){
                require(msg.value>=amountIn, "Invalid msg.value");
            }else{
                pay(srcToken, msg.sender, address(this), amountIn);
            }
        }
        _swap(path, flag);

Now, this function calls the _swap() function internally

    function _swap(address[] memory path, uint256[] memory flag) private {
        uint256 amountIn_;
        require(path.length - 1 == flag.length);
        for (uint256 i; i < flag.length; i++) {
            (address input, address output) = (path[i], path[i + 1]);
            (uint256 k, uint256 j, address swapContract) = SmartRouterHelper
                .getStableInfo(stableSwapFactory, input, output, flag[i]);
            if (input == ROSE) {
                amountIn_ = address(this).balance;
                IStableSwap(swapContract).exchange{value: amountIn_}(k, j, amountIn_, 0);
            }
            if (input != ROSE) {
                amountIn_ = IERC20(input).balanceOf(address(this));
                TransferHelper.safeApprove(input, swapContract, amountIn_);
                 IStableSwap(swapContract).exchange(k, j, amountIn_, 0);
            }
        }
    }

,and in the swap function, as we can see, it calls the exchange function at i.e., IStableSwap(swapContract).exchange{value: amountIn}(k, j, amountIn_, 0);.

 function exchange(
        uint256 i,
        uint256 j,
        uint256 dx,
        uint256 min_dy
    ) external payable nonReentrant {
        require(!is_killed, "Killed");
        if (!support_ROSE) {
            require(msg.value == 0, "Inconsistent quantity"); // Avoid sending ROSE by mistake.
        }

Now, the exchange() function will be invoked here. This function checks i.e., if the pool contract does not accept ROSE or it stops accepting ROSE, then the function will revert. The user's ROSE tokens will be trapped in the StableSwapRouter contract as the call to exchange function is reverted.

steps to reproduce

  • The user sends a certain amount of ROSE to the StableSwapRouter contract(as contract has logic for this). This increases the contract's balance of ROSE.
  • The user calls the exactInputStableSwap function with srcToken set to ROSE and amountIn set to Constants.CONTRACT_BALANCE.
  • This indicates that the user wants to swap the entire balance of ROSE held by the contract.
  • The function proceeds to call the _swap() function, passing the path and flags for the swap.
  • Within _swap(), the exchange function is called on the swapContract with the current balance of ROSE as msg.value.
  • The exchange function checks if the pool supports ROSE using the support_ROSE flag
  • If support_ROSE is false, the function reverts with "Inconsistent quantity".
  • If the exchange function reverts, the entire transaction is rolled back. However, since the ROSE was already in the contract, it remains there.
  • The ROSE tokens remain in the StableSwapRouter contract because the transaction was reverted after the function call and anyone call swap them.
  1. Revised Code File (Optional)
  • Implement a check before initiating the swap to verify if the pool supports ROSE. If not, revert the transaction early to prevent the trap of ROSE.
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Oct 13, 2024
@omega-audits
Copy link

There are many other issues submitted that start with scenarios where users send tokens directly to the router and then lose their tokens. This comment explains why this is not an issue: #22 (comment)

@omega-audits omega-audits added duplicate This issue or pull request already exists invalid This doesn't seem right and removed duplicate This issue or pull request already exists labels Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant