function transferOutAndCall(
address payable aggregator,
address finalToken,
address to,
uint256 amountOutMin,
string memory memo
) public payable nonReentrant {
uint256 _safeAmount = msg.value;
(bool erc20Success, ) = aggregator.call{value: _safeAmount}(
abi.encodeWithSignature(
"swapOut(address,address,uint256)",
finalToken,
to,
amountOutMin
)
);
if (!erc20Success) {
bool ethSuccess = payable(to).send(_safeAmount); // If can't swap, just send the recipient the ETH
if (!ethSuccess) {
-> payable(address(msg.sender)).transfer(_safeAmount); // For failure, bounce back to vault & continue.
}
}
-> emit TransferOutAndCall(
msg.sender,
aggregator,
_safeAmount,
finalToken,
to,
amountOutMin,
memo
);
}
In the transferOutAndCall function, if the aggregator's swap fails and directly sending ETH to the recipient also fails (i.e. !erc20Success && !ethSuccess == true), the ETH is sent back to the vault.
In this case, the transferOutAndCall should be a no-op transaction, meaning it should have no side effects.
However, it still emits a TransferOutAndCall event, which will be logged by the Bifrost. when Bifrost observes a TransferOutAndCall event, it can not tell whether the funds were transferred to the recipient or sent back to the vault. This could lead to a situation where the THORNChain network believes the recipient has received a fund when they were actually sent back to the vault, resulting in a loss of funds for the recipient.
The same issue also applies to the transferOut and _transferOutV5 functions, which emit the TransferOut event even when ETH is sent back to the vault.
It could also apply to the _transferOutAndCallV5 functions, which emit the TransferOutAndCallV5 event when ETH is sent back to the vault.
Tools Used
Manual Review
Recommended Mitigation Steps
Design a distinct event for the scenario where ETH is sent back to the vault, and handle it separately in Bifrost.
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L261-L293 https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L209-L238 https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L185-L207 https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L304-L341
Vulnerability details
Impact
In the
transferOutAndCall
function, if the aggregator's swap fails and directly sending ETH to the recipient also fails (i.e. !erc20Success && !ethSuccess == true), the ETH is sent back to the vault.In this case, the
transferOutAndCall
should be a no-op transaction, meaning it should have no side effects.However, it still emits a
TransferOutAndCall
event, which will be logged by the Bifrost. when Bifrost observes aTransferOutAndCall
event, it can not tell whether the funds were transferred to the recipient or sent back to the vault. This could lead to a situation where the THORNChain network believes the recipient has received a fund when they were actually sent back to the vault, resulting in a loss of funds for the recipient.The same issue also applies to the
transferOut
and_transferOutV5
functions, which emit theTransferOut
event even when ETH is sent back to the vault. It could also apply to the_transferOutAndCallV5
functions, which emit theTransferOutAndCallV5
event when ETH is sent back to the vault.Tools Used
Manual Review
Recommended Mitigation Steps
Design a distinct event for the scenario where ETH is sent back to the vault, and handle it separately in Bifrost.
Assessed type
Error