Axelar Network contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-04-axelar/blob/dee2f2d352e8f20f20027977d511b19bfcca23a3/src/AxelarGateway.sol#L545-L548 https://github.com/code-423n4/2022-04-axelar/blob/dee2f2d352e8f20f20027977d511b19bfcca23a3/src/AxelarGatewayProxy.sol#L16-L24

Vulnerability details

Impact

As written in the solidity documentation, the low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Proof of Concept

The low-level functions call and delegatecall are used in some places in the code and it can be problematic. For example, in the _callERC20Token of the AxelarGateway contract there is a low level call in order to call the ERC20 functions, but if the given tokenAddress doesn't exist success will be equal to true and the function will return true and the code execution will be continued like the call was successful.

function _callERC20Token(address tokenAddress, bytes memory callData) internal returns (bool) {
    (bool success, bytes memory returnData) = tokenAddress.call(callData);
    return success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
}

Another place that this can happen is in AxelarGatewayProxy's constructor

constructor(address gatewayImplementation, bytes memory params) {
    _setAddress(KEY_IMPLEMENTATION, gatewayImplementation);

   (bool success, ) = gatewayImplementation.delegatecall(
       abi.encodeWithSelector(IAxelarGateway.setup.selector, params)
   );

    if (!success) revert SetupFailed();
}

If the gatewayImplementation address doesn't exist, the delegate call will return true and the function won't revert.

Tools Used

Remix, VS Code

Recommended Mitigation Steps

Check before any low-level call that the address actually exists, for example before the low level call in the callERC20 function you can check that the address is a contract by checking its code size.

Lines of code

https://github.com/code-423n4/2022-04-axelar/blob/dee2f2d352e8f20f20027977d511b19bfcca23a3/src/AxelarGateway.sol#L384-L389

Vulnerability details

Impact

When transferring tokens to other chain, the tokens in the source chain are burned - if they are external they will be transferred to the AxelarGateway, otherwise they will be burned. In the target chain the same amount of tokens will be minted for the user - if it is external it will be transferred to him from the AxelarGateway, otherwise it will be minted to him. But there is a problem - if the AxelarGateway doesn't have the needed amount of token for some reason, the _callERC20Token with the transfer function selector will fail and return false, which will make the _mintToken function revert. Because it reverted, the user won't get his funds on the destination chain, although he payed the needed amount in the source chain.

Tools Used

VS Code and Remix

Recommended Mitigation Steps

Instead of reverting when the transfer is not successful, simply call the callContractWithToken with the source chain as the destination chain in order to return the user his funds.

Low and Non-critical Bugs

• Force receiving ether in the AxelarGatewayProxy might be unwanted The receive function of the `` contract is implemented so ETH can't be transferred to the contract.

  receive() external payable {
      revert('NO_ETHER');
  }

  However, ETH can still be transferred to the contract by using selfdestruct. selfdestruct sends all remaining Ether stored in the contract to a designated address. So ETH can still be transferred to the contract.

• Forgot a TODO comment in the burnFrom function of the MintableCappedERC20 contract

• Anyone can destroy the DepositHandler contract and receive its balance It could be better to use access controls in order to prevent malicious function calls (in the current implementation there's no problem because the contract is destructed in the same transaction it is created, but it is a problematic and dangerous implementation).

• Use accept ownership to avoid transferring ownership to a non-existing address In the current implementation, if the transferOwnership function is called with an invalid address, the ownership will be transferred to this address and it can DoS the system. It will be better to make the new owner call a function before actually transferring the ownership, in order to validate that the new owner's address is valid and reachable.

  function transferOwnership(address newOwner) public virtual onlyOwner {
      require(newOwner != address(0), 'ZERO_ADDR');
  
      emit OwnershipTransferred(owner, newOwner);
      owner = newOwner;
  }

Gas Optimizations

• For loop in AdminMultisigBase, AxelarGatewayMultisig

• Loop optimizations Loops can be optimized in several ways. Let's take for example the loop in the _areValidOwnersInEpoch function in AxelarGatewayMultisig.

  for (uint256 i; i < accounts.length; i++) {
      if (_isOwner(epoch, accounts[i]) && ++validSignerCount >= threshold) return true;
  }

  We can do multiple things here:

  1. Use ++i instead of i++ to save some gas spent in every iteration.
  2. Save the array length in a local variable before the loop instead of accessing it in every iteration.

  There are more loops in the code that can be optimized the same way, most of them in the AxelarGatewayMultisig and some more in the AdminMultisigBase and AxelarGateway contracts.

• Use unchecked in the ERC20 contract In several functions in the ERC20 contract unchecked can be used. For example, in the _mint function unchecked can be used on the addition to the balance of the user, because if it will overflow, the totalSupply will overflow too. new code:

  function _mint(address account, uint256 amount) internal virtual {
      require(account != address(0), 'ZERO_ADDR');
  
      _beforeTokenTransfer(address(0), account, amount);
  
      totalSupply += amount;
      unchecked {
          balanceOf[account] += amount;
      }
      emit Transfer(address(0), account, amount);
  }

  In the _burn function, unchecked can be used when subtracting from the totalSupply, because if it will underflow the balance of the user will underflow too. new code:

  function _burn(address account, uint256 amount) internal virtual {
      require(account != address(0), 'ZERO_ADDR');
  
      _beforeTokenTransfer(account, address(0), amount);
  
      balanceOf[account] -= amount;
      unchecked {
          totalSupply -= amount;
      }
      emit Transfer(account, address(0), amount);
  }

  In the _transfer function we can use unchecked when adding to the recipient balance, because the totalSupply doesn't change and it promises us that the balance of the user will be less than or equal to the totalSupply, so it won't overflow. new code:

  function _transfer(
      address sender,
      address recipient,
      uint256 amount
  ) internal virtual {
      require(sender != address(0), 'ZERO_ADDR');
      require(recipient != address(0), 'ZERO_ADDR');
  
      _beforeTokenTransfer(sender, recipient, amount);
  
      balanceOf[sender] -= amount;
      unchecked {
          balanceOf[recipient] += amount;
      }
      emit Transfer(sender, recipient, amount);
  }

• Don't cast numbers to uint256, it does that automatically and costs more gas if you do (for example in the AxelarGateway contract you are using uint256(0) instead of simply 0, which costs more gas).

• Use 1 and 2 as the mutex values in DepositHandler to save gas. Changing a value of a uint variable from non-zero value to a non-zero value costs less gas than changing it from zero to a non-zero value or from a non-zero value ot zero.

• Calculate hash values pre-deployment to save gas on the constructor. For example in AdminMultisigBase there are multiple hashes that can be calculated pre-deployment instead of being calculated when deploying and waste gas

  bytes32 internal constant KEY_OWNER_EPOCH = keccak256('owner-epoch');
  
  bytes32 internal constant PREFIX_OWNER = keccak256('owner');
  bytes32 internal constant PREFIX_OWNER_COUNT = keccak256('owner-count');
  bytes32 internal constant PREFIX_OWNER_THRESHOLD = keccak256('owner-threshold');
  bytes32 internal constant PREFIX_IS_OWNER = keccak256('is-owner');
  
  bytes32 internal constant KEY_OPERATOR_EPOCH = keccak256('operator-epoch');
  
  bytes32 internal constant PREFIX_OPERATOR = keccak256('operator');
  bytes32 internal constant PREFIX_OPERATOR_COUNT = keccak256('operator-count');
  bytes32 internal constant PREFIX_OPERATOR_THRESHOLD = keccak256('operator-threshold');
  bytes32 internal constant PREFIX_IS_OPERATOR = keccak256('is-operator');

• Use deleteBool instead of _setVoteCount(adminEpoch, topic, uint256(0)); in AdminMultisigBase to get gas refunds for deleting a storage value

• Use one require instead of two in _transfer

  require(sender != address(0) && recipient != address(0), 'ZERO_ADDR');

  this can be done also in the _approve function in ERC20 and _beforeTokenTransfer function in BurnableMintableCappedERC20

• Save return value of _msgSender() in decreaseAllowance, increaseAllowance, transferFrom instead of calling it multiple times (or simply use msg.sender to avoid a function call)