Axelar Network v2 contest - bharg4v's results

QA REPORT

Number of Issues: 2

1. ERC20 return values aren't checked in some places

Impact

The ERC20.transferFrom(), ERC20.transfer(), ERC20.approve() functions return a boolean value indicating success. This parameter needs to be checked for success. But in the code below ,it is not checked at all .

One should use safeTransfer instead of transfer only. As there are popular tokens, such as USDT that transfer/transferFrom method does'nt return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must check the transferFrom return value

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L23

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L38

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

  2022-07-axelar/contracts/deposit-service/AxelarDepositService.sol::30 => IERC20(wrappedTokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::38 => IERC20(tokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::64 => IERC20(wrappedTokenAddress).approve(gateway, amount);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::86 => recipient.transfer(amount);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::128 => if (amount > 0) receiver.transfer(amount);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::144 => receiver.transfer(amount);
  2022-07-axelar/contracts/test/TestWeth.sol::23 => payable(msg.sender).transfer(amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapExecutable.sol::28 => IERC20(tokenA).approve(address(swapper), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapExecutable.sol::31 => IERC20(tokenB).approve(address(gateway), convertedAmount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapForecallable.sol::28 => IERC20(tokenA).approve(address(swapper), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainSwapForecallable.sol::31 => IERC20(tokenB).approve(address(gateway), convertedAmount);
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::24 => IERC20(tokenAddress).transferFrom(msg.sender, address(this), amount);
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::36 => IERC20(toTokenAddress).transfer(recipient, convertedAmount);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::37 => IERC20(tokenX).transferFrom(msg.sender, address(this), amount + gasFee);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::39 => IERC20(tokenX).approve(address(gasService), gasFee);
  2022-07-axelar/contracts/test/gmp/SourceChainSwapCaller.sol::52 => IERC20(tokenX).approve(address(gateway), amount);
  2022-07-axelar/xc20/contracts/XC20Wrapper.sol::63 => payable(msg.sender).transfer(address(this).balance);

Mitigation

We still recommend checking the return value as a best practice. Consider using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant staking tokens.

References

https://github.com/code-423n4/2021-12-nftx-findings/issues/141

https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

2. NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

Impact

Avoid floating pragmas for non-library contracts. While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain. It is recommended to pin to a concrete compiler version.

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/interfaces/IAxelarAuth.sol#L3

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/interfaces/IAxelarAuthWeighted.sol#L3

  2022-07-axelar/contracts/interfaces/IAxelarAuth.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarAuthWeighted.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarDepositService.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarExecutable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarForecallable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarGasService.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IAxelarGateway.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IBurnableMintableCappedERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IDepositBase.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20Burn.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20BurnFrom.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IERC20Permit.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IMintableCappedERC20.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IOwnable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IReceiverImplementation.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/ITokenDeployer.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IUpgradable.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/contracts/interfaces/IWETH9.sol::3 => pragma solidity ^0.8.9;
  2022-07-axelar/xc20/contracts/interfaces/LocalAsset.sol::3 => pragma solidity ^0.8.0;
  2022-07-axelar/xc20/contracts/interfaces/Permit.sol::3 => pragma solidity ^0.8.0;

Mitigation

Used a fixed compiler version

References

https://code4rena.com/reports/2022-06-nibbl/#n-16-non-libraryinterface-files-should-use-fixed-compiler-versions-not-floating-ones

Gas Optimizations Report

Table of Contents

1. Don't Initialize Variables with Default Value
2. Cache Array Length Outside of Loop
3. Use != 0 instead of > 0 for Unsigned Integer Comparison
4. Use Shift Right/Left instead of Division/Multiplication if possible

1. Don't Initialize Variables with Default Value

Impact

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

Example

Bad:

uint256 x = 0;
bool y = false;

Good:

uint256 x;
bool y;

Background Information

• Mudit Gupta's Blog

POC

  2022-07-axelar/contracts/AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::68 => uint256 totalWeight = 0;
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::69 => for (uint256 i = 0; i < weightsLength; ++i) {
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::94 => uint256 operatorIndex = 0;
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::95 => uint256 weight = 0;
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::98 => for (uint256 i = 0; i < signatures.length; ++i) { 

2. Cache Array Length Outside of Loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Example

Bad:

for (uint256 i = 0; i < array.length; i++) {
    // invariant: array's length is not changed
}

Good:

uint256 len = array.length
for (uint256 i = 0; i < len; i++) {
    // invariant: array's length is not changed
}

Background Information

• Issue submitted by WatchPug

POC

  2022-07-axelar/contracts/AdminMultisigBase.sol::149 => uint256 adminLength = accounts.length;
  2022-07-axelar/contracts/AxelarGateway.sol::49 => if (authModule.code.length == 0) revert InvalidAuthModule();
  2022-07-axelar/contracts/AxelarGateway.sol::50 => if (tokenDeployerImplementation.code.length == 0) revert InvalidTokenDeployer();
  2022-07-axelar/contracts/AxelarGateway.sol::205 => if (symbols.length != limits.length) revert InvalidSetDailyMintLimitsParams();
  2022-07-axelar/contracts/AxelarGateway.sol::207 => for (uint256 i = 0; i < symbols.length; i++) {
  2022-07-axelar/contracts/AxelarGateway.sol::228 => if (setupParams.length != 0) {
  2022-07-axelar/contracts/AxelarGateway.sol::255 => if (newOperatorsData.length > 0) {
  2022-07-axelar/contracts/AxelarGateway.sol::288 => uint256 commandsLength = commandIds.length;
  2022-07-axelar/contracts/AxelarGateway.sol::290 => if (commandsLength != commands.length || commandsLength != params.length) revert InvalidCommands();
  2022-07-axelar/contracts/AxelarGateway.sol::355 => if (tokenAddress.code.length == uint256(0)) revert TokenContractDoesNotExist(tokenAddress);
  2022-07-axelar/contracts/AxelarGateway.sol::388 => if (!success || (returnData.length != uint256(0) && !abi.decode(returnData, (bool)))) revert BurnFailed(symbol);
  2022-07-axelar/contracts/AxelarGateway.sol::462 => return success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
  2022-07-axelar/contracts/AxelarGatewayProxy.sol::19 => if (gatewayImplementation.code.length == 0) revert InvalidImplementation();
  2022-07-axelar/contracts/DepositHandler.sol::23 => if (callee.code.length == 0) revert NotContract();
  2022-07-axelar/contracts/ECDSA.sol::32 => // Check the signature length
  2022-07-axelar/contracts/ECDSA.sol::33 => if (signature.length != 65) revert InvalidSignatureLength();
  2022-07-axelar/contracts/ECDSA.sol::75 => // 32 is the length in bytes of hash, 

3. Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0.

Example

Bad:

// `a` being of type unsigned integer
require(a > 0, "!a > 0");

Good:

// `a` being of type unsigned integer
require(a != 0, "!a > 0");

POC

  2022-07-axelar/contracts/AxelarGateway.sol::255 => if (newOperatorsData.length > 0) {
  2022-07-axelar/contracts/AxelarGateway.sol::613 => if (limit > 0 && amount > limit) revert ExceedDailyMintLimit(symbol);
  2022-07-axelar/contracts/ECDSA.sol::58 => if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) revert InvalidS();
  2022-07-axelar/contracts/ERC20Permit.sol::45 => if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) revert InvalidS();
  2022-07-axelar/contracts/auth/AxelarAuthWeighted.sol::76 => if (epochForHash[newOperatorsHash] > 0) revert SameOperators();
  2022-07-axelar/contracts/deposit-service/AxelarDepositService.sol::165 => if (addressForNativeDeposit(salt, refundAddress, destinationChain, destinationAddress).balance > 0 && msg.sender != refundAddress)
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/deposit-service/ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::128 => if (amount > 0) receiver.transfer(amount);
  2022-07-axelar/contracts/gas-service/AxelarGasService.sol::131 => if (amount > 0) _safeTransfer(token, receiver, amount);
  2022-07-axelar/contracts/util/Upgradable.sol::50 => if (params.length > 0) {
  2022-07-axelar/xc20/contracts/ERC20Permit.sol::45 => if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) revert InvalidS(); 

4. Use Shift Right/Left instead of Division/Multiplication if possible

Description

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

Impact

Bad:

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

Good:

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Background Information

• EVM Opcodes

POC

  2022-07-axelar/contracts/ECDSA.sol::56 => // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
  2022-07-axelar/contracts/ERC20.sol::15 => * https://forum.zeppelin.solutions/t/how-to-implement-erc20-supply-mechanisms/226[How
  2022-07-axelar/contracts/interfaces/IERC20.sol::49 => * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::29 => convertedAmount = amount * 2;
  2022-07-axelar/contracts/test/gmp/DestinationChainTokenSwapper.sol::33 => convertedAmount = amount / 2;
  2022-07-axelar/xc20/contracts/ERC20.sol::15 => * https://forum.zeppelin.solutions/t/how-to-implement-erc20-supply-mechanisms/226[How
  2022-07-axelar/xc20/contracts/interfaces/IERC20.sol::63 => * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729