Axelar Network v2 contest - 0x1f8b's results

• Low
  • 1. Outdated compiler
  • 2. Lack of checks
  • 3. Upgradable contracts without GAP can lead a upgrade disaster
  • 4. Ether could be blocked
  • 5. Unsafe ERC20 calls
• Non-Critical
  • 6. Packages with vulnerabilities
  • 7. Use encode instead of encodePacked for hashig

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.9;
pragma solidity  0.8.9;

The minimum required version must be 0.8.15; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• XC20Wrapper.sol#L27

In the following lines, ether is burned if address(0) is used as refundAddress:

• ReceiverImplementation.sol#L23
• ReceiverImplementation.sol#L71

3. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• XC20Wrapper.sol#L11
• AxelarGasService.sol#L10
• Upgradable.sol#L7
• AxelarDepositService.sol#L15

4. Ether could be blocked

The user's ether could be locked and unrecoverable.

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• XC20Wrapper.sol#L63
• ReceiverImplementation.sol#L23
• ReceiverImplementation.sol#L51
• ReceiverImplementation.sol#L71
• ReceiverImplementation.sol#L86
• AxelarGasService.sol#L128
• AxelarGasService.sol#L144

5. Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• EIP-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above). Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for approve:

• ReceiverImplementation.sol#L38
• ReceiverImplementation.sol#L64
• AxelarDepositService.sol#L30

---

Non-Critical

6. Packages with vulnerabilities

The project contains packages that urgently need to be updated because they contain important vulnerabilities.

55 vulnerabilities (15 moderate, 37 high, 3 critical)

npm audit report:

async  2.0.0 - 2.6.3
Severity: high
Prototype Pollution in async - https://github.com/advisories/GHSA-fwr7-v2mv-hh25
fix available via `npm audit fix`

cross-fetch  <=2.2.5 || 3.0.0 - 3.1.4 || >=3.2.0-alpha.0
Severity: high
Incorrect Authorization in cross-fetch - https://github.com/advisories/GHSA-7gc6-qh9x-w6h8
Depends on vulnerable versions of node-fetch
fix available via `npm audit fix`

elliptic  <6.5.4
Severity: moderate
Use of a Broken or Risky Cryptographic Algorithm - https://github.com/advisories/GHSA-r9p9-mrjm-926w
fix available via `npm audit fix`

got  <11.8.5
Severity: moderate
Got allows a redirect to a UNIX socket - https://github.com/advisories/GHSA-pfrx-2q88-qq97
No fix available

json-schema  <0.4.0
Severity: critical
json-schema is vulnerable to Prototype Pollution - https://github.com/advisories/GHSA-896r-f27r-55mw
fix available via `npm audit fix`

lodash  <=4.17.20
Severity: high
Command Injection in lodash - https://github.com/advisories/GHSA-35jh-r3h4-6jhm
Regular Expression Denial of Service (ReDoS) in lodash - https://github.com/advisories/GHSA-29mw-wpgm-hmr9
No fix available

minimist  <1.2.6
Severity: critical
Prototype Pollution in minimist - https://github.com/advisories/GHSA-xvch-5gv4-984h
fix available via `npm audit fix`

node-fetch  <=2.6.6
Severity: high
node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://github.com/advisories/GHSA-r683-j2x4-v87g
The `size` option isn't honored after following a redirect in node-fetch - https://github.com/advisories/GHSA-w7rc-rwvf-8q5r
No fix available

normalize-url  4.3.0 - 4.5.0
Severity: high
ReDoS in normalize-url - https://github.com/advisories/GHSA-px4h-xg32-q955
fix available via `npm audit fix`

path-parse  <1.0.7
Severity: moderate
Regular Expression Denial of Service in path-parse - https://github.com/advisories/GHSA-hj48-42vr-x3v9
fix available via `npm audit fix`

simple-get  <2.8.2
Severity: high
Exposure of Sensitive Information in simple-get - https://github.com/advisories/GHSA-wpg7-2c88-r8xv
fix available via `npm audit fix`

tar  <=4.4.17
Severity: high
Arbitrary File Creation/Overwrite on Windows via insufficient relative path sanitization - https://github.com/advisories/GHSA-5955-9wpr-37jh
Arbitrary File Creation/Overwrite via insufficient symlink protection due to directory cache poisoning using symbolic links - https://github.com/advisories/GHSA-qq89-hq3f-393p
Arbitrary File Creation/Overwrite via insufficient symlink protection due to directory cache poisoning using symbolic links - https://github.com/advisories/GHSA-9r2w-394v-53qc
Arbitrary File Creation/Overwrite due to insufficient absolute path sanitization - https://github.com/advisories/GHSA-3jfq-g458-7qm9
Arbitrary File Creation/Overwrite via insufficient symlink protection due to directory cache poisoning - https://github.com/advisories/GHSA-r628-mhmh-qjhw
fix available via `npm audit fix`

underscore  1.3.2 - 1.12.0
Severity: high
Arbitrary Code Execution in underscore - https://github.com/advisories/GHSA-cf4h-3jhx-xvhq
No fix available

undici  <=5.7.0
Severity: moderate
undici before v5.8.0 vulnerable to CRLF injection in request headers - https://github.com/advisories/GHSA-3cvr-822r-rqcc
undici before v5.8.0 vulnerable to uncleared cookies on cross-host / cross-origin redirect - https://github.com/advisories/GHSA-q768-x9m6-m9qp
fix available via `npm audit fix`

ws  5.0.0 - 5.2.2
Severity: moderate
ReDoS in Sec-Websocket-Protocol header - https://github.com/advisories/GHSA-6fc8-4gx4-v693
fix available via `npm audit fix`

yargs-parser  <=5.0.0
Severity: moderate
yargs-parser Vulnerable to Prototype Pollution - https://github.com/advisories/GHSA-p9pc-299p-vxgp
No fix available

7. Use encode instead of encodePacked for hashig

Use of abi.encodePacked is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• AxelarGateway.sol#L540-L556

• Gas
  • 1. Cache keccak256 results in immutable variables
  • 2. delete optimization
  • 3. Don't use the length of an array for loops condition
  • 4. There's no need to set default values for variables
  • 5. ++i costs less gas compared to i++ or i += 1
  • 6. constants expressions are expressions, not constants

Gas

1. Cache keccak256 results in immutable variables

There are calls to keccak256 with constant arguments that could be cached to an immutable variable in order to save gas.

Affected source code:

• XC20Wrapper.sol#L41
• AxelarDepositServiceProxy.sol#L9
• AxelarGasServiceProxy.sol#L10
• AxelarGasService.sol#L181
• AxelarDepositService.sol#L242

2. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

• XC20Wrapper.sol#L71-L72

3. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• AxelarAuthWeighted.sol#L17
• AxelarAuthWeighted.sol#L98
• AxelarAuthWeighted.sol#L116
• AxelarGasService.sol#L123
• AxelarDepositService.sol#L114
• AxelarDepositService.sol#L168
• AxelarDepositService.sol#L204
• AxelarGateway.sol#L207

4. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• AxelarAuthWeighted.sol#L68-L69
• AxelarAuthWeighted.sol#L94-L98
• AxelarGateway.sol#L207

5. ++i costs less gas compared to i++ or i += 1

++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 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• AxelarGasService.sol#L123
• AxelarDepositService.sol#L114
• AxelarDepositService.sol#L168
• AxelarDepositService.sol#L204
• AxelarGateway.sol#L207

6. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Affected source code:

• AxelarGateway.sol#L30-L43