The _transferOutAndCallV5() allows a Vault to swap native ETH to any ERC20 token supported by the protocol through the swapOutV5() function called on an aggregator.
function _transferOutAndCallV5(
TransferOutAndCallData calldata aggregationPayload
) private {
if (aggregationPayload.fromAsset == address(0)) {
// call swapOutV5 with ether
(bool swapOutSuccess, ) = aggregationPayload.target.call{
value: msg.value
}(
abi.encodeWithSignature(
"swapOutV5(address,uint256,address,address,uint256,bytes,string)",
aggregationPayload.fromAsset,
aggregationPayload.fromAmount,
aggregationPayload.toAsset,
aggregationPayload.recipient,
aggregationPayload.amountOutMin,
aggregationPayload.payload,
aggregationPayload.originAddress
)
);
if (!swapOutSuccess) {
bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
if (!sendSuccess) {
payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
}
}
In case the swap fails (out-of-gas or some obscure reasons), the protocol implements a "fallback" mechanism which consists of "just send the recipient the gas asset" as stated by the comment.
In reality, the gas asset is transferred to the aggregator aggregationPayload.target not the recipient, which would be aggregationPayload.recipient.
If we abstract from the developers comment and the actual intention was to send the gas asset to the aggregator, another issue occurs : the gas assets will remain on the aggregator contract.
This allows anyone to call the swapOutV5() function on the aggregator to swap the gas assets to another token resulting in these being effectively stolen from the legitimate user it belonged to.
In both scenarios, the current implementation introduces a vulnerability for the users of the protocol.
Tools used
Manual review
Recommended mitigation steps
Send the gas asset to aggregationPayload.recipient rather than aggregationPayload.target.
Doing so will result in the legitimate user to receive its funds even if the swap failed.
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L324
Vulnerability details
Impact
Loss of funds
Potential profit for a malicious party
Proof of concept
The
_transferOutAndCallV5()
allows a Vault to swap native ETH to any ERC20 token supported by the protocol through theswapOutV5()
function called on an aggregator.https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L304-L328
In case the swap fails (out-of-gas or some obscure reasons), the protocol implements a "fallback" mechanism which consists of "just send the recipient the gas asset" as stated by the comment.
In reality, the gas asset is transferred to the aggregator
aggregationPayload.target
not the recipient, which would beaggregationPayload.recipient
.If we abstract from the developers comment and the actual intention was to send the gas asset to the aggregator, another issue occurs : the gas assets will remain on the aggregator contract.
This allows anyone to call the
swapOutV5()
function on the aggregator to swap the gas assets to another token resulting in these being effectively stolen from the legitimate user it belonged to.In both scenarios, the current implementation introduces a vulnerability for the users of the protocol.
Tools used
Manual review
Recommended mitigation steps
Send the gas asset to
aggregationPayload.recipient
rather thanaggregationPayload.target
.Doing so will result in the legitimate user to receive its funds even if the swap failed.
Assessed type
ETH-Transfer