howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

Whenever a user deposits a fee on transfer token like PAXG or STA (both are whitelisted), there are a few ways he could get his funds locked into the contract.

Proof of Concept

Fee on transfer tokens are tokens that charge a fee on transfer making recipients receive less than the sent amount.

This creates a few possible issues for the contract.

Imagine the following scenario:

User deposits PAXG (an allowed token) into the contract using depositWithExpiry() specifying that he wants to call transferOutAndCallV5() using the memo input parameter

This is the part of _transferOutAndCallV5() responsible for swapping from a token to another token:

      _vaultAllowance[msg.sender][aggregationPayload.fromAsset] -= aggregationPayload.fromAmount;

      (bool transferSuccess, bytes memory data) = aggregationPayload.fromAsset.call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
            aggregationPayload.fromAmount
          )
        );

      require(
        transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
        "Failed to transfer token before dex agg call"
      );

      (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
        )
      );

We transfer the aggregationPayload.fromAmount of the PAXG asset to the target aggregator contract and the target contract will receive less than the amount specified
We then call swapOutV5 on the target contract with that same fromAmount and that contract will conduct the swap
Since the contract would have received less than the specified amount, this will definitely revert upon calling Uniswap or any other protocol they are using for the swap as the fromAmount specified is not actually available in the aggregator contract

Now, the funds will be locked in the aggregator contract until they are manually rescued.

Tools Used

Manual Review

Recommended Mitigation Steps

Handle such cases appropriate or disallow such tokens altogether.

Assessed type

Under/Overflow

c4-judge commented 4 months ago

trust1995 changed the severity to 2 (Med Risk)