code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L127-L128 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L143-L144 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L127-L128 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L63 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L23 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L51 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L71

Vulnerability details

Impact

Using transfer to send ETH relies on a hardcoded gas amount being sent, which can cause the operation to fail if the destination is a smart contract.

Specifically, the transfer will inevitably fail when: 1) The withdrawer smart contract does not implement a payable fallback function. 2) The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units. 3) The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L127-L128

function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
[...]
    if (token == address(0)) {
        uint256 amount = address(this).balance;
        if (amount > 0) receiver.transfer(amount);
    }

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L143-L144

function refund(
    address payable receiver,
    address token,
    uint256 amount
) external onlyOwner {
    if (receiver == address(0)) revert InvalidAddress();

    if (token == address(0)) {
        receiver.transfer(amount);
    }
[...]

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L127-L128

function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
[...]
if (token == address(0)) {
    uint256 amount = address(this).balance;
    if (amount > 0) receiver.transfer(amount);
}

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L63

function addWrapping(
[...]
    payable(msg.sender).transfer(address(this).balance);

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L23

function receiveAndSendToken(
    address payable refundAddress,
    string calldata destinationChain,
    string calldata destinationAddress,
    string calldata symbol
) external {

    if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
[...]

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L51

function receiveAndSendNative(
    address payable refundAddress,
    string calldata destinationChain,
    string calldata destinationAddress
) external {
[...]
    if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L71

function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external {
    if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
    [...]

Tools Used

vim

Recommended Mitigation Steps

These methods should be avoided. Use .call.value(...)("") instead. This carries a risk regarding reentrancy. Be sure to use one of the robust methods available for preventing reentrancy vulnerabilities.

GalloDaSballo commented 2 years ago

Claims to use .call without POC, historically QA

re1ro commented 2 years ago

Duplicate of #4

code-423n4 / 2022-07-axelar-findings