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

XC20Wrapper may lost received token forever if LocalAsset(xc20).mint is reverted indefinitely #176

Open
code423n4 opened this issue Aug 3, 2022 · 2 comments
Labels
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 sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

XC20Wrapper may lost received token forever if LocalAsset(xc20).mint is reverted indefinitely.

Similar to ERC20, the spec said that if mitn returns false it means minting is failed. But it is commonly revert instead of returning false which is also a minting failure. XC20 may revert on minting as well and common sense also guiding programmers to use the revert pattern instead of returning false.

This case is not handled if SC20 minting is reverted indefinitely. No matter how hard you retry the GMP message execution, it always fail thus the token get locked forever.

Proof of Concept

    function _executeWithToken(
        string calldata,
        string calldata,
        bytes calldata payload,
        string calldata tokenSymbol,
        uint256 amount
    ) internal override {
        address receiver = abi.decode(payload, (address));
        address tokenAddress = gateway().tokenAddresses(tokenSymbol);
        address xc20 = wrapped[tokenAddress];
        if (xc20 == address(0) || !LocalAsset(xc20).mint(receiver, amount)) {
            _safeTransfer(tokenAddress, receiver, amount);
        }
    }
  • Token is sent to gateway before executing the message on the destination chain.
  • If _executeWithToken fail, the token remain inside gateway. The only way to use that token is to execute the _executeWithToken succesfully.
  • Assume LocalAsset(xc20).mint(...) revert indefinitely, _executeWithToken also revert indefinitely.
  • As a result, _executeWithToken never success thus the tokens remain inside gateway forever.

Tools Used

Manual review

Recommended Mitigation Steps

Use try catch

    function _executeWithToken(
        string calldata,
        string calldata,
        bytes calldata payload,
        string calldata tokenSymbol,
        uint256 amount
    ) internal override {
        address receiver = abi.decode(payload, (address));
        address tokenAddress = gateway().tokenAddresses(tokenSymbol);
        address xc20 = wrapped[tokenAddress];
        if (xc20 == address(0)) {
            _safeTransfer(tokenAddress, receiver, amount);
        }

        try LocalAsset(xc20).mint(receiver, amount) returns (bool success) {
            if (!success) _safeTransfer(tokenAddress, receiver, amount);
        } catch { _safeTransfer(tokenAddress, receiver, amount); }
    }
@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 3, 2022
code423n4 added a commit that referenced this issue Aug 3, 2022
@re1ro
Copy link
Member

re1ro commented Aug 5, 2022

Mitigation

We addressed the issue with introducing _safeMint function
axelarnetwork/axelar-xc20-wrapper#4

@re1ro re1ro added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Aug 23, 2022
@GalloDaSballo
Copy link
Collaborator

The warden states that mint() may fail and cause a revert instead of returning false.

With the code in scope we can check the used ERC20 implementation and we find:

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

        if (account == address(0)) revert InvalidAccount();

Because a revert can happen, the scenario, which hypothetically would brick the functionality can actually happen.

We may also have reverts due to overflow and underflow.

Because the code is built to assume that no revert can happen, but the warden demonstrated how a revert could factually happen, I do agree with Medium Severity.

The sponsor has mitigated by using _safeMint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants