Axelar Network v2 contest - Sm4rty's results

1. USE SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().

Instances:

contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64

contracts/deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);

Recommended Mitigation Steps

Update to safeapprove in the function.

---

2. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

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.

Instances:

contracts/gas-service/AxelarGasService.sol: L173 L128 L144

contracts/gas-service/AxelarGasService.sol:173:            abi.encodeWithSelector(IERC20.transferFrom.selector, from, address(this), amount)
contracts/gas-service/AxelarGasService.sol:128:                if (amount > 0) receiver.transfer(amount);
contracts/gas-service/AxelarGasService.sol:144:            receiver.transfer(amount);

contracts/deposit-service/ReceiverImplementation.sol: L23 L51 L71 L86

contracts/deposit-service/ReceiverImplementation.sol:23:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
contracts/deposit-service/ReceiverImplementation.sol:51:            if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
contracts/deposit-service/ReceiverImplementation.sol:71:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
contracts/deposit-service/ReceiverImplementation.sol:86:        recipient.transfer(amount);

xc20/contracts/XC20Wrapper.sol: L63 L107

xc20/contracts/XC20Wrapper.sol:63:        payable(msg.sender).transfer(address(this).balance);
xc20/contracts/XC20Wrapper.sol:107:            abi.encodeWithSelector(IERC20.transferFrom.selector, from, address(this), amount)

contracts/AxelarGateway.sol: L501 L523

contracts/AxelarGateway.sol:501:                abi.encodeWithSelector(IERC20.transferFrom.selector, sender, address(this), amount)
contracts/AxelarGateway.sol:523:                IERC20.transferFrom.selector,

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

---

3. The Contract Should Approve(0) first

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.

IERC20(token).approve(address(operator), 0); IERC20(token).approve(address(operator), amount);

Proof of Concept

contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64

contracts/deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);

Recommended Mitigation Steps

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

---

4. Missing checks for approve()’s return status

Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin’s safeApprove(), which reverts if there’s a failure, instead

Instances:

contracts/deposit-service/AxelarDepositService.sol: L30, L38, L64

contracts/deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
contracts/deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);

Reference:

https://code4rena.com/reports/2022-05-sturdy#l-03-missing-checks-for-approves-return-status

---

5. USE OF FLOATING PRAGMA

Recommend using fixed solidity version

Instances

contracts/interfaces/IDepositBase.sol contracts/interfaces/IAxelarGasService.sol contracts/interfaces/IAxelarAuth.sol contracts/interfaces/IAxelarAuthWeighted.sol contracts/interfaces/IAxelarDepositService.sol

contracts/interfaces/IDepositBase.sol:3:pragma solidity ^0.8.9;
contracts/interfaces/IAxelarGasService.sol:3:pragma solidity ^0.8.9;
contracts/interfaces/IAxelarAuth.sol:3:pragma solidity ^0.8.9;
contracts/interfaces/IAxelarAuthWeighted.sol:3:pragma solidity ^0.8.9;
contracts/interfaces/IAxelarDepositService.sol:3:pragma solidity ^0.8.9;

---

6. Used receive() function will lock Ether in contract

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

Instances:

contracts/deposit-service/AxelarDepositServiceProxy.sol:13 contracts/deposit-service/DepositReceiver.sol:29

contracts/deposit-service/AxelarDepositServiceProxy.sol:13:    receive() external payable override {}
contracts/deposit-service/DepositReceiver.sol:29:    receive() external payable {}

1. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0;

Instance Includes:

contracts/auth/AxelarAuthWeighted.sol:68 contracts/auth/AxelarAuthWeighted.sol:94 contracts/auth/AxelarAuthWeighted.sol:95

contracts/auth/AxelarAuthWeighted.sol:68:        uint256 totalWeight = 0;
contracts/auth/AxelarAuthWeighted.sol:94:        uint256 operatorIndex = 0;
contracts/auth/AxelarAuthWeighted.sol:95:        uint256 weight = 0;

Recommendation:

I suggest removing explicit initializations for default values.

---

2. An array’s length should be cached to save gas in for-loops

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.

Instances:

Links: contracts/gas-service/AxelarGasService.sol:123

contracts/gas-service/AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {

contracts/auth/AxelarAuthWeighted.sol: L17, L98, L116

contracts/auth/AxelarAuthWeighted.sol:17:        for (uint256 i; i < recentOperators.length; ++i) {
contracts/auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
contracts/auth/AxelarAuthWeighted.sol:116:        for (uint256 i; i < accounts.length - 1; ++i) {

contracts/AxelarGateway.sol:207

contracts/AxelarGateway.sol:207:        for (uint256 i = 0; i <symbols.length; i++) {

ontracts/deposit-service/AxelarDepositService.sol: L114 L186 L204

contracts/deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
contracts/deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
contracts/deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {

Remediation:

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead.

---

3. <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Instances:

contracts/auth/AxelarAuthWeighted.so;: 70 , 105

contracts/auth/AxelarAuthWeighted.sol:70:            totalWeight += newWeights[i];
contracts/auth/AxelarAuthWeighted.sol:105:            weight += weights[operatorIndex];

References:

https://github.com/code-423n4/2022-05-backd-findings/issues/108

---