code423n4 commented 2 years ago

Lines of code

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

Deprecated transfer might not work with msg.sender/address payable

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when :

The claimer smart contracts does not implement a payable function
The claimer smart contract does implement a payable function whcih uses more than 2300 gas unit
The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy ,raising the call's gas usage above 2300

File: XC20Wrapper.sol line 63

        payable(msg.sender).transfer(address(this).balance);

File: ReceiverImplementation.sol line 51

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

File: ReceiverImplementation.sol line 71

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

File: ReceiverImplementation.sol line 86

        recipient.transfer(amount);

File: AxelarGasService.sol line 128

                if (amount > 0) receiver.transfer(amount);

File: AxelarGasService.sol line 144

            receiver.transfer(amount);

Tools used

Manual code review

Recommendation

To prevent unexpected behavior , consider using CALL() instead of TRANSFER() . Additionally, note that 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:

GalloDaSballo commented 2 years ago

Same as #230

re1ro commented 2 years ago

Duplicate of #4

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