Use of Solidity's transfer() function might render ETH impossible to withdraw

[code423n4]

Lines of code

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

Vulnerability details

Impact

Using Solidity's transfer function has some notable shortcomings when the caller is a smart contract, which can render registers via ETH impossible. Specifically, the transfer will inevitably fail when:

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

The sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. 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. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

Proof of Concept

deposit-service/ReceiverImplementation.sol:

L23 - refundAddress.transfer(address(this).balance);
L51 - refundAddress.transfer(address(this).balance);
L71 - refundAddress.transfer(address(this).balance);
L86 - recipient.transfer(amount);

gas-service/AxelarGasService.sol:

L128 - receiver.transfer(amount);
L144 - receiver.transfer(amount);

Tools Used

Manual review

Recommended mitigation steps

Use Solidity's low-level call() function or the sendValue function available in OpenZeppelin Contract’s Address library to send Ether.

[GalloDaSballo]

See #203

[re1ro]

Duplicate of #4