When transferring ETH, use call() instead of transfer().
The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:
The contract does not have a payable callback
The contract's payable callback spends more than 2300 gas (which is only enough to emit something)
The contract is called through a proxy which itself uses up the 2300 gas
Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.
Proof of concept
The function THORChain_Router::transferOut() is used to transfer out any asset to any recipient but the issue arises when the first send() fails it goes tries to transfer the funds back to the msg.sender, the transfer() here doesn't check if it is successful or not may lead to loss of funds if the explained conditions are met.
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. // @audit issue
}
} else {
_vaultAllowance[msg.sender][asset] -= amount; // Reduce allowance
(bool success, bytes memory data) = asset.call(
abi.encodeWithSignature("transfer(address,uint256)", to, amount)
);
require(success && (data.length == 0 || abi.decode(data, (bool))));
safeAmount = amount;
}
emit TransferOut(msg.sender, to, asset, safeAmount, memo);
}
Tools Used
Manual Review
Recommendation
Check if the the transfer() is successful otherwise revert with an error.
Lines of code
https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L196
Vulnerability details
Impact
When transferring ETH, use
call()
instead oftransfer()
.The use of
payable.transfer()
is heavily frowned upon because it can lead to the locking of funds. Thetransfer()
call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.
Proof of concept
The function THORChain_Router::transferOut() is used to transfer out any asset to any recipient but the issue arises when the first
send()
fails it goes tries to transfer the funds back to themsg.sender
, thetransfer()
here doesn't check if it is successful or not may lead to loss of funds if the explained conditions are met.Tools Used
Manual Review
Recommendation
Check if the the transfer() is successful otherwise revert with an error.
Assessed type
call/delegatecall