Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Call Instead of Transfer for Address Payable #75

Open
code423n4 opened this issue Aug 2, 2022 · 1 comment
Open

Use Call Instead of Transfer for Address Payable #75

code423n4 opened this issue Aug 2, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

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
https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L63

Vulnerability details

Impact

It is strongly recommended to avoid using payable.transfer,
since it can cause the transaction to fail when:

  1. The calling contract does not have payable function
  2. The calling contract have a payable function but spends more than 2300 gas
  3. The calling contract have a payable function and spends less than 2300 gas
    but is called through proxy which uses over 2300 gas.

Also it might be mandatory for some multisig wallets to use higher than 2300 gas.
Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

./ReceiverImplementation.sol:23:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:51:            if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:71:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:86:        recipient.transfer(amount);
./AxelarGasService.sol:128:                if (amount > 0) receiver.transfer(amount);
./AxelarGasService.sol:144:            receiver.transfer(amount);
./XC20Wrapper.sol:63:        payable(msg.sender).transfer(address(this).balance);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Aug 2, 2022
code423n4 added a commit that referenced this issue Aug 2, 2022
@re1ro
Copy link
Member

re1ro commented Aug 5, 2022

Duplicate of #4

@re1ro re1ro marked this as a duplicate of #4 Aug 22, 2022
@re1ro re1ro added the duplicate This issue or pull request already exists label Aug 22, 2022
@re1ro re1ro closed this as completed Aug 22, 2022
@GalloDaSballo GalloDaSballo added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Aug 28, 2022
@GalloDaSballo GalloDaSballo removed the duplicate This issue or pull request already exists label Sep 6, 2022
@GalloDaSballo GalloDaSballo reopened this Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

3 participants