Axelar Network v2 contest - Rolezn's results

(1) IERC20 approve() Is Deprecated

Severity: Low

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

Proof Of Concept

IERC20(wrappedTokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L30

IERC20(tokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L38

IERC20(wrappedTokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L64

Recommended Mitigation Steps

Consider using safeIncreaseAllowance() / safeDecreaseAllowance() instead.

(2) The Contract Should Approve(0) First

Severity: Low

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.

Proof Of Concept

IERC20(wrappedTokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L30

IERC20(tokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L38

IERC20(wrappedTokenAddress).approve(gateway, amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L64

Recommended Mitigation Steps

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

(3) Missing Checks for Address(0x0)

Severity: Low

Proof Of Concept

function _setImplementation(address newImplementation) internal { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L655

function receiveAndUnwrapNative(address payable refundAddress, address payable recipient) external { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L70

Recommended Mitigation Steps

Consider adding zero-address checks in the mentioned codebase.

(4) Use Safetransfer Instead Of Transfer

Severity: Low

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity (https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call) finding from Consensys Diligence Audit of Fei Protocol.

Proof Of Concept

payable(msg.sender).transfer(address(this).balance); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/XC20Wrapper.sol#L63

if (address(this).balance > 0) refundAddress.transfer(address(this).balance); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L23

if (address(this).balance > 0) refundAddress.transfer(address(this).balance); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L51

if (address(this).balance > 0) refundAddress.transfer(address(this).balance); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L71

if (amount > 0) receiver.transfer(amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L128

receiver.transfer(amount); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L144

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

(5) Unused Receive() Function Will Lock Ether In Contract

Severity: Low

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

Proof Of Concept

receive() external payable {} https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/DepositReceiver.sol#L29

Recommended Mitigation Steps

The function should call another function, otherwise it should revert

(6) Constants Should Be Defined Rather Than Using Magic Numbers

Severity: Non-Critical

Proof Of Concept

    return getUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days));

https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L157

    _setUint(_getTokenDailyMintAmountKey(symbol, block.timestamp / 1 days), amount);

https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L615

(7) Event Is Missing Indexed Fields

Severity: Non-Critical

Each event should use three indexed fields if there are three or more fields. In addition, each event should have at least one indexed fields to allow easier filtering of logs.

Proof Of Concept

event OperatorshipTransferred(address[] newOperators, uint256[] newWeights, uint256 newThreshold); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarAuthWeighted.sol#L14

event GasPaidForContractCall( address indexed sourceAddress, string destinationChain, string destinationAddress, bytes32 indexed payloadHash, address gasToken, uint256 gasFeeAmount, address refundAddress ); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L13

event GasPaidForContractCallWithToken( address indexed sourceAddress, string destinationChain, string destinationAddress, bytes32 indexed payloadHash, string symbol, uint256 amount, address gasToken, uint256 gasFeeAmount, address refundAddress ); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L23

event NativeGasPaidForContractCall( address indexed sourceAddress, string destinationChain, string destinationAddress, bytes32 indexed payloadHash, uint256 gasFeeAmount, address refundAddress ); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L35

event NativeGasPaidForContractCallWithToken( address indexed sourceAddress, string destinationChain, string destinationAddress, bytes32 indexed payloadHash, string symbol, uint256 amount, uint256 gasFeeAmount, address refundAddress ); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L44

event GasAdded(bytes32 indexed txHash, uint256 indexed logIndex, address gasToken, uint256 gasFeeAmount, address refundAddress); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L55

event NativeGasAdded(bytes32 indexed txHash, uint256 indexed logIndex, uint256 gasFeeAmount, address refundAddress); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L57

(8) Use a more recent version of Solidity

Severity: Non-Critical

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Proof Of Concept

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/XC20Wrapper.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositServiceProxy.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/DepositBase.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/DepositReceiver.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/ReceiverImplementation.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasServiceProxy.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarAuth.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarAuthWeighted.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarDepositService.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IAxelarGasService.sol#L3

Found old version 0.8.9 of Solidity https://github.com/code-423n4/2022-07-golom/tree/main/contracts/interfaces/IDepositBase.sol#L3

Recommended Mitigation Steps

Consider updating to a more recent solidity version.

(1) Abi.encode() Is Less Efficient Than Abi.encodepacked()

Severity: Gas Optimizations

Proof Of Concept

return keccak256(abi.encode(PREFIX_CONTRACT_CALL_APPROVED, commandId, sourceChain, sourceAddress, contractAddress, payloadHash)); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L566

abi.encode( https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L580

bytes32 operatorsHash = keccak256(abi.encode(operators, weights, threshold)); https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L32

(2) <Array>.length Should Not Be Looked Up In Every Loop Of A For-loop

Severity: Gas Optimizations

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

Proof Of Concept

for (uint256 i = 0; i < symbols.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L207

for (uint256 i = 0; i < signatures.length; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L98

for (uint256 i; i < accounts.length - 1; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L116

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L114

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L168

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L204

for (uint256 i; i < tokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L123

(3) Empty Blocks Should Be Removed Or Emit Something

Severity: Gas Optimizations

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

Proof Of Concept

for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {} https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L101

receive() external payable override {} https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositServiceProxy.sol#L13

(4) ++i Costs Less Gas Than i++, Especially When It’s Used In For-loops (--i/i-- Too)

Severity: Gas Optimizations

Saves 6 gas per loop

Proof Of Concept

for (uint256 i = 0; i < symbols.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L207

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L114

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L168

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L204

for (uint256 i; i < tokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L123

Recommended Mitigation Steps

For example, use ++i instead of i++

(5) It Costs More Gas To Initialize Variables To Zero Than To Let The Default Of Zero Be Applied

Severity: Gas Optimizations

Proof Of Concept

for (uint256 i = 0; i < symbols.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L207

for (uint256 i = 0; i < weightsLength; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L69

for (uint256 i = 0; i < signatures.length; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L98

(6) Multiple Address Mappings Can Be Combined Into A Single Mapping Of An Address To A Struct, Where Appropriate

Severity: Gas Optimizations

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

Proof Of Concept

mapping(address => address) public wrapped; mapping(address => address) public unwrapped;

https://github.com/code-423n4/2022-07-golom/tree/main/contracts/XC20Wrapper.sol#L20

(7) <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables

Severity: Gas Optimizations

Proof Of Concept

totalWeight += newWeights[i]; https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L70

weight += weights[operatorIndex]; https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L105

(8) ++i/i++ Should Be Unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops

Severity: Gas Optimizations

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

Proof Of Concept

for (uint256 i; i < adminCount; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L195

for (uint256 i = 0; i < symbols.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L207

for (uint256 i; i < commandsLength; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/AxelarGateway.sol#L292

for (uint256 i = 0; i < weightsLength; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L69

for (uint256 i = 0; i < signatures.length; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L98

for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {} https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L101

for (uint256 i = 0; i < signatures.length; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L98

for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {} https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L101

for (uint256 i; i < accounts.length - 1; ++i) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/auth/AxelarAuthWeighted.sol#L116

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L114

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L168

for (uint256 i; i < refundTokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/deposit-service/AxelarDepositService.sol#L204

for (uint256 i; i < tokens.length; i++) { https://github.com/code-423n4/2022-07-golom/tree/main/contracts/gas-service/AxelarGasService.sol#L123

abi.encodePacked('ab','c') == abi.encodePacked('a', 'bc')