howlbot-integration[bot] commented 3 months ago

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 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 :

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L261-L293

a low-level call to the swapOut() function of the aggregator is made to swap native coin to an ERC20 before sending them to the recipient

uint256 _safeAmount = msg.value;
(bool erc20Success, ) = aggregator.call{value: _safeAmount}(
    abi.encodeWithSignature(
        "swapOut(address,address,uint256)",
        finalToken,
        to,
        amountOutMin
    )
);

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.

Assessed type

Context

c4-judge commented 3 months ago

trust1995 changed the severity to 3 (High Risk)