The Bifrost might intercept events while nothing actually happened on the blockchain, no state change whatsoever.
Actions might be overtaken by Bifrost once it intercepts such a TransferOut* event, which could mess up internal accounting or lead to unexpected behaviors.
Proof of concept
The file smartcontract_log_parser.go is responsible for intercepting events emitted by THORChain_Router.sol, notably TransferOut() (corresponding to the transferOutEvent variable in the go file) and TransferOutAndCall() (corresponding to the transferOutAndCallEvent variable in the go file) .
These events are parsed later on in a switch statement which triggers internal mechanisms to take it into account.
There is a scenario in which nothing actually happens on the blockchain but the event is still emitted, which could lead to unexpected behaviors.
Consider the transferOutAndCall() function and the following scenario :
the previous fails (for some reason) and as a fallback, the native coin is sent directly to the recipient
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.
}
}
the native coin transfer also fails (for some other reason) so these coins are sent back to the caller (e.g. the vault)
if (!ethSuccess) {
payable(address(msg.sender)).transfer(_safeAmount); // For failure, bounce back to vault & continue.
}
at this point, the state of the blockchain has not changed, there was only a native coin transfer from the vault to the router contract which sent these native coin back to the vault again
because the transaction did not revert, the TransferOutAndCall() event is still triggered to notify the Bifrost to undertake internal actions.
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
);
The described issue has been discussed with the team but the consequences of it are still unclear and should undergo an in-depth investigation in my opinion. From my experience, a piece of code that is executed for no particular reason can have terrible effects.
Note this behavior affects the following functions :
public transferOut()
internal _transferOutV5()
public transferOutAndCall()
internal _transferOutAndCallV5()
Due to time constraints, I won't be able to investigate before the contest ends but I'll make sure to give a feedback to the protocol team on Discord and notify the judges during the review.
Tools used
Manual review
Recommended mitigation steps
The transaction should revert instead of sending the native coin to the vault and emitting the corresponding event in order to avoid a mislead on Bifrost.
In case the transaction MUST NOT EVER be reverted, adding a specific value to the event and parsing it differently in the log parser when this happens.
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L196 https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L213 https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L280 https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L326
Vulnerability details
Impact
The Bifrost might intercept events while nothing actually happened on the blockchain, no state change whatsoever.
Actions might be overtaken by Bifrost once it intercepts such a
TransferOut*
event, which could mess up internal accounting or lead to unexpected behaviors.Proof of concept
The file
smartcontract_log_parser.go
is responsible for intercepting events emitted byTHORChain_Router.sol
, notablyTransferOut()
(corresponding to thetransferOutEvent
variable in thego
file) andTransferOutAndCall()
(corresponding to thetransferOutAndCallEvent
variable in thego
file) .These events are parsed later on in a
switch
statement which triggers internal mechanisms to take it into account.There is a scenario in which nothing actually happens on the blockchain but the event is still emitted, which could lead to unexpected behaviors.
Consider the
transferOutAndCall()
function and the following scenario :https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L261-L293
swapOut()
function of the aggregator is made to swap native coin to an ERC20 before sending them to the recipientat this point, the state of the blockchain has not changed, there was only a native coin transfer from the vault to the router contract which sent these native coin back to the vault again
because the transaction did not revert, the
TransferOutAndCall()
event is still triggered to notify the Bifrost to undertake internal actions.The described issue has been discussed with the team but the consequences of it are still unclear and should undergo an in-depth investigation in my opinion. From my experience, a piece of code that is executed for no particular reason can have terrible effects.
Note this behavior affects the following functions :
transferOut()
_transferOutV5()
transferOutAndCall()
_transferOutAndCallV5()
Due to time constraints, I won't be able to investigate before the contest ends but I'll make sure to give a feedback to the protocol team on Discord and notify the judges during the review.
Tools used
Manual review
Recommended mitigation steps
The transaction should revert instead of sending the native coin to the vault and emitting the corresponding event in order to avoid a mislead on Bifrost.
In case the transaction MUST NOT EVER be reverted, adding a specific value to the event and parsing it differently in the log parser when this happens.
Assessed type
Context