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

QA Report #187

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

QA Report #187

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

Comments

@code423n4
Copy link
Contributor

code423n4 commented Aug 3, 2022

Low

Proxy setup may fail silently

Since low level calls do not check for contract existence, and return true if a contract does not exist, it's possible for Axelar proxy contracts that delegatecall to a setup function to fail silently if their implementationAddress does not exist.

util/Proxy#init:

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

xc20 Proxy#constructor:

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

Suggestion: Check address.code.length for implementationAddress. For example:

        if (implementationAddress.code.length == 0) revert ImplementationDoesNotExist();
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = implementationAddress.delegatecall(
            //0x9ded06df is the setup selector.
            abi.encodeWithSelector(0x9ded06df, params)
        );
        if (!success) revert SetupFailed();

Gateway may be incompatible with rebasing tokens

In addition to fee-on-transfer tokens (see this finding from the previous C4 contest), the Axelar gateway is not fully compatible with ERC20 tokens that increase an account's underlying balance, like Lido stETH and Aave aTokens. Due to rebases, the gateway contract may accrue a balance greater than the token amount minted on a destination chain, and these excess amounts could remain locked in the gateway. There is not really a reasonable technical solution here: instead it's probably best to document this incompatibility for end users of the protocol if token creation becomes permissionless, or avoid these tokens if it is permissioned.

Fixed chain ID in domain separator

The ERC20Permit contract generates and permanently stores an EIP-712 domain separator using block.chainid at construction time:

ERC20Permit#constructor

    constructor(string memory name) {
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
        );
    }

ERC20Permit#constructor

    constructor(string memory name) {
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
        );
    }

Since chainid is set permanently, there is a risk of replay attacks in the event of a hard fork: since contracts on both forks would use the same stored chainid, signatures on one chain would be replayable on the fork. This is a pretty low probablity scenario, but as a cross-chain protocol supporting many EVM chains, Axelar is probably at higher risk of exposure to a hard fork than a typical single chain application.

Suggestion: cache chainId and DOMAIN_SEPARATOR at construction time. Use the cached DOMAIN_SEPARATOR unless chainID has changed. If so, regenerate DOMAIN_SEPARATOR.

QA

Extract a SafeTransfer library

A similar ERC20 _safeTransfer function is defined twice, in both DepositBase and AxelarGasService. Consider extracting this function to a shared library. (Note that the version in AxelarGasService includes a zero amount check that should be moved outside the function if you do implement this refactor).

DepositBase#_safeTransfer:

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert TokenTransferFailed();
    }

AxelarGasService#_safeTransfer

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        if (amount == 0) revert NothingReceived();

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert TransferFailed();
    }

Note that a similar function exists in the XC20 contract as well, although it may make sense to keep it isolated:

XC20Wrapper.sol#_safeTransfer

    function _safeTransfer(
        address tokenAddress,
        address receiver,
        uint256 amount
    ) internal {
        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));

        if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
    }
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Aug 3, 2022
code423n4 added a commit that referenced this issue Aug 3, 2022
@GalloDaSballo
Copy link
Collaborator

## Proxy setup may fail silently
I think this is the one time where instead of just address(0) check we'd also want a contract.size check, valid L

Gateway may be incompatible with rebasing tokens

L

Fixed chain ID in domain separator

L

Extract a SafeTransfer library

R

Refreshing to see concise and accurate advice

3L 1R

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working edited-by-warden 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

2 participants