howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The function can't ever be executed as intended.
Potential loss of user funds

Proof of concept

The _transferOutAndCallV5() is an internal function that is used in transferOutAndCallV5() and batchTransferOutAndCallV5() which allows a THOR Vault to transfer an amount of native coin or ERC20 tokens to a recipient by first swapping the corresponding amount using an aggregator.

Here is the code snippet responsible for swapping 1 asset to another before sending the output asset to the recipient :

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

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

// send ERC20 to aggregator contract so it can do its thing
(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"
);

// add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
// call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
(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
    )
);

First, the THORChain_Router contract (that holds the asset to be swapped) transfers the input tokens (aggregationPayload.fromAsset) to the aggregator (aggregationPayload.target) using a low-level call.

At this point, the aggregator holds the input assets and new low-level call is made to the aggregator to swap these tokens to the output assets using swapOutV5().

When dealing with fee-on-transfer and rebase tokens, the actual number of tokens received after a transfer() might not be equal to the amount of tokens sent.

This means the call to swapOutV5() on the aggregator will most likely fail because it will attempt to swap an amount of tokens that exceeds balance of the THORChain_Router.

However, the swap won't revert the transaction which will end-up successfully (this is intended as stated by the comment above the call to swapOutV5()).

To be clearer, here is an example scenario :

Vault executes the transferOutAndCallV5() function with these parameters :
- aggregationPayload.fromAsset : TKNA which is a fee-on-transfer token
- aggregationPayload.toAsset : ETH (native)
- aggregationPayload.fromAmount: 1000 (TKNA)
The transfer() is executed and sends the 1000 TKNA to the aggregator
The aggregator receives 999 TKNA (the fee is 0.1%)
Now swapOutV5() is called on the aggregator with aggregationPayload.fromAmount equal to 1000 still
The 1000 amount of TKNA will be passed as a parameter to swapExactTokensForETH() on a router (e.g. Uniswap) which will attempt to pull 1000 TKNA from the aggrgator resulting in a revert due to an insufficient amount of tokens.

The above scenario can be even more dramatic if the contract holds some TKNA that belong to another user.

In this case, the swap will go through and the 1 missing TKNA levied as fees will be "stolen" from the other user.

The described scenario will now affect that other user.

Tools used

Manual review

Recommended mitigation steps

Cache the balance of the input token the aggregator holds before the transfer then subtract it from the balance the aggregator effectively holds after the transfer and use it as the fromAmount in the swapOutV5() call.

Assessed type

Token-Transfer

c4-judge commented 3 months ago

trust1995 changed the severity to 2 (Med Risk)