Axelar Network v2 contest - defsec's results

ISSUE LIST

C4-001 : Low level calls with solidity version 0.8.x can result in optimizer bug. - LOW

C4-002 : The Contract Should safeApprove(0) first - LOW

C4-003 : Centralization Risk - Low

C4-004 : Missing zero-address check in the setter functions in the constructor - Low

C4-005 : Use of Self-Destruct - Low

C4-006 : Implement check effect interaction- Low

C4-007 : Contract should have pause/unpause functionality - Low

C4-008 : transferOwnership should be two step process - Low

C4-009 : ERC20 approve method missing return value check - LOW

C4-010 : DelegateCall to self destruct on implementation contract - LOW

C4-001 : Low level calls with solidity version 0.8.x can result in optimizer bug.

Impact

The protocol is using low level calls with solidity version 0.8.x which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Proof of Concept

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.15.

C4-002 : The Contract Should safeApprove(0) first - LOW

Impact

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

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

When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-003 : Centralization Risk

Impact - LOW

Owner role has absolute power across the contracts with several onlyOwner functions. There is no ability to change admin to a new address or renounce it which is helpful for lost/compromised admin keys or to delegate control to a different governance/DAO address in future.

The project does not use the widely used OpenZeppelin Ownable library which provides transfer/renounce functions to mitigate such compromised/accidental situations with admin keys. This makes admin role/key a single-point of failure.

Location

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L136

Recommended Mitigation Steps

Ensure admins are reasonably redundant/independent (3/7 or 5/9) multisigs and add transfer/renounce functionality for admin. Consider using OpenZeppelin’s Ownable library.

C4-004 : Missing zero-address check in the setter functions in the constructor - Low

Impact

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L12

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L110

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

C4-005 : Use of Self-Destruct - Low

Impact

Selfdestruct is a function in the Solidity smart contract used to delete contracts on the blockchain. When a contract executes a self-destruct operation, the remaining ether on the contract account will be sent to a specified target, and its storage and code are erased. The self-destruct function is a double-edged sword. However, using the self.destruct function can break the contract and leads to loss of functionality.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/DepositHandler.sol#L29

Tools Used

Code Review

Recommended Mitigation Steps

Consider reviewing self.destruct functionality.

C4-006 : Implement check effect interaction

Impact - LOW

There is no impact to the funds but to align with best practices, it is always better to update internal state before any external function calls.

Location

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

Recommended Mitigation Steps

Call external interaction before accounting. (Update the internal accounting before transferring the tokens out.)

C4-007 : Contract should have pause/unpause functionality

Impact

In case a hack is occuring or an exploit is discovered, the team should be able to pause functionality until the necessary changes are made to the system.

To use a thorchain example again, the team behind thorchain noticed an attack was going to occur well before the system transferred funds to the hacker. However, they were not able to shut the system down fast enough. (According to the incidence report here: https://github.com/HalbornSecurity/PublicReports/blob/master/Incident%20Reports/Thorchain_Incident_Analysis_July_23_2021.pdf)

Proof of Concept

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

Tools Used

Code Review

Recommended Mitigation Steps

Pause functionality on the contract would have helped secure the funds quickly.

C4-008 : # transferOwnership should be two step process

Impact

The contracts inherit OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasService.sol#L120".
2. The contracts have many onlyOwner function.
3. The contract is inherited from the Ownable which includes transferOwnership.

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

C4-009 : ERC20 approve method missing return value check - LOW

Impact

The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

1. Navigate to the following contracts.

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

2. Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

C4-010 : DelegateCall to self destruct on implementation contract - LOW

Impact

A mixture of Failing to calling init the implementation contract after it is deployed can leads to self-destruct through proxy.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/gas-service/AxelarGasServiceProxy.sol
https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/util/Proxy.sol#L26

Tools Used

None

Recommended Mitigation Steps

Ensure that init function called.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

ISSUE LIST

C4-001 : Adding unchecked directive can save gas [S]

C4-002 : Check if amount > 0 before token transfer can save gas [S]

C4-003 : There is no need to assign default values to variables [S]

C4-004 : Using operator && used more gas [S]

C4-005 : Non-strict inequalities are cheaper than strict ones [M]

C4-006 : Cache array length in for loops can save gas [S]

C4-007 : Use calldata instead of memory for function parameters [M]

C4-008 : ++i is more gas efficient than i++ in loops forwarding

C4-009 : > 0 can be replaced with != 0 for gas optimization

C4-010 : Keccak functions in constants waste gas [M]

C4-011 : Free gas savings for using solidity 0.8.10+ [S]

C4-001 : Adding unchecked directive can save gas

Impact

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

  2022-07-axelar-main/contracts/auth/AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {

Tools Used

None

Recommended Mitigation Steps

Consider applying unchecked arithmetic where overflow/underflow is not possible. Example can be seen from below.

Unchecked{i++};

C4-002 : Check if amount > 0 before token transfer can save gas

Impact

Since _amount can be 0. Checking if (_amount != 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L87

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider checking amount != 0.

C4-003 : There is no need to assign default values to variables

Impact - Gas Optimization

Uint is default initialized to 0. There is no need assign false to variable.

Proof of Concept

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

Tools Used

Code Review

Recommended Mitigation Steps

uint x = 0 costs more gas than uint x without having any different functionality.

C4-004 : Using operator && used more gas

Impact

Using double require instead of operator && can save more gas.

Proof of Concept

1. Navigate to the following contracts.

2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol:159:        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol:175:        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
2022-07-axelar-main/contracts/deposit-service/DepositBase.sol:72:        bool transferred = success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol:118:            if (refundTokens[i] == gatewayToken && msg.sender != refundAddress) continue;
2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol:165:        if (addressForNativeDeposit(salt, refundAddress, destinationChain, destinationAddress).balance > 0 && msg.sender != refundAddress)
2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol:208:            if (refundTokens[i] == wrappedTokenAddress && msg.sender != refundAddress) continue;
2022-07-axelar-main/contracts/AxelarGateway.sol:388:            if (!success || (returnData.length != uint256(0) && !abi.decode(returnData, (bool)))) revert BurnFailed(symbol);
2022-07-axelar-main/contracts/AxelarGateway.sol:462:        return success && (returnData.length == uint256(0) || abi.decode(returnData, (bool)));
2022-07-axelar-main/contracts/AxelarGateway.sol:613:        if (limit > 0 && amount > limit) revert ExceedDailyMintLimit(symbol);

Tools Used

Code Review

Recommended Mitigation Steps

Example

using &&:

function check(uint x)public view{
require(x == 0 && x < 1 );
}
// gas cost 21630

using double require:

require(x == 0 );
require( x < 1);
}
}
// gas cost 21622

C4-005 : Non-strict inequalities are cheaper than strict ones

Impact

Strict inequalities add a check of non equality which costs around 3 gas.

Proof of Concept

  2022-07-axelar-main/contracts/auth/AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {

Tools Used

Code Review

Recommended Mitigation Steps

Use >= or <= instead of > and < when possible.

C4-006 : Cache array length in for loops can save gas

Impact

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

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

1. Navigate to the following smart contract line.

  2022-07-axelar-main/contracts/auth/AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {

Tools Used

None

Recommended Mitigation Steps

Consider to cache array length.

C4-008 : Use calldata instead of memory for function parameters

Impact

In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

contract C {
function add(uint[] memory arr) external returns (uint sum) {
 uint length = arr.length;
 for (uint i = 0; i < arr.length; i++) {
     sum += arr[i];
 }
}
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
function add(uint[] calldata arr) external returns (uint sum) {
 uint length = arr.length;
 for (uint i = 0; i < arr.length; i++) {
     sum += arr[i];
 }
}
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

Proof of Concept

1. Navigate to the following smart contract line.

2022-07-axelar-main/contracts/auth/AxelarAuthWeighted.sol:56:        (address[] memory newOperators, uint256[] memory newWeights, uint256 newThreshold) = abi.decode(

Tools Used

None

Recommended Mitigation Steps

Some parameters in examples given above are later hashed. It may be beneficial for those parameters to be in memory rather than calldata.

C4-009 : ++i is more gas efficient than i++ in loops forwarding

Impact

++i is more gas efficient than i++ in loops forwarding.

Proof of Concept

1. Navigate to the following contracts.

  2022-07-axelar-main/contracts/auth/AxelarAuthWeighted.sol::116 => for (uint256 i; i < accounts.length - 1; ++i) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::114 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::168 => for (uint256 i; i < refundTokens.length; i++) {
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::204 => for (uint256 i; i < refundTokens.length; i++) {

Tools Used

Code Review

Recommended Mitigation Steps

It is recommend to use unchecked{++i} and change i declaration to uint256.

C4-010 : > 0 can be replaced with != 0 for gas optimization

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

1. Navigate to the following contract sections.

  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::165 => if (addressForNativeDeposit(salt, refundAddress, destinationChain, destinationAddress).balance > 0 && msg.sender != refundAddress)
  2022-07-axelar-main/contracts/deposit-service/ReceiverImplementation.sol::23 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar-main/contracts/deposit-service/ReceiverImplementation.sol::51 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
  2022-07-axelar-main/contracts/deposit-service/ReceiverImplementation.sol::71 => if (address(this).balance > 0) refundAddress.transfer(address(this).balance);

Tools Used

None

Recommended Mitigation Steps

Consider to replace > 0 with != 0 for gas optimization.

C4-011 : Keccak functions in constants waste gas

Impact

The contracts assigns two constants to the result of a keccak operation, which results in gas waste since the expression is computed each time the constant is accessed.

See this issue for more context: ethereum/solidity#9232 (https://github.com/ethereum/solidity/issues/9232)

Proof of Concept

  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::227 => keccak256(
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::233 => keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositService.sol::242 => return keccak256('axelar-deposit-service');
  2022-07-axelar-main/contracts/deposit-service/AxelarDepositServiceProxy.sol::9 => return keccak256('axelar-deposit-service');
  2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol::27 => keccak256(payload),
  2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol::52 => keccak256(payload),
  2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol::71 => emit NativeGasPaidForContractCall(sender, destinationChain, destinationAddress, keccak256(payload), msg.value, refundAddress);
  2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol::90 => keccak256(payload),
  2022-07-axelar-main/contracts/gas-service/AxelarGasService.sol::181 => return keccak256('axelar-gas-service');

Tools Used

None

Recommended Mitigation Steps

Replace the constant directive with immutable, or assign the already hashed value to the constants.

C4-012 : Free gas savings for using solidity 0.8.10+

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

Proof of Concept

All Contracts

Solidity 0.8.13 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

Solidity 0.8.13 has some improvements too but not well tested.

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.13.