Axelar Network v2 contest - 8olidity's results

Unused receive() function will lock Ether in contract

contracts\AxelarGatewayProxy.sol:
  47  
  48:     receive() external payable {
  49          revert('NO_ETHER');

contracts\deposit-service\AxelarDepositServiceProxy.sol:
  12      // solhint-disable-next-line no-empty-blocks
  13:     receive() external payable override {}
  14  }

contracts\deposit-service\DepositReceiver.sol:
  28      // solhint-disable-next-line no-empty-blocks
  29:     receive() external payable {}
  30  }

contracts\util\Proxy.sol:
  85  
  86:     receive() external payable virtual {
  87          revert EtherNotAccepted();

xc20\contracts\Proxy.sol:
  59  
  60:     receive() external payable virtual {
  61          revert EtherNotAccepted();

address.call{value:x}() should be used instead of payable.transfer()

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

The contract does not have a payable callback The contract's payable callback spends more than 2300 gas (which is only enough to emit something) The contract is called through a proxy which itself uses up the 2300 gas

contracts\deposit-service\ReceiverImplementation.sol:
  22          // Always refunding native otherwise it's sent on DepositReceiver self destruction
  23:         if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  24  

  50          if (refund != address(0)) {
  51:             if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  52  

  70      function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external {
  71:         if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  72  

  85          IWETH9(wrappedTokenAddress).withdraw(amount);
  86:         recipient.transfer(amount);
  87      }

contracts\gas-service\AxelarGasService.sol:
  127                  uint256 amount = address(this).balance;
  128:                 if (amount > 0) receiver.transfer(amount);
  129              } else {

  143          if (token == address(0)) {
  144:             receiver.transfer(amount);
  145          } else {



xc20\contracts\XC20Wrapper.sol:
  62          if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');
  63:         payable(msg.sender).transfer(address(this).balance);
  64      }

ZERO-ADDRESS CHECKS ARE MISSING

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters.

 2022-07-axelar/contracts/AxelarGateway.sol
         48┆     constructor(address authModule, address tokenDeployerImplementation) {
         49┆         if (authModule.code.length == 0) revert InvalidAuthModule();
         50┆         if (tokenDeployerImplementation.code.length == 0) revert InvalidTokenDeployer();
         51┆
         52┆         AUTH_MODULE = authModule;
         53┆         TOKEN_DEPLOYER_IMPLEMENTATION = tokenDeployerImplementation;
         54┆     }


 2022-07-axelar/contracts/AxelarGatewayProxy.sol
         16┆     constructor(address gatewayImplementation, bytes memory params) {
         17┆         _setAddress(KEY_IMPLEMENTATION, gatewayImplementation);
         18┆
         19┆         if (gatewayImplementation.code.length == 0) revert InvalidImplementation();
         20┆
         21┆         (bool success, ) = gatewayImplementation.delegatecall(abi.encodeWithSelector(IAxelarGateway.setup.selector, params));
         22┆
         23┆         if (!success) revert SetupFailed();
         24┆     }


 2022-07-axelar/contracts/BurnableMintableCappedERC20.sol
         12┆     constructor(
         13┆         string memory name,
         14┆         string memory symbol,
         15┆         uint8 decimals,
         16┆         uint256 capacity
         17┆     ) MintableCappedERC20(name, symbol, decimals, capacity) {}


 2022-07-axelar/contracts/ERC20.sol
         46┆     constructor(
         47┆         string memory name_,
         48┆         string memory symbol_,
         49┆         uint8 decimals_
         50┆     ) {
         51┆         name = name_;
         52┆         symbol = symbol_;
         53┆         decimals = decimals_;
         54┆     }


 2022-07-axelar/contracts/ERC20Permit.sol
         28┆     constructor(string memory name) {
         29┆         DOMAIN_SEPARATOR = keccak256(
         30┆             abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
         31┆         );
         32┆     }


 2022-07-axelar/contracts/MintableCappedERC20.sol
         14┆     constructor(
         15┆         string memory name,
         16┆         string memory symbol,
         17┆         uint8 decimals,
         18┆         uint256 capacity
         19┆     ) ERC20(name, symbol, decimals) ERC20Permit(name) Ownable() {
         20┆         cap = capacity;
         21┆     }


 2022-07-axelar/contracts/Ownable.sol
         10┆     constructor() {
         11┆         owner = msg.sender;
         12┆         emit OwnershipTransferred(address(0), msg.sender);
         13┆     }


 2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol
         16┆     constructor(bytes[] memory recentOperators) {
         17┆         for (uint256 i; i < recentOperators.length; ++i) {
         18┆             _transferOperatorship(recentOperators[i]);
         19┆         }
         20┆     }


 2022-07-axelar/contracts/deposit-service/AxelarDepositService.sol
         18┆     constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {
         19┆         receiverImplementation = address(new ReceiverImplementation(gateway, wrappedSymbol));
         20┆     }


 2022-07-axelar/contracts/deposit-service/DepositBase.sol
         21┆     constructor(address gateway_, string memory wrappedSymbol_) {
         22┆         if (gateway_ == address(0)) revert InvalidAddress();
         23┆
         24┆         gateway = gateway_;
         25┆
         26┆         // Checking if token symbol exists in the gateway
         27┆         if (IAxelarGateway(gateway_).tokenAddresses(wrappedSymbol_) == address(0)) revert InvalidSymbol();
         28┆
         29┆         // Converting a string to bytes32 for immutable storage
         30┆         bytes memory symbolBytes = bytes(wrappedSymbol_);


 2022-07-axelar/contracts/deposit-service/DepositReceiver.sol
          8┆     constructor(bytes memory delegateData) {
          9┆         // Reading the implementation of the AxelarDepositService
         10┆         // and delegating the call back to it
         11┆         // solhint-disable-next-line avoid-low-level-calls
         12┆         (bool success, ) = IAxelarDepositService(msg.sender).receiverImplementation().delegatecall(delegateData);
         13┆
         14┆         // if not success revert with the original revert data
         15┆         if (!success) {
         16┆             // solhint-disable-next-line no-inline-assembly
         17┆             assembly {


 2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol
         12┆     constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {}


 2022-07-axelar/contracts/interfaces/IAxelarExecutable.sol
         12┆     constructor(address gateway_) {
         13┆         gateway = IAxelarGateway(gateway_);
         14┆     }


 2022-07-axelar/contracts/interfaces/IAxelarForecallable.sol
         16┆     constructor(address gatewayAddress) {
         17┆         gateway = IAxelarGateway(gatewayAddress);
         18┆     }


 2022-07-axelar/contracts/util/Proxy.sol
         19┆     constructor() {
         20┆         // solhint-disable-next-line no-inline-assembly
         21┆         assembly {
         22┆             sstore(_OWNER_SLOT, caller())
         23┆         }
         24┆     }


 2022-07-axelar/xc20/contracts/ERC20.sol
         46┆     constructor(
         47┆         string memory name_,
         48┆         string memory symbol_,
         49┆         uint8 decimals_
         50┆     ) {
         51┆         name = name_;
         52┆         symbol = symbol_;
         53┆         decimals = decimals_;
         54┆     }


 2022-07-axelar/xc20/contracts/ERC20Permit.sol
         28┆     constructor(string memory name) {
         29┆         DOMAIN_SEPARATOR = keccak256(
         30┆             abi.encode(DOMAIN_TYPE_SIGNATURE_HASH, keccak256(bytes(name)), keccak256(bytes('1')), block.chainid, address(this))
         31┆         );
         32┆     }


 2022-07-axelar/xc20/contracts/Proxy.sol
         17┆     constructor(address implementationAddress, bytes memory params) {
         18┆         // solhint-disable-next-line no-inline-assembly
         19┆         assembly {
         20┆             sstore(_IMPLEMENTATION_SLOT, implementationAddress)
         21┆         }
         22┆         // solhint-disable-next-line avoid-low-level-calls
         23┆         (bool success, ) = implementationAddress.delegatecall(
         24┆             //0x9ded06df is the setup selector.
         25┆             abi.encodeWithSelector(0x9ded06df, params)
         26┆         );


 2022-07-axelar/xc20/contracts/XC20Sample.sol
         19┆     constructor(address owner_, string memory name) ERC20Permit(name) ERC20('', '', 0) {
         20┆         owner = owner_;
         21┆     }


 2022-07-axelar/xc20/contracts/XC20Wrapper.sol
         26┆     constructor(address gatewayAddress_) {
         27┆         gatewayAddress = gatewayAddress_;
         28┆     }

<array>.length should not be looked up in every loop of a for-loop

contracts\AxelarGateway.sol:
  206  
  207:         for (uint256 i = 0; i < symbols.length; i++) {
  208              string memory symbol = symbols[i];


contracts\auth\AxelarAuthWeighted.sol:
   16      constructor(bytes[] memory recentOperators) {
   17:         for (uint256 i; i < recentOperators.length; ++i) {
   18              _transferOperatorship(recentOperators[i]);


   97          // assuming that both operators and signatures are sorted
   98:         for (uint256 i = 0; i < signatures.length; ++i) {


  115      function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {
  116:         for (uint256 i; i < accounts.length - 1; ++i) {
  117              if (accounts[i] >= accounts[i + 1]) {

contracts\deposit-service\AxelarDepositService.sol:
  113      ) external {
  114:         for (uint256 i; i < refundTokens.length; i++) {
  115              address gatewayToken = IAxelarGateway(gateway).tokenAddresses(tokenSymbol);

  167  
  168:         for (uint256 i; i < refundTokens.length; i++) {
  169              refundToken = refundTokens[i];

  203      ) external {
  204:         for (uint256 i; i < refundTokens.length; i++) {
  205              address wrappedTokenAddress = wrappedToken();

contracts\gas-service\AxelarGasService.sol:
  122  
  123:         for (uint256 i; i < tokens.length; i++) {
  124              address token = tokens[i];

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

contracts\AdminMultisigBase.sol:
  14      // AUDIT: slot names should be prefixed with some standard string
  15:     bytes32 internal constant KEY_ADMIN_EPOCH = keccak256('admin-epoch');
  16  
  17:     bytes32 internal constant PREFIX_ADMIN = keccak256('admin');
  18:     bytes32 internal constant PREFIX_ADMIN_COUNT = keccak256('admin-count');
  19:     bytes32 internal constant PREFIX_ADMIN_THRESHOLD = keccak256('admin-threshold');
  20:     bytes32 internal constant PREFIX_ADMIN_VOTE_COUNTS = keccak256('admin-vote-counts');
  21:     bytes32 internal constant PREFIX_ADMIN_VOTED = keccak256('admin-voted');
  22:     bytes32 internal constant PREFIX_IS_ADMIN = keccak256('is-admin');
  23  

contracts\AxelarGateway.sol:
   27      bytes32 internal constant KEY_IMPLEMENTATION = bytes32(0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc);

   29      // AUDIT: slot names should be prefixed with some standard string
   30:     bytes32 internal constant PREFIX_COMMAND_EXECUTED = keccak256('command-executed');
   31:     bytes32 internal constant PREFIX_TOKEN_ADDRESS = keccak256('token-address');
   32:     bytes32 internal constant PREFIX_TOKEN_TYPE = keccak256('token-type');
   33:     bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED = keccak256('contract-call-approved');
   34:     bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED_WITH_MINT = keccak256('contract-call-approved-with-mint');
   35:     bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_LIMIT = keccak256('token-daily-mint-limit');
   36:     bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_AMOUNT = keccak256('token-daily-mint-amount');
   37  
   38:     bytes32 internal constant SELECTOR_BURN_TOKEN = keccak256('burnToken');
   39:     bytes32 internal constant SELECTOR_DEPLOY_TOKEN = keccak256('deployToken');
   40:     bytes32 internal constant SELECTOR_MINT_TOKEN = keccak256('mintToken');
   41:     bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL = keccak256('approveContractCall');
   42:     bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL_WITH_MINT = keccak256('approveContractCallWithMint');
   43:     bytes32 internal constant SELECTOR_TRANSFER_OPERATORSHIP = keccak256('transferOperatorship');

x = x + y is cheaper than x += y

contracts\auth\AxelarAuthWeighted.sol:
   69          for (uint256 i = 0; i < weightsLength; ++i) {
   70:             totalWeight += newWeights[i];
   71          }

  104              // return if weight sum above threshold
  105:             weight += weights[operatorIndex];
  106              // weight needs to reach or surpass threshold

Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file

File: contracts\util\Proxy.sol
    45  // solhint-disable-next-line avoid-low-level-calls
    46  (bool success, ) = implementationAddress.delegatecall(
    47    //0x9ded06df is the setup selector.
    48    abi.encodeWithSelector(0x9ded06df, params)
);