QA Report

[code423n4]

• 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

[re1ro]

1

We are not using the latest version for security reason. Non of the described bug fixes are affecting us, but there might new bugs in 0.8.15 that are not discovered yet

2

Ack

#3
Dup #102

4

Dup #4

5

Dup #3

6

Ack

7

Dup #8 (5)

[GalloDaSballo]

1. Outdated compiler

Valid R because it's explained (NC without)

2. Lack of checks

Lack of 0 check, valid Low

3. Upgradable contracts without GAP can lead a upgrade disaster

Per the dispute of #102 , in lack of any detail (finding is copy pasted from work done by ||||), am not considering

4. Ether could be blocked

Valid Low

5. Unsafe ERC20 calls

L

6. Packages with vulnerabilities

NC

7. Use encode instead of encodePacked for hashig

No clash was demonstrated, not valid

3L 1R 1NC

[0x1f8b]

3. Upgradable contracts without GAP can lead a upgrade disaster

Per the dispute of #102 , in lack of any detail (finding is copy pasted from work done by ||||), am not considering

@GalloDaSballo What do you mean with finding is copy pasted from work done by ||||?

I remember you that I didn't have access to this repository until contest end, so please tell me how can it be a copy paste.