ADDRESS.CALL{VALUE:X}() SHOULD BE USED INSTEAD OF PAYABLE.TRANSFER()

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L128 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L144 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#L71 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L86 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L63

Vulnerability details

ADDRESS.CALL{VALUE:X}() SHOULD BE USED INSTEAD OF PAYABLE.TRANSFER()

Impact

When operations use a wrapped native token, the withdraw is being handled with a payable.transfer() method.

When withdrawing and refund extra ETH, the ETHRegistrarController contract uses Solidity’s transfer() function.

Using Solidity's transfer() function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• 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.

Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-Effects-Interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. 

Proof of Concept

xc20/contracts/XC20Wrapper.sol
63:         payable(msg.sender).transfer(address(this).balance);

contracts/deposit-service/ReceiverImplementation.sol
23:         if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
71:         if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
86:         recipient.transfer(amount);

contracts/gas-service/AxelarGasService.sol
128:        if (amount > 0) receiver.transfer(amount);
144:        receiver.transfer(amount);

References:

The issues with transfer() are outlined here

For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised, reference.

[GalloDaSballo]

230

[re1ro]

Duplicate of https://github.com/code-423n4/2022-07-axelar-findings/issues/4