QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	5
Non-Critical Risk	3

Table of Contents

• 1. Low Risk Issues
  • 1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug
  • 1.2. Deprecated approve() function
  • 1.3. Missing address(0) checks
  • 1.4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 1.5. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. Use bytes.concat()
  • 2.3. public functions not called by the contract should be declared external instead

1. Low Risk Issues

1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug

The protocol is using low level calls with solidity version <= 0.8.14 which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Consider upgrading to solidity 0.8.15 here:

deposit-service/DepositBase.sol:3:pragma solidity 0.8.9;
deposit-service/DepositBase.sol:71:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
gas-service/AxelarGasService.sol:3:pragma solidity 0.8.9;
gas-service/AxelarGasService.sol:158:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
gas-service/AxelarGasService.sol:172:        (bool success, bytes memory returnData) = tokenAddress.call(
xc20/contracts/XC20Wrapper.sol:3:pragma solidity 0.8.9;
xc20/contracts/XC20Wrapper.sol:95:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
xc20/contracts/XC20Wrapper.sol:106:        (bool success, bytes memory returnData) = tokenAddress.call(
AxelarGateway.sol:3:pragma solidity 0.8.9;
AxelarGateway.sol:320:            (bool success, ) = address(this).call(abi.encodeWithSelector(commandSelector, params[i], commandId));
AxelarGateway.sol:461:        (bool success, bytes memory returnData) = tokenAddress.call(callData);

1.2. Deprecated approve() function

Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);
deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);

1.3. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• receiverImplementation

File: AxelarDepositService.sol
16:     address public immutable receiverImplementation;
17: 
18:     constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {
19:         receiverImplementation = address(new ReceiverImplementation(gateway, wrappedSymbol));
20:     }

• gatewayAddress

File: XC20Wrapper.sol
24:     address public immutable gatewayAddress;
25: 
26:     constructor(address gatewayAddress_) {
27:         gatewayAddress = gatewayAddress_;

This is already done here:

• gateway

File: DepositBase.sol
13:     address public immutable gateway;
...
21:     constructor(address gateway_, string memory wrappedSymbol_) {
22:         if (gateway_ == address(0)) revert InvalidAddress();

and here:

• AUTH_MODULE and TOKEN_DEPLOYER_IMPLEMENTATION

File: AxelarGateway.sol
44: 
45:     address internal immutable AUTH_MODULE;
46:     address internal immutable TOKEN_DEPLOYER_IMPLEMENTATION;
47: 
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:     }
55: 

1.4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here
Original issue: Hash collisions when using abi.encodePacked() with multiple variable length arguments

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

AxelarGateway.sol:298:            bytes32 commandHash = keccak256(abi.encodePacked(commands[i]));

1.5. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role.
Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

auth/AxelarAuthWeighted.sol:7:import { Ownable } from '../Ownable.sol';
auth/AxelarAuthWeighted.sol:9:contract AxelarAuthWeighted is Ownable, IAxelarAuthWeighted {
auth/AxelarAuthWeighted.sol:47:    function transferOperatorship(bytes calldata params) external onlyOwner {
gas-service/AxelarGasService.sol:120:    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
gas-service/AxelarGasService.sol:140:    ) external onlyOwner {
interfaces/IAxelarAuth.sol:5:import { IOwnable } from './IOwnable.sol';
interfaces/IAxelarAuth.sol:7:interface IAxelarAuth is IOwnable {
xc20/contracts/XC20Wrapper.sol:36:        _transferOwnership(owner_);
xc20/contracts/XC20Wrapper.sol:44:    function setXc20Codehash(bytes32 newCodehash) external onlyOwner {
xc20/contracts/XC20Wrapper.sol:53:    ) external payable onlyOwner {
xc20/contracts/XC20Wrapper.sol:66:    function removeWrapping(string calldata symbol) external onlyOwner {

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

The emit keyword is at line L224, consider moving it after L234:

File: AxelarGateway.sol
217:     function upgrade(
218:         address newImplementation,
219:         bytes32 newImplementationCodeHash,
220:         bytes calldata setupParams
221:     ) external override onlyAdmin {
222:         if (newImplementationCodeHash != newImplementation.codehash) revert InvalidCodeHash();
223: 
224:         emit Upgraded(newImplementation);
225: 
226:         // AUDIT: If `newImplementation.setup` performs `selfdestruct`, it will result in the loss of _this_ implementation (thereby losing the gateway)
227:         //        if `upgrade` is entered within the context of _this_ implementation itself.
228:         if (setupParams.length != 0) {
229:             (bool success, ) = newImplementation.delegatecall(abi.encodeWithSelector(IAxelarGateway.setup.selector, setupParams));
230: 
231:             if (!success) revert SetupFailed();
232:         }
233: 
234:         _setImplementation(newImplementation);
235:     }

2.2. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)), this is relevant.
Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>)), but it isn't used in the solution, so the upgrade isn't relevant (0.8.9 is ok)

deposit-service/AxelarDepositService.sol:3:pragma solidity 0.8.9;
deposit-service/AxelarDepositService.sol:228:                            abi.encodePacked(
deposit-service/AxelarDepositService.sol:233:                                keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
AxelarGateway.sol:3:pragma solidity 0.8.9;
AxelarGateway.sol:540:        return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_LIMIT, symbol));
AxelarGateway.sol:544:        return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day));
AxelarGateway.sol:548:        return keccak256(abi.encodePacked(PREFIX_TOKEN_TYPE, symbol));
AxelarGateway.sol:552:        return keccak256(abi.encodePacked(PREFIX_TOKEN_ADDRESS, symbol));
AxelarGateway.sol:556:        return keccak256(abi.encodePacked(PREFIX_COMMAND_EXECUTED, commandId));

2.3. public functions not called by the contract should be declared external instead

deposit-service/AxelarDepositService.sol:241:    function contractId() public pure returns (bytes32) {
deposit-service/DepositBase.sol:41:    function wrappedToken() public view returns (address) {
deposit-service/DepositBase.sol:46:    function wrappedSymbol() public view returns (string memory symbol) {
xc20/contracts/XC20Wrapper.sol:30:    function gateway() public view override returns (IAxelarGateway) {
xc20/contracts/XC20Wrapper.sol:40:    function contractId() public pure returns (bytes32) {
AxelarGateway.sol:152:    function tokenDailyMintLimit(string memory symbol) public view override returns (uint256) {
AxelarGateway.sol:156:    function tokenDailyMintAmount(string memory symbol) public view override returns (uint256) {
AxelarGateway.sol:164:    function implementation() public view override returns (address) {
AxelarGateway.sol:176:    function isCommandExecuted(bytes32 commandId) public view override returns (bool) {

[GalloDaSballo]

1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug

NC

1.2. Deprecated approve() function

Disagree with adding allowance but agree with using safeApprove to avoid nasty compatibility
L

1.3. Missing address(0) checks

L

## 1.4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
I don't believe this is the case unless you can demonstrate non-idempotency caused by the order of commands, however in that case the finding would be of higher severity

1.5. Use a 2-step ownership transfer pattern

NC

2.1. It's better to emit after all processing is done

Disagree because it's mostly opinion based and slither will produce false positives

2.2. Use bytes.concat()

NC

2.3. public functions not called by the contract should be declared external instead

R

Pretty good

2L 1R 3NC