The protocol have a whitelist for the tokens, the whitelist includes some Fee-on-Transfer tokens as well like PAXG. The ThorChain_Router contract's functions should be compatible with the Fee-on-transfer tokens but they are not.
The _transferOutAndCallV5 function are implementetd to allow users to make swaps with external liquidity, it uses an aggregator to create the swaps possible:
As we can see, the _transferOutAndCallV5 function first transfers the tokens to the aggregator and then calls the aggregator's sawpOutV5() to complete the swap.
function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {
if (aggregationPayload.fromAsset == address(0)) {
.....
} else {
_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"
);
(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
)
);
}
}
The problem is that the protocol tries to swap the same amount of tokens it sent to the aggregator before the swap aggregationPayload.fromAmount. Which means this will revert due to insufficient balance when the token will be a Fee-on-transfer token:
(bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
abi.encodeWithSignature(
"swapOutV5(address,uint256,address,address,uint256,bytes,string)",
aggregationPayload.fromAsset,
///@audit-issue M fee-on-transfer tokens will not work bcz they transfer the same amt and then try to swap same amt as well
@> aggregationPayload.fromAmount,
aggregationPayload.toAsset,
aggregationPayload.recipient,
aggregationPayload.amountOutMin,
aggregationPayload.payload,
aggregationPayload.originAddress
)
);
Impact
Broken Functionality, doesn't allow users to interact with fee-on-transfer tokens with the protocol
Proof of Concept
Bob deposits 5000 PAXG on Ethereum in order to bridge it to Arbirtum and swap it to the USDT.
Bob's PAXG gets bridged to Arbitrum successfully and the vault calls the transferOutAndCallV5() to perform the swap
TC Router transfers the 5000 PAXG to the aggregator, the aggregator receives 4999 PAXG due to PAXG's 0.02% fee
TC Router calls the swapOutV5 on the aggregator with the 5000 PAXG
The Trx reverts due to the insufficient balance of the aggregator
As the aggregators will be whitelisted and probably deployed by the TC, I think instead of transferring the tokens to the aggregator, the aggregators should only get the allowance for the transfer and then they should handle the fee-on-transfer tokens based on the start and end balance math.
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Router.sol#L368
Vulnerability details
The protocol have a whitelist for the tokens, the whitelist includes some Fee-on-Transfer tokens as well like
PAXG
. TheThorChain_Router
contract's functions should be compatible with the Fee-on-transfer tokens but they are not.The
_transferOutAndCallV5
function are implementetd to allow users to make swaps with external liquidity, it uses an aggregator to create the swaps possible:As we can see, the
_transferOutAndCallV5
function first transfers the tokens to theaggregator
and then calls theaggregator
'ssawpOutV5()
to complete the swap.The problem is that the protocol tries to swap the same amount of tokens it sent to the aggregator before the swap
aggregationPayload.fromAmount
. Which means this will revert due to insufficient balance when the token will be a Fee-on-transfer token:Impact
Broken Functionality, doesn't allow users to interact with fee-on-transfer tokens with the protocol
Proof of Concept
5000
PAXG on Ethereum in order to bridge it to Arbirtum and swap it to the USDT.transferOutAndCallV5()
to perform the swap5000
PAXG to the aggregator, the aggregator receives4999
PAXG due to PAXG's0.02%
feeswapOutV5
on the aggregator with the5000
PAXGTools Used
Shaheen's YadDasht
Recommended Mitigation Steps
As the aggregators will be whitelisted and probably deployed by the TC, I think instead of transferring the tokens to the aggregator, the aggregators should only get the allowance for the transfer and then they should handle the fee-on-transfer tokens based on the start and end balance math.
Assessed type
ERC20