`_transferOutAndCallV5()` is incompatible with fee on transfer tokens

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The _transferOutAndCallV5() is supposed to work with virtually any kind of tokens, but it will most likely fail every single time when a swap with fee on transfer tokens is made. Before calling the aggregator, the tokens are already sent to the contract. With non-weird ERC20 tokens there will be no issues, but if fee on transfer tokens are used, that might not be the case.

Proof of Concept

1. A user deposits 100 tokens through depositWithExpiry() (Let's assume for the example that the fee is 10% -> the _vaultAllowance mapping will then store 90 for that vault and token)
2. The vault calls transferOutAndCallV5(), the balance will be properly reduced
3. When the transfer is supposed to happen, not 90, but 81 tokens will be transfered to the aggregator (90 tokens * 90% == 81 tokens)
4. During the swapOutV5, unless there are more tokens sent by mistake, the aggregator will try to make a call to the Uniswap router and fail -> it approves the Uniswap router for 90 tokens, but has only 81

Note: The revert happens in the aggregator, but the THORChain_Router contract is at fault. The reason why: even if the issue is fixed inside the THORChain_Router contract, there will be no reverts inside the aggregator. However, because the aggregator also does not account for tokens with fees on transfers, the revert will happen inside the Uniswap router contract.

As a summary, both contracts will have issues with fee on transfer tokens and even though the -transferOutAndCallV5() does not revert, because of the lack of validation, a revert will happen in the following call to the aggregator and same for the aggregator and the Uniswap router

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to use the same pattern as inside safeTransferFrom() - checking what is the balance of the aggregator before and after sending the tokens prior to calling swapOutV5

Assessed type

Other

[c4-judge]

trust1995 marked the issue as satisfactory