code-423n4 / 2024-06-thorchain-findings

6 stars 3 forks source link

If the fromAsset in the _transferOutAndCallV5 function is a fee-on token, the exchange may fail. #58

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L347-L360

Vulnerability details

Impact

If the fromAsset in the _transferOutAndCallV5 function is a fee-on token, the exchange may fail.

Proof of Concept

If fromAsset in the _transferOutAndCallV5 function is a charging token, first transfer the funds to the target, and the amount is fromAmount. Since it is a fee-based token, the amount received by target must be less than fromAmount.

Then call the target for redemption, and the incoming parameter is fromAmount.

      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

In the target, the swapOutV5 function also uses fromAmount for exchange.

        (bool aggSuccess, ) = address(swapRouter).call(
          abi.encodeWithSignature(
            "swapExactTokensForETH(uint256,uint256,address[],address,uint256)",
            fromAmount,
            amountOutMin,
            path,
            recipient,
            type(uint).max // Assuming using the maximum uint256 value as the deadline
          )
        );
        require(aggSuccess, "swapExactTokensForETH failed");

If there are no extra funds in the target, the exchange will fail. Even if there are excess funds, this will occupy other funds for exchange.

Tools Used

manual

Recommended Mitigation Steps

It is recommended to determine the fromAmount when exchanging based on the actual number of tokens transferred to the target.

Assessed type

ERC20

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory