hats-finance / Thorn-protocol-0x1286ecdac50215a366458a14968fbca4bd95067d

GNU General Public License v3.0
0 stars 0 forks source link

attacker can frontrunn and swap user funds #70

Open hats-bug-reporter[bot] opened 3 weeks ago

hats-bug-reporter[bot] commented 3 weeks ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x45e7adf08e0fbb26088e37a95a27fce428e9343efc0164a1d90b60a9ec0bf0c4 Severity: high

Description: Description\ exactInputStableSwap facilitates the swapping of tokens within a stable swap pool,here user can use this function in two ways i.e

1. Transferring assets first: the user can first transfer the assets to the contract and then call exactInputStableSwap with amountIn  to Constants.CONTRACT_BALANCE. This approach uses the transferred balance of the contract for the srcToken as the input amount

2. Direct Call with Actual Amount: here user can call exactInputStableSwap with a specific amountIn value. In this case, the function will transfer the specified amountIn from the user to the contract using

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

issue: now here for the 1st case,the attacker waits for the user to transfer the assets first and when the user call the exactInputStableSwap function with amountIn == Constants.CONTRACT_BALANCE the attacker frontrunn and calls this function and swaps the user balance freely and the legitimate user get Dosed

Attack Scenario\

Alice's Intent:

Attacker's Strategy:

Outcome:

Attachments

  1. Proof of Concept (PoC) File

    
    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);
    
        if (dstToken == ROSE) {
            amountOut = address(this).balance;
        } else {
            amountOut = IERC20(dstToken).balanceOf(address(this));
        }
    
        require(amountOut >= amountOutMin);
    
        // find and replace to addresses
        if (to == Constants.MSG_SENDER) to = msg.sender;
        else if (to == Constants.ADDRESS_THIS) to = address(this);
    
        if (to != address(this)){
            pay(dstToken, address(this), to, amountOut);
        }
    
        emit StableExchange(msg.sender,amountIn,path[0],amountOut,path[path.length - 1], to);
    }


2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->
* allow users to transfer the tokens in the function itself
omega-audits commented 2 weeks ago

This is a duplicate of #22 and #19. In #22 there is a clear explanation why your scenario is not one that shoul dever occur. Specifically

Alice transfers 1000 ROSE to the StableSwapRouter contract.

Alice should simply not do this. Why would she? If she does, she will lose her money, as described by your issues.