Axelar Network v2 contest - rbserver's results

[L-01] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESS

To prevent unintended actions, unexpected loss of assets, etc., the critical address input should be checked against address(0). Please consider checking gatewayAddress_ for the following code.

xc20\contracts\XC20Wrapper.sol
  26-28:
    constructor(address gatewayAddress_) {
      gatewayAddress = gatewayAddress_;
    }

[L-02] MISSING RETURN VALUE CHECK FOR APPROVE()

Some tokens do not revert but return false when approval fails. Please consider checking the return values of the following approve functions to prevent silent approval failures.

contracts\deposit-service\AxelarDepositService.sol
  30: IERC20(wrappedTokenAddress).approve(gateway, amount);    

contracts\deposit-service\ReceiverImplementation.sol
  38: IERC20(tokenAddress).approve(gateway, amount);    
  64: IERC20(wrappedTokenAddress).approve(gateway, amount);

[N-01] INCONSISTENT RETURN VALUE CHECK FOR CALL()

For checking the return value of the call function, most code check success && (returnData.length == uint256(0) || abi.decode(returnData, (bool))) but the following code checks differently. For readability and maintainability, please consider changing its checking condition to be consistent with others.

contracts\AxelarGateway.sol
  388: if (!success || (returnData.length != uint256(0) && !abi.decode(returnData, (bool)))) revert BurnFailed(symbol);

[N-02] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic number in the following code with a constant.

contracts\deposit-service\DepositBase.sol
  32: if (symbolBytes.length == 0 || symbolBytes.length > 31) revert InvalidSymbol();

[N-03] PUBLIC FUNCTIONS NOT CALLED BY OWN CONTRACT CAN BE EXTERNAL

For public functions that are not called by their own contract, their visibilities can be set to external for readability and maintainability.

contracts\deposit-service\AxelarDepositService.sol
  241: function contractId() public pure returns (bytes32) {

xc20\contracts\XC20Wrapper.sol
  40: function contractId() public pure returns (bytes32) {

[N-04] INCOMPLETE NATSPEC COMMENTS

NatSpec provides rich documentation for code. @param and/or @return are missing for the following NatSpec functions.

contracts\deposit-service\AxelarDepositService.sol
  23: function sendNative(string calldata destinationChain, string calldata destinationAddress) external payable {     
  35: function addressForTokenDeposit(     
  56: function addressForNativeDeposit(     
  75: function addressForNativeUnwrap(     
  85: function sendTokenDeposit(     
  106: function refundTokenDeposit(     
  138: function sendNativeDeposit(     
  157: function refundNativeDeposit(     
  185: function nativeUnwrap(     
  198: function refundNativeUnwrap(     

contracts\deposit-service\DepositBase.sol
  46: function wrappedSymbol() public view returns (string memory symbol) {     

contracts\deposit-service\ReceiverImplementation.sol
  16: function receiveAndSendToken(     
  44: function receiveAndSendNative(     
  70: function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external {     

[N-05] MISSING NATSPEC COMMENTS

NatSpec provides rich documentation for code. NatSpec comments are missing for the following contracts and functions.

contracts\auth\AxelarAuthWeighted.sol
  26: function validateProof(bytes32 messageHash, bytes calldata proof) external view returns (bool currentOperators) {  
  47: function transferOperatorship(bytes calldata params) external onlyOwner {  
  55: function _transferOperatorship(bytes memory params) internal {  
  86: function _validateSignatures(  
  115: function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {  

contracts\deposit-service\AxelarDepositService.sol
  220: function _depositAddress(bytes32 create2Salt, bytes memory delegateData) internal view returns (address) {  
  241: function contractId() public pure returns (bytes32) {  ;

contracts\deposit-service\AxelarDepositServiceProxy.sol
  8: function contractId() internal pure override returns (bytes32) {  

contracts\deposit-service\DepositBase.sol
  41: function wrappedToken() public view returns (address) {  
  65: function _safeTransfer(  

contracts\gas-service\AxelarGasService.sol
  12: function payGasForContractCall(  
  35: function payGasForContractCallWithToken(  
  62: function payNativeGasForContractCall(  
  75: function payNativeGasForContractCallWithToken(  
  98: function addGas(  
  110: function addNativeGas(  
  120: function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {  
  136: function refund(     
  150: function _safeTransfer(  
  164: function _safeTransferFrom(  
  180: function contractId() external pure returns (bytes32) {  

contracts\gas-service\AxelarGasServiceProxy.sol
  8: contract AxelarGasServiceProxy is Proxy {  

xc20\contracts\XC20Wrapper.sol
  30: function gateway() public view override returns (IAxelarGateway) {  
  34: function _setup(bytes calldata data) internal override {  
  40: function contractId() public pure returns (bytes32) {  ;
  44: function setXc20Codehash(bytes32 newCodehash) external onlyOwner {  
  48: function addWrapping(  
  66: function removeWrapping(string calldata symbol) external onlyOwner {  
  75: function wrap(address axelarToken, uint256 amount) external {  
  82: function unwrap(address wrappedToken, uint256 amount) external {  
  90: function _safeTransfer(  
  101: function _safeTransferFrom(  
  114: function _executeWithToken(

[G-01] REVERT CHECK ON INPUT CAN BE PLACED AT START OF FUNCTION BODY

When a revert check on the input is placed at the start of the function body, the subsequent operations that cost more gas are prevented from running if it does revert.

For the following code, if (gatewayImplementation.code.length == 0) revert InvalidImplementation(); can be placed above _setAddress(KEY_IMPLEMENTATION, gatewayImplementation);.

contracts\AxelarGatewayProxy.sol
  16-24:
    constructor(address gatewayImplementation, bytes memory params) {
        _setAddress(KEY_IMPLEMENTATION, gatewayImplementation);

        if (gatewayImplementation.code.length == 0) revert InvalidImplementation();

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

        if (!success) revert SetupFailed();
    }

[G-02] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

contracts\AxelarGateway.sol
  207: for (uint256 i = 0; i < symbols.length; i++) {

contracts\auth\AxelarAuthWeighted.sol
  69: for (uint256 i = 0; i < weightsLength; ++i) {
  98: for (uint256 i = 0; i < signatures.length; ++i) {

[G-03] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, symbols.length in the following code can be cached outside of the loop like uint256 symbolsLength = symbols.length, and i < symbolsLength can be used for each iteration.

contracts\AxelarGateway.sol
  207: for (uint256 i = 0; i < symbols.length; i++) {

contracts\auth\AxelarAuthWeighted.sol
  17: for (uint256 i; i < recentOperators.length; ++i) {
  98: for (uint256 i = 0; i < signatures.length; ++i) {
  116: for (uint256 i; i < accounts.length - 1; ++i) {

contracts\deposit-service\AxelarDepositService.sol
  114: for (uint256 i; i < refundTokens.length; i++) {
  168: for (uint256 i; i < refundTokens.length; i++) {
  204: for (uint256 i; i < refundTokens.length; i++) {

contracts\gas-service\AxelarGasService.sol
  123: for (uint256 i; i < tokens.length; i++) {

[G-04] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

contracts\AxelarGateway.sol
  207: for (uint256 i = 0; i < symbols.length; i++) {

contracts\deposit-service\AxelarDepositService.sol
  114: for (uint256 i; i < refundTokens.length; i++) {
  168: for (uint256 i; i < refundTokens.length; i++) {
  204: for (uint256 i; i < refundTokens.length; i++) {

contracts\gas-service\AxelarGasService.sol
  123: for (uint256 i; i < tokens.length; i++) {

[G-05] X = X + Y CAN BE USED INSTEAD OF X += Y

x = x + y costs less gas than x += y. For example, totalWeight += newWeights[i] can be changed to totalWeight = totalWeight + newWeights[i] in the following code.

contracts\auth\AxelarAuthWeighted.sol
  70: totalWeight += newWeights[i];
  105: weight += weights[operatorIndex];

[G-06] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, if increasing the counter variable is very unlikely to overflow, then unchecked {++i} at the end of the loop block can be used, where i is the counter variable.

contracts\AxelarGateway.sol
  195: for (uint256 i; i < adminCount; ++i) {
  207: for (uint256 i = 0; i < symbols.length; i++) {
  292: for (uint256 i; i < commandsLength; ++i) {

contracts\auth\AxelarAuthWeighted.sol
  17: for (uint256 i; i < recentOperators.length; ++i) {
  69: for (uint256 i = 0; i < weightsLength; ++i) {
  98: for (uint256 i = 0; i < signatures.length; ++i) {
  101: for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
  116: for (uint256 i; i < accounts.length - 1; ++i) {

contracts\deposit-service\AxelarDepositService.sol
  114: for (uint256 i; i < refundTokens.length; i++) {
  168: for (uint256 i; i < refundTokens.length; i++) {
  204: for (uint256 i; i < refundTokens.length; i++) {

contracts\gas-service\AxelarGasService.sol
  123: for (uint256 i; i < tokens.length; i++) {

[G-07] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REVERT() WITH REASON STRING

revert with custom error can cost less gas than revert() with reason string. Please consider using revert with custom error to replace the following revert().

xc20\contracts\XC20Wrapper.sol
  55: if (axelarToken == address(0)) revert('NotAxelarToken()');
  56: if (xc20Token.codehash != xc20Codehash) revert('NotXc20Token()');
  57: if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');
  58: if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');
  61: if (!LocalAsset(xc20Token).set_team(address(this), address(this), address(this))) revert('NotOwner()');
  62: if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');
  68: if (axelarToken == address(0)) revert('NotAxelarToken()');
  70: if (xc20Token == address(0)) revert('NotWrappingToken()');
  78: if (wrappedToken == address(0)) revert('NotAxelarToken()');
  79: if (!LocalAsset(wrappedToken).mint(msg.sender, amount)) revert('CannotMint()');
  84: if (axelarToken == address(0)) revert('NotXc20Token()');
  85: if (IERC20(wrappedToken).balanceOf(msg.sender) < amount) revert('InsufficientBalance()');
  86: if (!LocalAsset(wrappedToken).burn(msg.sender, amount)) revert('CannotBurn()');
  98: if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
  111: if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');