Axelar Network v2 contest - Chom's results

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); }
    }

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

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/DepositReceiver.sol#L8-L26 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L85-L102 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L106-L135 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L138-L153 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L157-L182 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L185-L194 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L198-L218

Vulnerability details

Impact

selfdestruct will be banned after "The Purge" phase of ethereum merge. DepositReceiver may be broken. Any interaction between AxelarDepositService may result in the loss of fund. Since AxelarDepositService that deal with native token is highly associated with DepositReceiver contract which use to be banned selfdestruct command.

Look at this image in "The Purge" section in "EVM simplification track" that said "Ban SELFDESTRUCT".

This means any contract that use selfdestruct will break after "The Purge"

Reference: https://medium.com/@Ground_Zero/gz-daily-the-merge-surge-verge-purge-and-splurge-of-the-ethereum-blockchain-b1a9e95ff28d

Proof of Concept

    constructor(bytes memory delegateData) {
        // Reading the implementation of the AxelarDepositService
        // and delegating the call back to it
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, ) = IAxelarDepositService(msg.sender).receiverImplementation().delegatecall(delegateData);

        // if not success revert with the original revert data
        if (!success) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let ptr := mload(0x40)
                let size := returndatasize()
                returndatacopy(ptr, 0, size)
                revert(ptr, size)
            }
        }

        selfdestruct(payable(msg.sender));
    }

DepositReceiver contract use to be banned selfdestruct as shown above.

Once the purge is implemented, new DepositReceiver instruction will be reverted.

Since the fund is sent to DepositReceiver before any functions in AxelarDepositService is called.

Any function in AxelarDepositService that leverage DepositReceiver including sendTokenDeposit, refundTokenDeposit, sendNativeDeposit, refundNativeDeposit, nativeUnwrap, refundNativeUnwrap will always revert. As a result, the fund in DepositReceiver will be lost forever since nobody can deploy a working contract to that address.

Tools Used

Manual review, listen to news and use google to find reference

Recommended Mitigation Steps

Reimplement DepositReceiver such that it can do the job without using selfdestruct.

Transferring dust native fund is not related to addWrapping

https://github.com/code-423n4/2022-07-axelar/blob/main/xc20/contracts/XC20Wrapper.sol#L63

    function addWrapping(
        string calldata symbol,
        address xc20Token,
        string memory newName,
        string memory newSymbol
    ) external payable onlyOwner {
        address axelarToken = gateway().tokenAddresses(symbol);
        if (axelarToken == address(0)) revert('NotAxelarToken()');
        if (xc20Token.codehash != xc20Codehash) revert('NotXc20Token()');
        if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');
        if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');
        wrapped[axelarToken] = xc20Token;
        unwrapped[xc20Token] = axelarToken;
        if (!LocalAsset(xc20Token).set_team(address(this), address(this), address(this))) revert('NotOwner()');
        if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');
        payable(msg.sender).transfer(address(this).balance);
    }

Why transfer dust fund using addWrapper? Consider adding a separate function to do this.

Use payable(...).call{value: ...}("") instead of .transfer

Using deprecated transfer() on address payable may revert in these cases:

1. The withdraw recipient is a smart contract that does not implement a payable function.
2. The withdraw recipient is a smart contract that does implement a payable fallback which uses more than 2300 gas unit.
3. The withdraw recipient is a smart contract that 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.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

An example on Axelar codebase that using .transfer is in https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L144

receiver.transfer(amount);

Should be

      (bool success, ) = payable(receiver).call{value: amount}("");
      require(success, "ETH transfer failed");

XC20Wrapper is not emitting any event

XC20Wrapper should emit events on wrap / unwrap / receive cross chain fund (_executeWithToken) to allow indexers to track the transactions.

_isSortedAscAndContainsNoDuplicate failed if accounts.length == 0

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L115-L123

It is using accounts.length - 1 which will be underflow and revert when accounts.length == 0

Consider adding if to skip this case

    function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {
        if (accounts.length == 0) return false;

        for (uint256 i; i < accounts.length - 1; ++i) {
            if (accounts[i] >= accounts[i + 1]) {
                return false;
            }
        }

        return accounts[0] != address(0);
    }

Missing minimum newThreshold check

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/auth/AxelarAuthWeighted.sol#L72

if (newThreshold == 0 || totalWeight < newThreshold) revert InvalidThreshold();

New threshold shouldn't be too low such that just one low weight operator can execute malicious transaction.

It would be great to have some more shield on this important function by checking new threshold to be more than xx% of totalWeight

Should use pragma solidity ^0.8.9; instead of pragma solidity 0.8.9;

pragma solidity ^0.8.9; allow application that import this library to use more recent version of solidity 0.8.x such as 0.8.13 to compile the contracts while pragma solidity 0.8.9; doesn't allow it.

If that application use some library that require ^0.8.13. That application may not be able to used with Axelar.

AxelarDepositService is not support general message passing

We cannot see anything related to contract calling such as target contract address or payload in the https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/AxelarDepositService.sol.

Which means it doesn't support general message passing

XC20 token doesn't support general message passing on auto xc20 wrap

    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);
        }
    }

payload only hold receiver which is not sufficient for general message passing (require target contract and payload). Moreover, there aren't any logic involve contract calling on _executeWithToken.

Caching the length in for loops and increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Caching the length in for loops:

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

for loop postcondition can be made unchecked Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L168

for (uint256 i; i < refundTokens.length; i++) {

Can be optimized to

uint256 refundTokensLength = refundTokens.length;
for (uint256 i; i < refundTokensLength;) {
    ...
    unchecked { ++i; }
}

Consider using ++i instead of i++

Currently every loops such as

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L168

are using i++ which is more expensive than ++i

for (uint256 i; i < refundTokens.length; ++i) {

is better than

for (uint256 i; i < refundTokens.length; i++) {

Use delete to remove wrapping to save gas

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

Change

        wrapped[axelarToken] = address(0);
        unwrapped[xc20Token] = address(0);

to

        delete wrapped[axelarToken];
        delete unwrapped[xc20Token];

to save gas since delete is using negative gas (Get gas refund)