QA Report

[code423n4]

[L-01] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
IDepositBase.sol::3 => pragma solidity ^0.8.9;

[L-02] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

AxelarGateway.sol::157 => return getUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days));
AxelarGateway.sol::615 => _setUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days), amount);

[L-03] Unused receive() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

AxelarDepositServiceProxy.sol::13 => receive() external payable override {}
DepositReceiver.sol::29 => receive() external payable {}

[L-04] decimals() not part of ERC20 standard

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

XC20Wrapper.sol::62 => if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

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). Unless there is a compelling reason, abi.encode should be preferred. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

AxelarDepositService.sol::233 => keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
AxelarGateway.sol::298 => bytes32 commandHash = keccak256(abi.encodePacked(commands[i]));
AxelarGateway.sol::342 => bytes32 salt = keccak256(abi.encodePacked(symbol));
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));

[L-06] approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

approve is subject to a known front-running attack. Consider using safeApprove() or safeIncreaseAllowance() or safeDecreaseAllowance() instead

AxelarDepositService.sol::30 => IERC20(wrappedTokenAddress).approve(gateway, amount);
ReceiverImplementation.sol::38 => IERC20(tokenAddress).approve(gateway, amount);
ReceiverImplementation.sol::64 => IERC20(wrappedTokenAddress).approve(gateway, amount);

[L-07] Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

AxelarGasService.sol::128 => if (amount > 0) receiver.transfer(amount);
AxelarGasService.sol::144 => receiver.transfer(amount);
ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
ReceiverImplementation.sol::86 => recipient.transfer(amount);

[L-08] Missing checks for zero address

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

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

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(,) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(,) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

AxelarAuthWeighted.sol::3 => pragma solidity 0.8.9;
AxelarDepositServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarDepositService.sol::3 => pragma solidity 0.8.9;
AxelarGasServiceProxy.sol::3 => pragma solidity 0.8.9;
AxelarGasService.sol::3 => pragma solidity 0.8.9;
AxelarGateway.sol::3 => pragma solidity 0.8.9;
DepositBase.sol::3 => pragma solidity 0.8.9;
DepositReceiver.sol::3 => pragma solidity 0.8.9;
IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
IDepositBase.sol::3 => pragma solidity ^0.8.9;
ReceiverImplementation.sol::3 => pragma solidity 0.8.9;
XC20Wrapper.sol::3 => pragma solidity 0.8.9;

[N-02] require()/revert() statements should have descriptive reason strings

for troubleshotting purposes

XC20Wrapper.sol::55 => if (axelarToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::56 => if (xc20Token.codehash != xc20Codehash) revert('NotXc20Token()');
XC20Wrapper.sol::57 => if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');
XC20Wrapper.sol::58 => if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');
XC20Wrapper.sol::68 => if (axelarToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::70 => if (xc20Token == address(0)) revert('NotWrappingToken()');
XC20Wrapper.sol::78 => if (wrappedToken == address(0)) revert('NotAxelarToken()');
XC20Wrapper.sol::84 => if (axelarToken == address(0)) revert('NotXc20Token()');
XC20Wrapper.sol::85 => if (IERC20(wrappedToken).balanceOf(msg.sender) < amount) revert('InsufficientBalance()');
XC20Wrapper.sol::98 => if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
XC20Wrapper.sol::111 => if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');

[N-03] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

IAxelarAuthWeighted.sol::14 => event OperatorshipTransferred(address[] newOperators, uint256[] newWeights, uint256 newThreshold);

[re1ro]

(L1)

We allow Unspecific Compiler version for our interfaces, so they can be imported by other projects

(2)

Not applicable. We are not using block.timestamp for deriving entropy. Slight time manipulation is possible by miners but not critical for our application

(3)

Not applicable. We need receive to receive ether from WETH contract. Duplicate of #7

(4)

Not applicable. axelarToken is our own implementation in this context and it implements decimals

(5)

Yes. But that is applicable only to this one, because there is data following the dynamic type.

AxelarGateway.sol::544 => return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day));

(6)

Yes. Dup #3

(7)

Nope. Dup #3

(L8)

Yes. Dup #3

(N1)

Dup #3

(N2)

Those error messages are descriptive enough

(N3)

Nope. You can't index dynamic data structures without loosing data

[GalloDaSballo]

[L-01] Unspecific Compiler Version Pragma

NC

[L-02] Use of Block.timestamp

Disagree for the cases shown above in lack of explanation

[L-03] Unused receive() function

For the proxy L

[L-04] decimals() not part of ERC20 standard

L

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Disputed for the example shown

[L-06] approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

Disputed for those usages

[L-07] Unsafe use of transfer()/transferFrom() with IERC20

L

[L-08] Missing checks for zero address

L

[N-01] Use a more recent version of solidity

NC

[N-02] require()/revert() statements should have descriptive reason strings

Disputed

[N-03] Event is missing indexed fields

Don't think the sponsor would want to index those values as they will change each time, so what's the point

The turning test of QA

4L 2NC