Axelar Network v2 contest - Deivitto's results

QA

Low

Unused receive() function will lock ether in contract

Summary:

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

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/DepositReceiver.sol#L29

Mitigation:

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

Missing checks for address(0x0) when assigning values to address state variables

Summary

Zero address should be checked for state variables and some parameters in functions like mints, withdrawals... A zero address can lead into problems.

Github Permalinks

https://github.com/code-423n4/2022-07-axelar/blob/a1205d2ba78e0db583d136f8563e8097860a110f/xc20/contracts/XC20Wrapper.sol#L27

Mitigation

Check zero address for state variables before assigning to them a value Check in token/eth/permission assigned related the 0 address

Lack of permissions in critical function

Summary

While in the context of the project, this contract is only deployed and destroyed in the same scope, if for some reason somebody deploys an instance of the contract or inherits from this contract, any external user can trigger the suicidal function and get the ether of the contract.

Details

External function with suicidal can be exploited to get the ether of the contract.

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/DepositHandler.sol#L27-L30

Mitigation:

Create a source of access control so nobody deploys a contract with this suicidal function and a external actor exploits it.

Missing event for core paths/functions/parameters

Summary:

Events are important for critical parameter change and some paths of the code where ether/tokens are involved.

Github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/ReceiverImplementation.sol#L69-L87

Recommended Mitigation Steps

Public external functions where money / tokens are transferred should emit some type of event for better monitoring

Informational

Redundant statement

Summary:

For example value of 3 in currentEpoch uint epoch = currentEpoch + 1 // => 4 currentEpoch = epoch // => 4

This code could be simplified to : uint epoch = ++currentEpoch;

in one step both values would be 4 also the state variable is cached in a local variable

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L78-L82

Mitigation

Replace both lines to uint256 epoch = ++currentEpoch;

Missing indexed event parameters

Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/interfaces/IAxelarAuthWeighted.sol#L14

Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

block.timestamp used as time proxy

Summary:

Risk of using block.timestamp for time should be considered.

Details:

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.

Precision is not guaranteed using block.timestamp as it can be manipulated by miners to increase the value

References

SWC ID: 116

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L157 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L615

Mitigation:

Consider using an oracle for precision Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.

Missing Natspec

Summary:

Missing Natspec and regular comments affect readability and maintainability of a codebase.

Details:

Contracts has partial or full lack of comments. Adding Natspec to functions improves readability and maintainability. Also, public and external functions need it the most because they are shown in the devdoc section of the ABI.

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/interfaces/IDepositBase.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/interfaces/IAxelarAuth.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/interfaces/IAxelarGasService.sol https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/interfaces/IAxelarDepositService.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/interfaces/IAxelarAuthWeighted.sol

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasServiceProxy.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol# https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol# https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/ReceiverImplementation.sol https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/DepositReceiver.sol https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/DepositBase.sol https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/DepositHandler.sol#

https://github.com/code-423n4/2022-07-axelar/blob/a1205d2ba78e0db583d136f8563e8097860a110f/xc20/contracts/XC20Wrapper.sol

mitigation

Add @param descriptors Add @return descriptors Add Natspec comments where there is none Add inline comments. Add comments for what the contract does

Naming convention of state variable non constant

Summary:

Only constants are suggested to use style CONSTANTS_WITH_UNDERSCORES, other variables are suggested to use camelCase

Github Permalinks:

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L45-L46

Mitigation

Consider renaming to camelCase

        if (_getTokenType(symbol) == TokenType.External) {
            DepositHandler depositHandler = new DepositHandler{ salt: salt }();

            (bool success, bytes memory returnData) = depositHandler.execute(
                tokenAddress,
                abi.encodeWithSelector(IERC20.transfer.selector, address(this), IERC20(tokenAddress).balanceOf(address(depositHandler)))
            );

            if (!success || (returnData.length != uint256(0) && !abi.decode(returnData, (bool)))) revert BurnFailed(symbol);

            // NOTE: `depositHandler` must always be destroyed in the same runtime context that it is deployed.
            depositHandler.destroy(address(this));

GAS

Constants expressions are expressions, not constants.

summary

Constant expressions are left as expressions, not constants.

details

Reference to this kind of issue: https://github.com/ethereum/solidity/issues/9232

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L30-L43

mitigation

Use immutable for this expressions

Public function visibility can be made external

summary

Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.

details

If a function is never called from the contract it should be marked as external. This will save gas.

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L241-L243

mitigation

Consider changing visibility from public to external.

usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

summary

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

details

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size than downcast where needed

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L14

mitigation

Consider using some data type that uses 32 bytes, for example uint256

Explicit initialization

summary

It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.

details

If a variable is not set/initialized, it is assumed to have the default value ( 0 for uint, false for bool, address(0) for address…).

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L94-L95 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L68

mitigation

Don't initialize variables to default value

Index initialized in for loop

summary

In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L98 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L69 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L207

mitigation

Don't initialize variables to default value

use of i++ in loop rather than ++i

summary

++i costs less gas than i++, especially when it's used in for loops

details

using ++i doesn't affect the flow of regular for loops and improves gas cost

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L123 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L204 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L168 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L114 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L207

mitigation

Substitute to ++i

increments can be unchecked in loops

summary

Unchecked operations as the ++i on for loops are cheaper than checked one.

details

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..

The code would go from:

for (uint256 i; i < numIterations; i++) {
// ...
}

to

for (uint256 i; i < numIterations;) {
  // ...
  unchecked { ++i; }
}
The risk of overflow is inexistent for a uint256 here.

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L123 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L195 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L207 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L292 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L17 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L69 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L116 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L98 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L114 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L168 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L123 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L204

mitigation

Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

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

summary

In loops not assigning the length to a variable so memory accessed a lot (caching local variables)

details

The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas)

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L123 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L204 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L168 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L114 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L116 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L98 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L17 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/AxelarGateway.sol#L207

mitigation

Assign the length of the array.length to a local variable in loops for gas savings

Variable should be declared out of a loop if it has always the same value

summary

If a variable is redeclare all the time in a loop, it will cost a lot of gas.

details

I think this variable has always the same value as the function method doesn't include any kind of argument and nothing is modified in the state between the iterations

github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L205

Mitigation

Move the declaration before the for loop as it is always use the same value

Variables should be cached when used several times

summary

loop length variables assigned to a local variable improves gas usage

details

In loops or state variables, this is even more gas saving

github permalinks

cache refundTokens[i] https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L118-L121 https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L208-L210

mitigation

Cache variables used more than one into a local variable.

abi.encode() is more efficient than abi.encodePacked()

Summary

In general, abi.encodePacked is more gas-efficient.

Details

Changing the abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient.

Github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/a46fa61e73dd0f3469c0263bc6818e682d62fb5f/contracts/deposit-service/AxelarDepositService.sol#L228

mitigation

Recommended Mitigation Steps Change abi.encode to abi.encodePacked at line 355.

Functions guaranteed to revert when called by normal users can be marked payable

Summary

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.

Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Details

The extra opcodes avoided are: CALLVALUE (2), DUP1 (3), ISZERO (3), PUSH2 (3), JUMPI (10), PUSH1 (3), DUP1 (3), REVERT(0), JUMPDEST (1), POP (2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Github permalinks

https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/auth/AxelarAuthWeighted.sol#L47 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L120 https://github.com/code-423n4/2022-07-axelar/blob/3373c48a71c07cfce856b53afc02ef4fc2357f8c/contracts/gas-service/AxelarGasService.sol#L136

Mitigation

It's suggested to add payable to functions guaranteed to revert when called by normal users to improve gas costs