The THORChain_Router contract has several instances where Ether transfer failures are not correctly handled, resulting in the emission of incorrect events. This can cause downstream systems, such as smartcontract_log_parser.go, to incorrectly process these events.
Impact
The incorrect emission of events due to Ether transfer failures can lead to significant issues in downstream processing. Specifically, the smartcontract_log_parser.go component, which is designed to parse and take appropriate actions based on these events, will process the wrong events. This can result in incorrect accounting within the Thorchain ecosystem.
Proof of Concept
The issue occurs in the following instances within the THORChain_Router contract:
transferOut()
function transferOut(
address payable to,
address asset,
uint amount,
string memory memo
) public payable nonReentrant {
uint safeAmount;
if (asset == address(0)) {
safeAmount = msg.value;
bool success = to.send(safeAmount); // Send ETH.
if (!success) {
@>> payable(address(msg.sender)).transfer(safeAmount); // For failure, bounce back to vault & continue.
}
} else {
// <!-- SKIP -->
}
@>> emit TransferOut(msg.sender, to, asset, safeAmount, memo);
}
Please see the above functions, all are designed to send ETH back to the msg.sender incase if the ETH transfer is failed, but the issue even if its bounce back to the vault they are emitting events which are designed for successful transaction. Subsequently these events are processed by the smartcontract_log_parser.go and cause incorrect accounting or actions.
Tools Used
Manual Review
Recommended Mitigation Steps
Update the event emission logic to ensure that the incorrect events are not emitted even in case of Ether transfer failures. Adding return statement after the bounce back transfer will avoid functions to emit unnesearry events.
transferOut()
function transferOut(
address payable to,
address asset,
uint amount,
string memory memo
) public payable nonReentrant {
uint safeAmount;
if (asset == address(0)) {
safeAmount = msg.value;
bool success = to.send(safeAmount); // Send ETH.
if (!success) {
payable(address(msg.sender)).transfer(safeAmount); // For failure, bounce back to vault & continue.
+ return;
}
} else {
// <!-- SKIP -->
}
emit TransferOut(msg.sender, to, asset, safeAmount, memo);
}
_transferOutV5()
function _transferOutV5(TransferOutData memory transferOutPayload) private {
if (transferOutPayload.asset == address(0)) {
bool success = transferOutPayload.to.send(transferOutPayload.amount); // Send ETH.
if (!success) {
payable(address(msg.sender)).transfer(transferOutPayload.amount); // For failure, bounce back to vault & continue.
+ return;
}
} else {
// <!-- SKIP -->
}
emit TransferOut(
msg.sender,
transferOutPayload.to,
transferOutPayload.asset,
transferOutPayload.amount,
transferOutPayload.memo
);
}
transferOutAndCall()
function transferOutAndCall(
address payable aggregator,
address finalToken,
address to,
uint256 amountOutMin,
string memory memo
) public payable nonReentrant {
uint256 _safeAmount = msg.value;
// <!-- SKIP -->
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.
+ return;
}
}
emit TransferOutAndCall(
msg.sender,
aggregator,
_safeAmount,
finalToken,
to,
amountOutMin,
memo
);
}
_transferOutAndCallV5()
function _transferOutAndCallV5(
TransferOutAndCallData calldata aggregationPayload
) private {
if (aggregationPayload.fromAsset == address(0)) {
// <!-- SKIP -->
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.
+ return;
}
}
emit TransferOutAndCallV5(
msg.sender,
aggregationPayload.target,
msg.value,
aggregationPayload.toAsset,
aggregationPayload.recipient,
aggregationPayload.amountOutMin,
aggregationPayload.memo,
aggregationPayload.payload,
aggregationPayload.originAddress
);
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L185-L207 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L209-L229 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L261-L293 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L304-L340
Vulnerability details
The
THORChain_Router
contract has several instances where Ether transfer failures are not correctly handled, resulting in the emission of incorrect events. This can cause downstream systems, such assmartcontract_log_parser.go
, to incorrectly process these events.Impact
The incorrect emission of events due to Ether transfer failures can lead to significant issues in downstream processing. Specifically, the
smartcontract_log_parser.go
component, which is designed to parse and take appropriate actions based on these events, will process the wrong events. This can result in incorrect accounting within the Thorchain ecosystem.Proof of Concept
The issue occurs in the following instances within the
THORChain_Router
contract:transferOut()
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L185C3-L207C4
_transferOutV5()
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L209C3-L229C6
transferOutAndCall()
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L261C3-L293C4
_transferOutAndCallV5()
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L304C3-L340C9
Please see the above functions, all are designed to send ETH back to the
msg.sender
incase if the ETH transfer is failed, but the issue even if its bounce back to the vault they are emitting events which are designed for successful transaction. Subsequently these events are processed by thesmartcontract_log_parser.go
and cause incorrect accounting or actions.Tools Used
Manual Review
Recommended Mitigation Steps
Update the event emission logic to ensure that the incorrect events are not emitted even in case of Ether transfer failures. Adding
return
statement after the bounce back transfer will avoid functions to emit unnesearry events.transferOut()
_transferOutV5()
transferOutAndCall()
_transferOutAndCallV5()
Assessed type
Other