Axelar Network v2 contest - c3phas's results

QA

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

File: DepositBase.sol line 24

        gateway = gateway_;

File: XC20Wrapper.sol line 27

        gatewayAddress = gatewayAddress_;

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

File: DepositBase.sol line 41

    function wrappedToken() public view returns (address) {
        return IAxelarGateway(gateway).tokenAddresses(wrappedSymbol());
    }

File: XC20Wrapper.sol line 40-42

    function contractId() public pure returns (bytes32) {
        return keccak256('xc20-wrapper');
    }

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: DepositBase.sol line 50 0xff

        uint256 length = 0xff & uint256(symbolData);

File: DepositBase.sol line 37 0xff

        symbolNumber |= 0xff & symbolBytes.length;

Typos

File: ReceiverImplementation.sol line 37

        // Sending the token trough the gateway

trough instead of through

FINDINGS

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File: AxelarGasService.sol line 123

    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
        if (receiver == address(0)) revert InvalidAddress();


        for (uint256 i; i < tokens.length; i++) {

Other Instances to modify File: AxelarDepositService.sol line 114

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 168

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 204

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarAuthWeighted.sol line 17

        for (uint256 i; i < recentOperators.length; ++i) {

File: AxelarAuthWeighted.sol line 69

        for (uint256 i = 0; i < weightsLength; ++i) {

File: AxelarAuthWeighted.sol line 98

        for (uint256 i = 0; i < signatures.length; ++i) {

File: AxelarAuthWeighted.sol line 116

        for (uint256 i; i < accounts.length - 1; ++i) {

see resource

Cache the length of arrays in loops (saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: AxelarGasService.sol line 123

    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
        if (receiver == address(0)) revert InvalidAddress();


        for (uint256 i; i < tokens.length; i++) {

The above should be modified to

    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
        if (receiver == address(0)) revert InvalidAddress();

                uint256 length = tokens.length;
        for (uint256 i; i < length; i++) {

Other instances to modify File: AxelarDepositService.sol line 114

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 168

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 204

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarAuthWeighted.sol line 17

        for (uint256 i; i < recentOperators.length; ++i) {

File: AxelarAuthWeighted.sol line 98

        for (uint256 i = 0; i < signatures.length; ++i) {

File: AxelarAuthWeighted.sol line 116

        for (uint256 i; i < accounts.length - 1; ++i) {

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include: File: AxelarGasService.sol line 123

        for (uint256 i; i < tokens.length; i++) {

File: AxelarDepositService.sol line 114

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 168

        for (uint256 i; i < refundTokens.length; i++) {

File: AxelarDepositService.sol line 204

        for (uint256 i; i < refundTokens.length; i++) {

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source

All the contracts in scope are already using custom errors apart from File: XC20Wrapper.sol in the following lines

File: XC20Wrapper.sol line 55

        if (axelarToken == address(0)) revert('NotAxelarToken()');

File: XC20Wrapper.sol line 56

        if (xc20Token.codehash != xc20Codehash) revert('NotXc20Token()');

File: XC20Wrapper.sol line 57

        if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');

File: XC20Wrapper.sol line 58

        if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');

File: XC20Wrapper.sol line 61

        if (!LocalAsset(xc20Token).set_team(address(this), address(this), address(this))) revert('NotOwner()');

File: XC20Wrapper.sol line 62

        if (!LocalAsset(xc20Token).set_metadata(newName, newSymbol, IERC20(axelarToken).decimals())) revert('CannotSetMetadata()');

More instances

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L68 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L70 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L78 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L79 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L84 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L85 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L86 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L98 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L111